-
Notifications
You must be signed in to change notification settings - Fork 16.4k
AIP-72: Move DAG Params to Task SDK #46176
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
ashb
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.
Looks like you missed an import in the tests:
task_sdk/tests/definitions/test_mappedoperator.py:25: in <module>
from airflow.models.param import ParamsDict
E ModuleNotFoundError: No module named 'airflow.models.param'
Which also probably means that we should keep the old module around, but just have it import+"reexport" it from the Task SDK
Yeah, doesn't sound like a bad idea. Let me try that |
|
@ashb moved some more missing references and did some work on moving tests too. Can you take a look when you have some time? |
providers/edge/src/airflow/providers/edge/example_dags/integration_test.py
Outdated
Show resolved
Hide resolved
ashb
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.
Looking good.
A few nits and I think we need to move some more of the tests over too
providers/edge/src/airflow/providers/edge/example_dags/win_notepad.py
Outdated
Show resolved
Hide resolved
providers/edge/src/airflow/providers/edge/example_dags/win_test.py
Outdated
Show resolved
Hide resolved
ashb
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.
Pre-approving for once we've got the tests moved over.
|
Phew, just managed to add the tests, instead of relying on XCOMs, wrote the CustomOperator in such a way that we assert |
|
Very interesting pattern really: At runtime, we parse the |
closes: #44361
Why?
DAG Params are needed to provide runtime information to tasks: https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/params.html. They are generally useful when you want to provide a per DAG Run level configuration for a dag run, lets say some dynamic information.
These work with legacy Airflow 2 and should be ported over to task sdk.
definitionsalong with changing tests, references, documentation (atleast most of it), and general refs.Testing
Basic Testing
DAG used for testing:
Params Trigger UI from example dags
The failures above are because mappedtasks dont work yet
Params UI tutorial DAG demonstrating various options for a trigger form generated by DAG params
Run config:
Logs:
^ 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.rstor{issue_number}.significant.rst, in newsfragments.