I would be in favor of removing the experimental feature of the constructor arguments, but I don't understand the reasoning for removing the override. It is a breaking change from something that was there since 1.X so not really experimental anymore. I think you might underestimate how much it is used. What would be the migration path? It does not execute the same as TearDown / Setup and Moving it to a constructor argument requires it to be a part of a DAG, limiting deployment options.
B. On Sat, 29 Mar 2025 at 13:56, Kaxil Naik <kaxiln...@gmail.com> wrote: > @tamara : Correct, we are now proposing to remove overriding the pre/post > execute > > >Quick question if I am understanding the proposed change correctly. > > > What you want to remove is overriding the pre/post execute when creating > > custom operators: > > > > class MyOperator(BaseOperator): > > > > ... > > > > def pre_execute(self, context): # This would break? > > .... > > def post_execute(self, context): # And this as well? > > But keep the (currently experimental) use of the pre_execute and > > post_execute parameters (I've only used post_execute before for similar > > reasons as TP posted, interacting with outlets assets) > > > On Sat, 29 Mar 2025 at 18:23, Kaxil Naik <kaxiln...@gmail.com> wrote: > > > No, @run_if / @skip_if uses pre_execute from task argument [1] not the > the > > method and is just a syntactic sugar. You can also do the following as an > > example: > > > > ``` > > def skip_at_random(context): > > if randint(0, 1) == 0: > > raise AirflowSkipException() > > > > t2 = BashOperator(task_id='conditional2', pre_execute=skip_at_random, > > dag=dag, bash_command="airflow version") > > ``` > > > > [1]: > > > https://github.com/apache/airflow/blob/8c3a30e3ffc3f114c1d2cc3e6e109f4d9e29ca8b/airflow-core/src/airflow/decorators/condition.py#L57-L59 > > > > On Sat, 29 Mar 2025 at 05:09, Matthew Block <matthew.l.bl...@gmail.com> > > wrote: > > > >> Would this also break @run_if/@skip_if decorators? > >> > >> Best, > >> Matt Block > >> > >> > On Mar 28, 2025, at 3:44 PM, Tamara Fingerlin > >> <tamara.finger...@astronomer.io.invalid> wrote: > >> > > >> > Hey :) > >> > > >> > Quick question if I am understanding the proposed change correctly. > >> > > >> > What you want to remove is overriding the pre/post execute when > creating > >> > custom operators: > >> > > >> > class MyOperator(BaseOperator): > >> > ... > >> > def pre_execute(self, context): # This would break? > >> > .... > >> > > >> > def post_execute(self, context): # And this as well? > >> > > >> > > >> > But keep the (currently experimental) use of the pre_execute and > >> > post_execute parameters (I've only used post_execute before for > similar > >> > reasons as TP posted, interacting with outlets assets) > >> > > >> > BashOperator( > >> > task_id='hello_world', > >> > bash_command='sleep 5', > >> > pre_execute=lambda context: print("Pre-execute function > >> called!"), > >> > # this would still work? > >> > post_execute=lambda context: print("Post-execute function > >> > called!"), # this would be supposed to still work (it does not rn > 😅) > >> > ) > >> > > >> > The one situation I am worried about here is larger teams/orgs using > pre > >> > and post execute to standardize custom operators. For example team A > >> writes > >> > OurCompanyDatabaseBaseOperator that has a pre_execute and post_execute > >> with > >> > mandatory business logic and team B is allowed to write custom > >> operators on > >> > top of that but only override execute. > >> > I assume this would break? > >> > > >> > > >> >> On Fri, Mar 28, 2025 at 9:32 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > >> >> > >> >> I think the current proposal is to remove the pre/post in the Base > >> Operator > >> >> class (and overridability) and leave passing pre/post as constructor > >> >> arguments.. > >> >> > >> >>> On Fri, Mar 28, 2025 at 9:20 PM Bolke de Bruin <bdbr...@gmail.com> > >> wrote: > >> >>> > >> >>> Just one thing - the pre / post mechanisms are executed in-process > of > >> the > >> >>> task rather than the DAG. So they are not equal to setup/teardown. > Are > >> >> you > >> >>> proposing to remove the argument or the whole thing? > >> >>> > >> >>> B. > >> >>> > >> >>> > >> >>> > >> >>>> On Fri, 28 Mar 2025 at 20:58, Jarek Potiuk <ja...@potiuk.com> > wrote: > >> >>> > >> >>>> Indeed. Post/pre overriding in sub-classes should go away (and we > >> could > >> >>>> even likely implement a ruff rule to auto-fix those if someone has > a > >> >>> custom > >> >>>> executor. Sounds like 100% doable > >> >>>> > >> >>>> But passing them as "cross-cutting concerns" via callable in a > >> >>> constructor > >> >>>> is pretty useful and not easily fixable for back-compatibilty > >> >>>> > >> >>>> J. > >> >>>> > >> >>>> On Fri, Mar 28, 2025 at 6:14 PM Kaxil Naik <kaxiln...@gmail.com> > >> >> wrote: > >> >>>> > >> >>>>>> I think the ability of overriding pre_execute and post_execute > in a > >> >>>>> subclass can definitely go away. They are practically useles; you > >> can > >> >>>> just > >> >>>>> put everything in execute, which always needs to exist in a > >> >>> BaseOperator > >> >>>>> subclass anyway. > >> >>>>> > >> >>>>> Yeah I am fine with removing that then. Anyone disagrees? > >> >>>>> > >> >>>>> On Fri, 28 Mar 2025 at 20:36, Michał Modras < > >> michalmod...@google.com > >> >>>>> .invalid> > >> >>>>> wrote: > >> >>>>> > >> >>>>>> I'd prefer a world without separate pre_execute and post_execute > >> >>>>> functions > >> >>>>>> - as pointed out in the PR, they make reasoning about DAGs more > >> >>>> complex, > >> >>>>>> and can be error prone. > >> >>>>>> > >> >>>>>> Having said that, I know there are multiple users relying on > these > >> >>>>>> functionalities, so I'll bring up my usual - another breaking > >> >> change > >> >>> - > >> >>>>>> another obstacle to the AF3 adoption argument. > >> >>>>>> > >> >>>>>> And as for relying on operators vs. PythonOperator + hooks - > there > >> >>> are > >> >>>>> good > >> >>>>>> arguments for continuing relying on operators, or even rely on > them > >> >>>> more, > >> >>>>>> depending on customers' need and organizational setup. > >> >>>>>> > >> >>>>>> On Fri, Mar 28, 2025 at 3:42 PM Tzu-ping Chung > >> >>>> <t...@astronomer.io.invalid > >> >>>>>> > >> >>>>>> wrote: > >> >>>>>> > >> >>>>>>> Passing post_execute as an argument is somewhat useful for > >> >>> operators > >> >>>>> that > >> >>>>>>> don’t support assets natively (most of them) but when you want > to > >> >>>> emit > >> >>>>> a > >> >>>>>>> dynamic path. For example: > >> >>>>>>> > >> >>>>>>> def _send_asset_event(context, result): > >> >>>>>>> # Rendered value! > >> >>>>>>> name = context["task"].output > >> >>>>>>> # Trigger an event against the emitted path. > >> >>>>>>> context["outlet_events"][write_data_outlet].add(Asset(name)) > >> >>>>>>> > >> >>>>>>> write_data_outlet = AssetAlias("data") > >> >>>>>>> > >> >>>>>>> WriteSomeDataOperator( > >> >>>>>>> task_id="write_data", > >> >>>>>>> output="write_data_{{ run_id }}.parquet", > >> >>>>>>> outlets=[write_data_outlet], > >> >>>>>>> post_execute=_send_asset_event, > >> >>>>>>> ) > >> >>>>>>> > >> >>>>>>> Without the functionality, you’ll have to write a subclass for > >> >> each > >> >>>>>>> operator you want to do this, which is quite a bit boilerplate. > >> >>>>>>> > >> >>>>>>> Arguably this is only needed since we use operators too much. > >> >> This > >> >>>>>>> wouldn’t be an issue if we rely more on the PythonOperator+hooks > >> >>>>> approach > >> >>>>>>> (like Bolke discussed at last year’s Airflow Summit), but alas, > >> >>>> people > >> >>>>>>> don’t like to change how they do things, and operators are still > >> >>> very > >> >>>>>>> popular. > >> >>>>>>> > >> >>>>>>> The pre_execute argument *might* also be useful if you want to > >> >>>>>> pre-process > >> >>>>>>> some values. That’s probably a lot less common, so I wouldn’t > >> >> fret > >> >>>> too > >> >>>>>> much > >> >>>>>>> if it goes away. However, since post_execute and pre_execute > >> >>>> basically > >> >>>>>> use > >> >>>>>>> the same implementation, just one run right before and one right > >> >>>> after > >> >>>>>>> execute, they should probably stay or go together. > >> >>>>>>> > >> >>>>>>> I think the ability of overriding pre_execute and post_execute > >> >> in a > >> >>>>>>> subclass can definitely go away. They are practically useles; > you > >> >>> can > >> >>>>>> just > >> >>>>>>> put everything in execute, which always needs to exist in a > >> >>>>> BaseOperator > >> >>>>>>> subclass anyway. > >> >>>>>>> > >> >>>>>>> TP > >> >>>>>>> > >> >>>>>>> > >> >>>>>>>> On 28 Mar 2025, at 22:12, Kaxil Naik <kaxiln...@gmail.com> > >> >>> wrote: > >> >>>>>>>> > >> >>>>>>>> I am in favor of dropping support as they essentially do the > >> >> same > >> >>>> -- > >> >>>>>> and > >> >>>>>>>> setup & teardown is more "native" (first-class UI support) > >> >>>>>>>> > >> >>>>>>>> On Fri, 28 Mar 2025 at 19:41, Kaxil Naik <kaxiln...@gmail.com> > >> >>>>> wrote: > >> >>>>>>>> > >> >>>>>>>>> Hi team, > >> >>>>>>>>> > >> >>>>>>>>> Should we drop support for pre_execute and post_execute for AF > >> >>>> 3.0? > >> >>>>>> They > >> >>>>>>>>> are still marked as experimental [1]. They were added [2] in a > >> >>>> world > >> >>>>>>>>> without Setup and Teardown tasks. > >> >>>>>>>>> > >> >>>>>>>>> Regards, > >> >>>>>>>>> > >> >>>>>>>>> Kaxil > >> >>>>>>>>> > >> >>>>>>>>> > >> >>>>>>>>> [1]: > >> >>>>>>>>> > >> >>>>>>> > >> >>>>>> > >> >>>>> > >> >>>> > >> >>> > >> >> > >> > https://github.com/apache/airflow/blob/7af0319ba16749f4aea78085dfe7823f321d262a/task-sdk/src/airflow/sdk/bases/baseoperator.py#L715-L724 > >> >>>>>>>>> [2]: https://github.com/apache/airflow/pull/17576 > >> >>>>>>>>> > >> >>>>>>> > >> >>>>>>> > >> >>>>>>> > >> >>> > --------------------------------------------------------------------- > >> >>>>>>> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >> >>>>>>> For additional commands, e-mail: dev-h...@airflow.apache.org > >> >>>>>>> > >> >>>>>>> > >> >>>>>> > >> >>>>> > >> >>>> > >> >>> > >> >>> > >> >>> -- > >> >>> > >> >>> -- > >> >>> Bolke de Bruin > >> >>> bdbr...@gmail.com > >> >>> > >> >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >> For additional commands, e-mail: dev-h...@airflow.apache.org > >> > >> > -- -- Bolke de Bruin bdbr...@gmail.com
