+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 >
