-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Improve handling edge-cases in airlfow.models by applying mypy #20000
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
Conversation
This comment has been minimized.
This comment has been minimized.
|
AH NOOO!!!!!! I MISSED THE #20000 one! Bummer! |
|
It’s probably because Pendulum invents its own timezone class (which is fine) and expects to only receive that timezone class in its own datetime class (which is not fine). I’ll take a look. |
0680d24 to
4387320
Compare
268bf7b to
dd2b124
Compare
d061138 to
14d3d35
Compare
airflow/models/taskinstance.py
Outdated
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.
For other reviewers wondering, this method returns a command list (for e.g. subprocess), and passing in a None would not work, so all existing usages of this function should already be passing in a run_id, making this change safe.
|
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. |
ba59f07 to
52e459a
Compare
|
I might need to merge #20079 first -- this is crashing in building docs for some reason. |
airflow/models/dagrun.py
Outdated
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.
This is what is causing the docs build to fail.
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.
I have a fix for it -- so we can merge this and spend a bit more time looking at the sphinx upgrade.
d6a3fb2 to
b324cbd
Compare
And to fix these, I needed to fix a few other mistakes that are used/called by DAG's methods
Also moved the sentinel value implementation to a utils module. This should be useful when fixing typing issues in other modules.
|
Conflict is only in the import section: so once these tests pass I'll fix conflict then merge without waiting for another test run. |
|
Green build https://github.com/apache/airflow/actions/runs/1549766290 -- rebasing + merging. |
b324cbd to
fef2974
Compare
And to fix these, I needed to fix a few other mistakes that are
used/called by DAG's methods.
Relates to #19891 - it doesn't completely fix any packages, but this change was getting larger.
/cc @khalidmammadov