-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Un-ignore DeprecationWarning #20322
Un-ignore DeprecationWarning #20322
Conversation
No worries. We won't close it :) |
Yeah, failed at 50 (the max failures we've specified) in the "Other" tests :( |
5774b8e
to
d7dac20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far.
Alright this is working. I’m going to split the typing changes into a separate PR (that can likely be merged quickly) and rebase the rest on top of that. |
Unfortunately, external task sensors reference "execution date" quite extensively, and even include it as a part of its public API (both that exact variable, and things like execution_offset and excution_date_fn). It is definitely possible to migrate them to something else, but it is some substential work, and I actually suspect these aren't used widely, if by anyone at all, since we did not receive a single report pointing out those are currently emitting DeprecationWarning messages from core Airflow code. So I'm going to leave the public interface alone for now, and only change the internals to not emit warnings.
Similar to ExternalTaskSensor, the execution date terminology is unfortunately a part of its public API. This one is easier to deprecate, but again, it's entirely unclear how many people are actually using this, especially now we have async triggers. So I'm leaving the interface alone.
Again, execution date is unfortunately a part of the operator's public interface. But again, better to leave it again. 🤷
Where we actually want to make sure those arguments work properly.
To avoid deprecated context variables emitting warnings, we must call determine_kwargs directly on the context mapping first, instead of using the make_kwargs_callable wrapper, which uses **kwargs and eagerly access all the members.
Similar to other Jinja rendering situations, we can't just pass **context into Template.render().
Similar to DayOfWeekSensor and BranchDateTimeOperator, this also has execution date in its public interface. But let's not deal with it now.
Same as the regular handler.
More of the same thing.
Due to how Python works, we can't make the keyword argument dict lazy, and calling fn(**context) would emit deprecation warnings too eagerly. The previous warning strategy with lazy-object-proxy is brought back for this particular use case, and only deprecated entries are converted to lazy proxies if the context is being unpacked, to keep possible incompatibilities minimal.
bffabd3
to
b3b780f
Compare
Rebased on remove changes separated to #20376. I edited the original PR description to add details on the changes. |
Can I get some reviews on this one? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we removed the warning filter from the ci cmdline?
(cherry picked from commit 9876e19)
I introduced a flag in #20217 to help us not accidentally use deprecated context variables. This works locally, but I failed to notice it does not work in CI because our scripts explicitly pass
--pythonwarnings=ignore::DeprecationWarning
, which overrides my added config. Consequently, quite several deprecated usages leaked intomain
.This PR properly fixes all those deprecated usages. Also, since no explainations were offered at the time the flag was added (it’s been there ever since Pytest was adopted as the test runner), I removed that ignore flag entirely. This should help us identify more deprecated usages (there are quite several) and fix them eventually to work toward #19788.