Description
There are a number of Sensors that now support deferrable mode; that is, passing the recurring Sensor functionality to a Triggerer. We're testing this out in the latest amazon provider in particular.
We're generally very happy with it! However, we did notice behavior changes, in that by setting mode deferrable=True, we lose the ability to use the underlying BaseSensorOperator attributes like soft_fail and timeout.
Why I'm submitting this as a Feature Request rather than a Bug is that it seems that this may be deliberate based on other code surrounding deferrable operators:
- In some deferred sensors,
timeout is recalculated based on new params rather than reusing the BaseSensorOperator attribute, and is done per-sensor rather than in an upstream class parameter
- Sensors are now deliberately overwriting the
execute when in deferrable mode which clearly removes a lot of built-in BaseSensorOperator functionality
- The addition of "deferrable sensors" does seem to be a core initiative based on the references to conf operators/default_deferrable, suggesting the implementation strategy has been reviewed.
However, I do question this approach; with the ability to set default_deferrable to true, this hazards a lot of confusion around backwards compatibility, as sensors will operate under different parameters once this switch is flipped. Additionally, it makes the transition to deferrable sensors (in my limited experience thus far, a superior approach compared to the default) more rocky, as every single implementation of sensors will need to be reviewed for the proper parameters (as an example from the exact same module, EmrContainerSensor uses max_attempts whereas EmrJobFlowSensor uses max_retries).
So before starting I wanted to assess:
- Is there any appetite for a shift in direction here, or is this a settled approach that each deferrable Operator will custom-implement Triggers?
- If so, are there any thoughts on appropriate implementation? My preferred would be to implement support for
deferrable into the BaseSensorOperator execute function (at least in the amazon provider, the overwritten execute() seems to match or almost match in every case), and then remove custom execute functions from the deferrable sensors.
- Finally, given that the
deferrable code has already made its way into the main, if there's agreement on the above, should deferrable sensors be marked as Experimental to support the above refactor without surprising anyone down the line?
Use case/motivation
We'd like to be able to continue to use soft_fail when Sensor Triggerers are enabled to mark a task as Skipped rather than Failed when a sensor expires. It would also be nice to have alignment between deferrable sensor timeout parameters and calculations rather than being implemented on a per-sensor basis.
Related issues
No response
Are you willing to submit a PR?
Code of Conduct
Description
There are a number of Sensors that now support
deferrablemode; that is, passing the recurring Sensor functionality to a Triggerer. We're testing this out in the latestamazonprovider in particular.We're generally very happy with it! However, we did notice behavior changes, in that by setting mode
deferrable=True, we lose the ability to use the underlyingBaseSensorOperatorattributes likesoft_failandtimeout.Why I'm submitting this as a Feature Request rather than a Bug is that it seems that this may be deliberate based on other code surrounding deferrable operators:
timeoutis recalculated based on new params rather than reusing the BaseSensorOperator attribute, and is done per-sensor rather than in an upstream class parameterexecutewhen indeferrablemode which clearly removes a lot of built-inBaseSensorOperatorfunctionalityHowever, I do question this approach; with the ability to set
default_deferrabletotrue, this hazards a lot of confusion around backwards compatibility, as sensors will operate under different parameters once this switch is flipped. Additionally, it makes the transition to deferrable sensors (in my limited experience thus far, a superior approach compared to the default) more rocky, as every single implementation of sensors will need to be reviewed for the proper parameters (as an example from the exact same module, EmrContainerSensor usesmax_attemptswhereas EmrJobFlowSensor usesmax_retries).So before starting I wanted to assess:
deferrableinto theBaseSensorOperatorexecutefunction (at least in theamazonprovider, the overwrittenexecute()seems to match or almost match in every case), and then remove customexecutefunctions from the deferrable sensors.deferrablecode has already made its way into themain, if there's agreement on the above, shoulddeferrablesensors be marked asExperimentalto support the above refactor without surprising anyone down the line?Use case/motivation
We'd like to be able to continue to use
soft_failwhen Sensor Triggerers are enabled to mark a task as Skipped rather than Failed when a sensor expires. It would also be nice to have alignment between deferrable sensor timeout parameters and calculations rather than being implemented on a per-sensor basis.Related issues
No response
Are you willing to submit a PR?
Code of Conduct