-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Add parsing context to DAG Parsing #25161
Conversation
This is the first attempt to implement the "robust execution context" as a follow up after #25121 . I am not 100% if I got everything, but I think it might be quite close. @pingzh I know you have a ton of experience with varioous runners and the way they are implemented is a bit "convoluted" so I'd appreciate thorough review (note that it is based on #25121 which was documentation only so only last commit matter. |
6787927
to
092b36c
Compare
A bit cleaner interface (also appropriate to be implemented as "future". |
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 like this idea.
one thing is if https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-45+Remove+double+dag+parsing+in+airflow+run is released, we don't need to inject the context in the executor level, as airflow tasks run --local
process does not parse dag file anymore. This means we only need the context manager in the task_runner
092b36c
to
d79b073
Compare
d79b073
to
8ee0d60
Compare
Addressed all comments and got rid of the "non-robust" solution. |
b6dc941
to
bb067a8
Compare
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.
General point: lets mark anything we do here as experimental as Dag Fetcher (AIP-5) may give us a nicer way of doing this
We did already:
|
850521b
to
de70620
Compare
I think it should get green this time |
Ah, but it's not marked with the "experimental" sphinx tag https://github.com/apache/airflow/blame/main/docs/apache-airflow/listeners.rst#L31 |
Added. |
de70620
to
5b669f8
Compare
Jsut remaining question is about where to inject the parsing_context - @pingzh - any comments here? |
059d9d0
to
5ec6f06
Compare
Now I think it should all be fine. |
5936a4c
to
1b082af
Compare
Right. Finally Green. |
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.
LGTM other than the compat issue
1b082af
to
467ef2f
Compare
Adds proper context in the form of context managers setting evironment variables to indicate whethere the dag file is parsed in context of DAG processor or Task Execution and allows to retrieve DAG_ID and TASK_ID easily.
d56f68c
to
36c9b75
Compare
Adds proper context in the form of context managers setting
evironment variables to indicate whethere the
dag file is parsed in context of DAG processor or Task Execution
and allows to retrieve DAG_ID and TASK_ID easily.
^ 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.