-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Better typing for Job and JobRunners #31240
Conversation
cc: @uranusjr -> I'd appreciate your keen eye on it, I am not 100% sure if this is the right way of doing it, typing system of Python when it comes to Generics is kinda convoluted and I am still learning it. The way I implemented it, however should explain the intent I had, but maybe there are better ways of implementing the intent. |
ad2cb93
to
b690b8b
Compare
Thanks @uranusjr - yep, this is much nicer now :) |
b690b8b
to
e345c9b
Compare
e345c9b
to
f5aac1e
Compare
Actually - I am at a loss now :). had to remove it all from TYPE_CHECKING actually (Generic cannot have types defined in TYPE_CHECKING). ... And when I did ....
I even tried to cast it and ...
Typing is hard :) |
Somehow mypy thinks JobOrJobPydantic is actually JobPydantic :) |
Aactually - turned out to be quite a bit more - moving out of TYPE_CHECKING causes (of course) circular imports so things get messy (have to rethink it) |
@potiuk Might using a protocol here be a fix? Rather than trying to say JobOrJobPydantic, define a Protocol that has the type we need, which I think might be just this: class JobProtocol(Protocol):
job_type: str |
Yeah. That was my next idea to try :) |
By avoiding setting the job in the BaseJobRunner, the typing for Runners and Job and JobPydantic is now more complete and accurate. Scheduler and Backfill Runners limit their code to Job and can use all the things that ORM Job allows them to do Other runners are limited to union of Job and JobPydantic version so that they can be run on the client side of the internal API without having all the Job features. This is a follow up after apache#31182 that fixed missing job_type for DagProcessor Job and nicely extracted job to BaseRunner but broke MyPy/Typing guards implemented in the runners that should aid the AIP-44 implementation.
f5aac1e
to
a577260
Compare
Actually, it was easier than I thought. All that was needed was to move setting This (counterintuitively - we do not set job as a field in baseRunner, it makes perfect sense.
|
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’ll experiement some other approaches later as well, this assignment still fits better in the base class.
By leveraging Generics, the typing for Runners and Job and JobPydantic is now more complete and accurate.
Scheduler and Backfill Runners limit their code to Job and can use all the things that ORM Job allows them to do
Other runners are limited to union of Job and JobPydantic version so that they can be run on the client side of the internal API without having all the Job features.
This is a follow up after #31182 that fixed missing job_type for DagProcessor Job and nicely extracted job to BaseRunner but broke MyPy/Typing guards implemented in the runners that should aid the AIP-44 implementation.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.