-
Notifications
You must be signed in to change notification settings - Fork 16.6k
AIP-86 - Add Deadline References#50677
Conversation
Adds the framework for DeadlineReferences, along with a few examples and unit tests.
^ 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 airflow-core/newsfragments.
1fanwang
left a comment
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.
Thanks for the PR, LGTM, minor comments
|
Yeah, I don't like some of those changes in the last commit. I have another commit coming which rolls some of those changes back and also implement TP's suggestion. It made the models/deadline.py somewhat complicated so I moved these changes (and the relevant tests) into their own file(s), which is going to break your comment links. Sorry in advance. I'll push the new commit once mypy and static checks pass locally |
uranusjr
left a comment
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.
Another approach that may improve the evaluation interface may be to make the kwargs a required dict argument instead.
reference.evaluate_with()
# This might read better?
reference.evaluate_with(context={})
# Makes supplying values a bit verbose though.
reference.evaluate_with(context={"foo": 123})
- Update unit tests accordingly
- Break references into a new module deadline_reference.py to avoid cluttering deadline.py
- Roll back part of the previous commit that allowed _fetch_from_db to return datetime | None. If there is no datetime, we need to fail
a687fbc to
3ba1581
Compare
|
ah, found the email thread I was looking for; I always forget that the list's search function defaults to "newer than a month". Based on that discussion, I'm dropping the caplog part of the tests. |
|
Some minor comments above; none of them are blocking, just questions. |
|
@ashb - When you get a second can you have a look and see if I broke this down right? The part of the code that the user directly interacts with (and might be imported into a DAG) now lives in I think when I asked about it previously, the breakdown was "required db" vs "non-db" but this feels like a more natural break for this... is it still alright or do i need to put it back to all in the sdk path except the _fetch_from_db method? |