+1. I view it as adding a feature vs breaking compatibility. On Thu, Aug 4, 2022 at 4:15 PM Ferruzzi, Dennis <ferru...@amazon.com.invalid> wrote:
> I definitely like it, I love reducing boilerplate code like that. > > > ------------------------------ > *From:* Ash Berlin-Taylor <a...@apache.org> > *Sent:* Tuesday, August 2, 2022 3:43 AM > *To:* dev@airflow.apache.org > *Subject:* [EXTERNAL] Auto-registering of DAGs in DAG file? (no `as dag` > needed?) > > > *CAUTION*: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > 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 > >
