Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jedcunningham commented on PR #61436: URL: https://github.com/apache/airflow/pull/61436#issuecomment-3855088718 > Even though it's pointed out as bad practice as This just formalizes that it's best practice to have triggers come from elsewhere on sys.path instead to avoid that problem. in https://github.com/apache/airflow/pull/48603 > > From the user perspective, I would expect my triggers defined in Dag files could be imported by Triggerer, but the Triggerer doesn't respect AIRFLOW__CORE__DAGS_FOLDER currently. Don't focus as much on my PR description, but the changes in the PR. It's not just "best practice" not to in Airflow 3, we intentionally removed the ability to have them in a dag file. Also, don't focus on "dags folder" - you must have a multi-bundle mental model now. The "dags folder" config is simply a default for familiarity sake. It is completely valid to have a deployment that does not use `[core] dags_folder` at all, and any change referencing it directly should immediately raise a yellow flag :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jason810496 commented on PR #61436: URL: https://github.com/apache/airflow/pull/61436#issuecomment-3850934187 Thanks again for sharing the context! I will workaround with `environment_variables.env` for now, as defining triggers in Dag files is not best practice and restoring Triggerer aware of the Dag Bundle will not happen soon. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jason810496 closed pull request #61436: Add /files/dags to PYTHONPATH in Breeze URL: https://github.com/apache/airflow/pull/61436 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jason810496 commented on PR #61436:
URL: https://github.com/apache/airflow/pull/61436#issuecomment-3850928744
Thanks Jarek and Jed for sharing the context.
I think there two parts we pointed out here
1. Whether the triggers are importable in Dag files
2. The refreshing mechanism for triggers
> if we see a problem that it is not added, we should fix it.
Agreed.
For first part:
Even though it's pointed out as bad practice as `This just formalizes that
it's best practice to have triggers come from elsewhere on sys.path instead to
avoid that problem.` in https://github.com/apache/airflow/pull/48603
From the user perspective, I would expect my triggers defined in Dag files
could be imported by Triggerer, but the Triggerer doesn't respect
`AIRFLOW__CORE__DAGS_FOLDER` currently.
> In fact, triggers from Dag files was pretty broken before anyways, so we
intentionally removed that ability in AF3.
We are able to restore the importing triggers from Dag files by adding
`sys.path.insert(0, conf.get("core", "dags_folder"))` before entering
Triggerer's async loop.
https://github.com/apache/airflow/blob/fe0633d729c85131ca96aa41a8c56282a407b7d5/airflow-core/src/airflow/jobs/triggerer_job_runner.py#L885-L890
> as the triggerer is unable to account for changes without having a restart.
Then this level of problem will be second part:
Unless we refactor Triggerer to run the triggers by
`execution_time/task_runner` instead of running them as pure `asyncio`
coroutines, we will not be able to make the Triggerer aware of the Dag Bundle.
This will need much more discussion and effort and might not be finished soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jedcunningham commented on PR #61436: URL: https://github.com/apache/airflow/pull/61436#issuecomment-3849447168 See #48603. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
jedcunningham commented on PR #61436: URL: https://github.com/apache/airflow/pull/61436#issuecomment-3849427290 Correct. In fact, triggers from Dag files was pretty broken before anyways, so we intentionally removed that ability in AF3. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add /files/dags to PYTHONPATH in Breeze [airflow]
potiuk commented on PR #61436: URL: https://github.com/apache/airflow/pull/61436#issuecomment-3847216206 Hmm. I think that should not be the case - at least by default when Airflow imports Dags (whether it's in Triggerer, worker or Dag Processor), it should add bundle root to the PYTHONPATH automatically - and we should **avoid** adding it externally, rather than that - if we see a problem that it is not added, we should fix it. @jedcunningham @ephraimbuddy - am I right ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
