Will this impact DAG file processing time? If we consider to include the change, we might also need to consider informing the user that such functions need to be lightweight inline with what we've here for top-level-code best practices: https://airflow.apache.org/docs/apache-airflow/stable/best-practices.html#top-level-python-code
On Tue, 2 Aug 2022 at 16:48, Felix Uellendall <felue...@pm.me.invalid> wrote: > Hey Ash, > > I personally don't like it, because it is not obvious to me. > > Also what happens if you return the `dag_2` variable and set the return > value in the global context to `dag_2` as well? This is how I used to do it > when generating DAGs - and in my opinion this is pythonic way of doing it > without any magic. I mean magic is nice as long as it works.. > > Keep in mind that also some people use functions to hide the dag creation > i.e. factory pattern to clearly separate it from callers context (e.g. > business logic). Your solution would blurry this line. > > So I am leaning towards a "No", but keen to know what others think :) > > Best, > Felix > > > Sent with Proton Mail <https://proton.me/> secure email. > > ------- Original Message ------- > On Tuesday, August 2nd, 2022 at 12:43, Ash Berlin-Taylor <a...@apache.org> > wrote: > > Hello all, > > I'm on a bit of a kick thinking about developer (specifically DAG author) > experience and if there is anything we can > > Some time ago there was a previous conversation about if we should/could > "autoregister" DAGs, rather than just looking at the objects in the top > level (globals()) of a file, an I knocked up this PR > > https://github.com/apache/airflow/pull/23592 > > The question I have for you all is do we think this is good idea? It does > somewhat subtly change the behaviour in a few cases. Lets take this example > this from the docs > https://airflow.apache.org/docs/apache-airflow/stable/concepts/dags.html#loading-dags > > dag_1 = DAG('this_dag_will_be_discovered') > def my_function(): > dag_2 = DAG('but_this_dag_will_not') > > > my_function() > > As implemented right now the second dag won't get picked up (as the auto > registration is handled in the context manager, but if the example was > changed to use a context manager it will get loaded/discovered: > > with DAG('this_dag_will_be_discovered'): > > EmptyOperator(task_id='task') > > > def my_function(): with DAG('so_will_this_dag_now'): > > EmptyOperator(task_id='task') > > > my_function() > > With the change in my PR both DAGs would be picked up. Does that count as > a breaking change do you think? Is this behaviour more helpful to users, or > do we think it would be confusing? > > (If I get a few thumbs up I will update the docs in my PR to cover this > new behaviour.) > > -ash > > > -- Best regards, Pankaj Koti +91 97300 79985
