For me having to add “as dag” is somewhat non-evident. Also, it’s a few less boilerplate characters you have to write which I think is a good thing in this case.
Curious on the edge cases in DAG generation as mentioned by Felix, but I like the idea so +1 for me. Bas > On 2 Aug 2022, at 13:18, 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 >> <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 >> >> <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 >> >
