Sane points, keeping it for now :) On Mon, 31 Mar 2025 at 00:00, Jarek Potiuk <ja...@potiuk.com> wrote:
> Yeah. I am convinced by Bolke's and Constance posts. Changing my vote to > -0.9. > > On Sun, Mar 30, 2025 at 8:25 PM Constance Martineau > <consta...@astronomer.io.invalid> wrote: > > > Just to add to the discussion: > > > > Removing these overrides will disproportionately impact teams at > companies > > with more than a handful of Airflow users. Unless there’s a clear benefit > > to removing them *and* a well-defined migration path, I don’t think it’s > > worth the disruption. +1 to Bolke’s points. > > > > Like Tamara mentioned, we frequently see teams build internal providers > > where they subclass standard operators and use pre_execute/post_execute > to > > inject things like service-specific auth, tracking identifiers, or other > > cross-cutting concerns. These hooks act as a central entry point for > > operational consistency, especially in environments where DAG authors > > aren’t expected to manage those behaviors themselves. > > > > Pushing that logic down to every DAG or task definition would be a pretty > > big shift for those teams, and, from what I’ve seen, a regression in > terms > > of maintainability and user experience. > > > > On Sun, Mar 30, 2025 at 4:33 AM Michał Modras > > <michalmod...@google.com.invalid> wrote: > > > > > +1 to Bolke's points > > > > > > niedz., 30 mar 2025, 10:30 użytkownik Bolke de Bruin < > bdbr...@gmail.com> > > > napisał: > > > > > > > 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 > > > > <https://teams.googleplex.com/u/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 > > > > > > > > > >
