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
