Un-ignore DeprecationWarning #20322
Merged
Un-ignore DeprecationWarning #20322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No worries. We won't close it :) |
|
Yeah, failed at 50 (the max failures we've specified) in the "Other" tests :( |
uranusjr
force-pushed
the
no-ignore-deprecations
branch
3 times, most recently
from
5774b8e
to
d7dac20
Compare
ashb
reviewed
Dec 16, 2021
|
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.
uranusjr
force-pushed
the
no-ignore-deprecations
branch
from
bffabd3
to
b3b780f
Compare
uranusjr
requested review from
kaxil,
mik-laj,
potiuk and
XD-DENG
as code owners
|
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? |
kaxil
approved these changes
Dec 20, 2021
|
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. |
github-actions
bot
added
the
full tests needed
ashb
approved these changes
Dec 21, 2021
This was referenced Dec 31, 2021
jedcunningham
pushed a commit
that referenced
this pull request
Jan 21, 2022
(cherry picked from commit 9876e19)
40 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.