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
