Less is more. I like the lack of "as dag". I think it's not really a breaking change.
We can easily argue we are adding functionality rather than introducing a breaking change. I think the only reason why someone would create a DAG in a function is to return it and eventually add it to globals(). That's what also a number of guides on the internet will tell people to do. And if they did that, that is no-change for them - the DAG will be added to globals automatically, adding it again is a no-op. Surely it needs an explanation, something along the lines "It used to be needed to add to globals, now it's not. If you actually created a DAG and did not add it to globals, then you really have to rethink what and why you are doing it :). J.. On Tue, Aug 2, 2022 at 2:09 PM Felix Uellendall <felue...@pm.me.invalid> wrote: > Ah I should have checked your PR, sorry. I was looking at the first > example. In general I like the idea of removing the `as dag` in the context > manager syntax. > > Best, > Felix > > > > Sent with Proton Mail <https://proton.me/> secure email. > > ------- Original Message ------- > On Tuesday, August 2nd, 2022 at 13:29, Pankaj Koti < > pankajkoti...@gmail.com> wrote: > > 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 > > >
