Deprecate smart sensors #20151
Deprecate smart sensors #20151
Conversation
boring-cyborg
bot
added
area:Scheduler
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
|
What do you think about using deprecated on the Sensor Class? It gives a very nice message when a deprecated function/class is used. e.g: |
Fully agree. And also we should add a note in our docs, explaining at the very least how the users should approach the migration as correctly pointed out by @turbaszek in the devlist. I think this should be standard of any deprecation we do. We have done quite a bad job in the past assuming that our users read all the discussions and AIPs we write. They don't. They need some guidance. |
airflow/jobs/scheduler_job.py
Outdated
| @@ -145,6 +145,12 @@ def __init__( | |||
|
|
|||
| self.dagbag = DagBag(dag_folder=self.subdir, read_dags_from_db=True, load_op_links=False) | |||
|
|
|||
| if conf.getboolean('smart_sensor', 'use_smart_sensor'): | |||
| warnings.warn( | |||
| 'Smart sensors are deprecated. Please use Deferrable Operators instead.', | |||
There was a problem hiding this comment.
Do you'll think adding a link to the docsite for deferrable operators will be helpful?
@potiuk @turbaszek @eladkal and others ?
There was a problem hiding this comment.
Yep . As described in the devlist. We definitely need a place in the doc (and linked from here) where we explain (shortly):
- why we cannot provide precise migration instructions
- what should be general approach of the user who has to migrate. Something alongside "For every smart sensor you use, you have to either use or write your own defferable operator if want to achieve similar results. If you want to know how to write your own operator - find it [here]" (another link)
We should simply assume that the users have not read the docs, know nothing about deferrable operators and they should understand that they need to write they own deferrable operator instead BEFORE they read the whole documentation (which we shoudl helpfuly link so that they do not have to google it or search the documentation).
That's teh "approach I 'd use for any deprecation" . I think those two pieces are crucial:
- Explaining "why" we cannot give them instructions
- Explaining 'what" they need to do before actually reading the documentation is crucial (and link to detailed instruction on how they approach it).
The 1) prevents questions "Please give me exact instructions on how to remove the deprecation?"
The 2) prevents questions "What do I need to to do now?".
That's it. No more, no less.
It's simply giving the right answers to the anticipated questions straight there when user sees the warning.
There was a problem hiding this comment.
If we can be more explicit about what needs to be done - we should.
However I believe that if we know that a feature is deprecated we should say it as soon as possible - regardless if we have all the needed docs / explanations. At the end the explanations are for users who need to migrate from the deprecated features to the new one BUT there are also users who may just now playing with the feature and thinking to start use it - for them we just need to say that it's deprecated. They don't need migration docs.
So this is a tradeoff. i'm happy with what we have so far. We can always add more information.
Also want to add that we've been in this case before :)
TaskGroups <-> SubDags are also not 1-to-1
jedcunningham
force-pushed
the
deprecate_smart_sensors
branch
from
3957d3d to
0c44268
Compare
|
Ready for another look. I've:
The SchedulerJob message is intended for the folks operating Airflow, and the message in the task log for end users. |
|
Fantastic @jedcunningham! Thank you. This is perfect! |
Smart sensors are being replaced with Deferrable Operators. As they were marked as an early-access feature, we can remove them before Airflow 3.
Co-authored-by: Tomek Urbaszek <[email protected]>
jedcunningham
force-pushed
the
deprecate_smart_sensors
branch
from
2d5fffd to
019e165
Compare
jedcunningham
requested review from
ashb,
dstandish and
XD-DENG
as code owners
Smart sensors are being replaced with Deferrable Operators. As they were marked as an early-access feature, we can remove them before Airflow 3. (cherry picked from commit 77813b4)
Smart sensors are being replaced with Deferrable Operators. As they were marked as an early-access feature, we can remove them before Airflow 3. (cherry picked from commit 77813b4)
jedcunningham
added
type:misc/internal
Smart sensors are being replaced with Deferrable Operators. As they were
marked as an early-access feature, we can remove them before Airflow 3.
(Don't merge before December 15th: https://lists.apache.org/thread/3t6v6n8cz5hrmc1wjqp728v0dv5hl18q)