Skip to content
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

Merged
merged 20 commits into from
Dec 21, 2021
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 15, 2021

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 into main.

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.

@potiuk
Copy link
Member

potiuk commented Dec 15, 2021

No worries. We won't close it :)

@ashb
Copy link
Member

ashb commented Dec 15, 2021

Yeah, failed at 50 (the max failures we've specified) in the "Other" tests :(

@uranusjr uranusjr force-pushed the no-ignore-deprecations branch 3 times, most recently from 5774b8e to d7dac20 Compare December 16, 2021 17:51
Copy link
Member

@ashb ashb left a 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.

@uranusjr
Copy link
Member Author

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.
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 uranusjr marked this pull request as ready for review December 17, 2021 12:07
@uranusjr
Copy link
Member Author

uranusjr commented Dec 17, 2021

Rebased on remove changes separated to #20376. I edited the original PR description to add details on the changes.

@uranusjr
Copy link
Member Author

Can I get some reviews on this one?

@github-actions
Copy link

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 github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 20, 2021
@uranusjr uranusjr merged commit 9876e19 into apache:main Dec 21, 2021
@uranusjr uranusjr deleted the no-ignore-deprecations branch December 21, 2021 10:00
Copy link
Member

@ashb ashb left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants