Re: [DISCUSS] AIP-52 updates - setup / teardown tasks

[Jarek Potiuk <ja...@potiuk.com>]

>  we require that a setup always have a teardown

I'd personally prefer this one because "explicit is better than implicit".
I see the cases where it could be useful (I was also contemplating no
setup/no teardown before), but I think the wiggly behaviour is a recipe for
a number of mistakes (and issues raised by users), especially when someone
modifies existing DAG and adds teardown without realising the consequences.

Not a strong preference, I am ok with both, but if I got to choose, I'd
require teardown.

J.




On Fri, Jun 30, 2023 at 10:28 PM Daniel Standish
<da...@astronomer.io.invalid> wrote:

> Ok want to pick back up setup / teardown discussion as we get closer to
> 2.7.
>
> For personal reasons I had to take some time off work just as we were
> wrapping up work on 2.6, and at least partly due to that, we punted setup /
> teardown to 2.7.
>
> But we've picked it back up and continued along the path outlined in this
> thread and things look good for 2.7.
>
> In the time since the previous discussion, not a lot has changed except
> that we've implemented in main some of the things discussed.
>
> For one we added a one-liner syntax.
>
> So now if you have an existing dag such as this:
>
> create_cluster >> run_query >> delete_cluster
>
> You can convert it to use setup / teardown with the following:
>
> create_cluster >> run_query >>
> delete_cluster.as_teardown(setups=create_cluster)
>
> The context manager suggested by Ash has been added, so if you have setup
> `s` and teardown `t`, you can do the following:
>
> with s >> t:
>     some_work(other_work())
>
> Notably, just as with task group context manager, you must instantiate the
> task inside the context in order for it to be registered.  Ephraim is
> working on some improvements to the context manager to address this (so
> that you can explicitly add tasks to the context in that scenario) and so
> that we can throw an error on non-sensical usages such as `with a >> b >>
> c:`
>
> There is a docs PR outlining the current state of the feature here
> <https://github.com/apache/airflow/pull/32169> and you are all welcome to
> comment.  There's still time to make adjustments if desired.
>
> But I wanted to raise one point for discussion here.  It's something I
> could have gone either way on, and I want to tap into the wisdom of the
> community.
>
> The question of whether a setup with no teardown, or a teardown with no
> setup, should be added.
>
> There are two options as I see it.
>
> Option 1: if a setup has no teardown, then all the downstreams are assumed
> to be "in scope" for the setup
> Note: the notion of "scope" is about the clearing behavior aspect of setup
> / teardown: you clear a task, it clears its setups too.  A work task is in
> the scope of s and t if it's between s and t.
> So then with s1 >> w1 >> w2, clearing work task w1 would clear w1 w2 and
> s1.
> And clearing w2 would clear w2 and s1.
> This makes it easy to have a setup with no teardown. But it makes the
> behavior slightly more... wiggly... for lack of a better term.  With this
> example, s1 is a setup for w2.  But then suppose you add a teardown after
> w1: s1 >> w1 >> [w2, t1.as_teardown(setups=s1)].  Now s1 is not a setup for
> w2.
> So the logic is ... "a work task is in scope of a setup if it is in between
> the setup and its teardown, *or *if the setup has no teardowns".
>
> Option 1: we require that a setup always have a teardown
> With this approach, the rule is simpler and it forces unambiguously defined
> scopes.  If you just want to use a setup and don't need a teardown task,
> you add an empty teardown.  The rule is then "a work task is in scope of a
> setup if it is in between the setup and its teardown" -- i.e. the same rule
> minus the `or` clause.
> So the example would have to be s1 >> w1 >> w2 >>
> EmptyOperator().as_teardown(setups=s1).
>
> Thanks in advance for your engagement.
>
> P.S. note that those of us in the U.S. will be off for holiday soon so may
> be slower to respond but just wanted to get this conversation started so
> that we can get this feature to a place that people generally feel good
> enough about in 2.7.
>
> P.P.S. Random-ish note... we should consider marking some features as beta
> I think.  There's a difference between beta and experimental, though
> historically we've only employed the latter.  But I think beta can be good
> to signal "we are committed to this feature we just might make a change or
> two to the API".  Not saying we should do this with setup / teardown
> necessarily but it's an option.  Sometimes I think it can be a little
> overwhelming to put out features with the spectre of being locked in by
> backcompat guarantees and perhaps something like a beta feature could be a
> way to get things out there a little bit more freely and allow for a
> feedback loop.
>
> Thanks again,
>
> Daniel
>

Re: [DISCUSS] AIP-52 updates - setup / teardown tasks

[Daniel Standish <da...@astronomer.io.INVALID>]

You're not too late, and thanks for engaging with the issue.

I don't think anyone would dispute that users will sometimes want a setup
without a teardown.  But the question is should we require that users
explicitly make the scope of a setup well-defined.  Like Jarek, I have some
ambivalence on the question but lean toward requiring explicitness, though
I can accept the alternative.

Note currently we're not requiring an empty *setup* for unmatched teardowns
since it wouldn't make a difference anyway; i.e. the notion of "scope" is
only really a question for setups; for teardowns there's no material impact.

An aside concerning associations with unittests / pytest... I think it's
fair to say that this AIP took some inspiration from unittests / pytest.
But the goal of the AIP is not to replicate the API of unittests / pytest
in airflow.  Indeed airflow provides a much richer vocabulary for
expressing task pipelines, and this AIP adds to the richness of that
vocabulary.  And fundamentally, the goal is not to adhere to some aesthetic
ideal or perfect consistency with some other API but rather to provide
useful functionality for expressing real world pipelines and solving real
world problems in Airflow.  And along these lines I think that a 1-1
correspondence with unittests is neither something we should aspire to nor
be limited by (not that you're suggesting this, but it comes up).  Airflow
is something different from unittess, and the setup and teardown
functionality is something different from unittests / pytest,
notwithstanding the reasonable analogies we can observe.

Re: [DISCUSS] AIP-52 updates - setup / teardown tasks

[utkarsh sharma <ut...@gmail.com>]

Hey everyone,

I hope I'm not late to the discussion. :)

IMO, there is a similarity between Airflow's Setup/Teardown and
Pytest's Setup/Teardown, and Pytest allows setup and teardown to exist
independently. There are cases where you might need one and not the other.
But I'm not sure of any airflow-specific concerns here.

Thanks,
Utkarsh

On Tue, Jul 4, 2023 at 3:45 PM Jarek Potiuk <ja...@potiuk.com> wrote:

> >  we require that a setup always have a teardown
>
> I'd personally prefer this one because "explicit is better than implicit".
> I see the cases where it could be useful (I was also contemplating no
> setup/no teardown before), but I think the wiggly behaviour is a recipe for
> a number of mistakes (and issues raised by users), especially when someone
> modifies existing DAG and adds teardown without realising the consequences.
>
> Not a strong preference, I am ok with both, but if I got to choose, I'd
> require teardown.
>
> J.
>
>
>
>
> On Fri, Jun 30, 2023 at 10:28 PM Daniel Standish
> <da...@astronomer.io.invalid> wrote:
>
> > Ok want to pick back up setup / teardown discussion as we get closer to
> > 2.7.
> >
> > For personal reasons I had to take some time off work just as we were
> > wrapping up work on 2.6, and at least partly due to that, we punted
> setup /
> > teardown to 2.7.
> >
> > But we've picked it back up and continued along the path outlined in this
> > thread and things look good for 2.7.
> >
> > In the time since the previous discussion, not a lot has changed except
> > that we've implemented in main some of the things discussed.
> >
> > For one we added a one-liner syntax.
> >
> > So now if you have an existing dag such as this:
> >
> > create_cluster >> run_query >> delete_cluster
> >
> > You can convert it to use setup / teardown with the following:
> >
> > create_cluster >> run_query >>
> > delete_cluster.as_teardown(setups=create_cluster)
> >
> > The context manager suggested by Ash has been added, so if you have setup
> > `s` and teardown `t`, you can do the following:
> >
> > with s >> t:
> >     some_work(other_work())
> >
> > Notably, just as with task group context manager, you must instantiate
> the
> > task inside the context in order for it to be registered.  Ephraim is
> > working on some improvements to the context manager to address this (so
> > that you can explicitly add tasks to the context in that scenario) and so
> > that we can throw an error on non-sensical usages such as `with a >> b >>
> > c:`
> >
> > There is a docs PR outlining the current state of the feature here
> > <https://github.com/apache/airflow/pull/32169> and you are all welcome
> to
> > comment.  There's still time to make adjustments if desired.
> >
> > But I wanted to raise one point for discussion here.  It's something I
> > could have gone either way on, and I want to tap into the wisdom of the
> > community.
> >
> > The question of whether a setup with no teardown, or a teardown with no
> > setup, should be added.
> >
> > There are two options as I see it.
> >
> > Option 1: if a setup has no teardown, then all the downstreams are
> assumed
> > to be "in scope" for the setup
> > Note: the notion of "scope" is about the clearing behavior aspect of
> setup
> > / teardown: you clear a task, it clears its setups too.  A work task is
> in
> > the scope of s and t if it's between s and t.
> > So then with s1 >> w1 >> w2, clearing work task w1 would clear w1 w2 and
> > s1.
> > And clearing w2 would clear w2 and s1.
> > This makes it easy to have a setup with no teardown. But it makes the
> > behavior slightly more... wiggly... for lack of a better term.  With this
> > example, s1 is a setup for w2.  But then suppose you add a teardown after
> > w1: s1 >> w1 >> [w2, t1.as_teardown(setups=s1)].  Now s1 is not a setup
> for
> > w2.
> > So the logic is ... "a work task is in scope of a setup if it is in
> between
> > the setup and its teardown, *or *if the setup has no teardowns".
> >
> > Option 1: we require that a setup always have a teardown
> > With this approach, the rule is simpler and it forces unambiguously
> defined
> > scopes.  If you just want to use a setup and don't need a teardown task,
> > you add an empty teardown.  The rule is then "a work task is in scope of
> a
> > setup if it is in between the setup and its teardown" -- i.e. the same
> rule
> > minus the `or` clause.
> > So the example would have to be s1 >> w1 >> w2 >>
> > EmptyOperator().as_teardown(setups=s1).
> >
> > Thanks in advance for your engagement.
> >
> > P.S. note that those of us in the U.S. will be off for holiday soon so
> may
> > be slower to respond but just wanted to get this conversation started so
> > that we can get this feature to a place that people generally feel good
> > enough about in 2.7.
> >
> > P.P.S. Random-ish note... we should consider marking some features as
> beta
> > I think.  There's a difference between beta and experimental, though
> > historically we've only employed the latter.  But I think beta can be
> good
> > to signal "we are committed to this feature we just might make a change
> or
> > two to the API".  Not saying we should do this with setup / teardown
> > necessarily but it's an option.  Sometimes I think it can be a little
> > overwhelming to put out features with the spectre of being locked in by
> > backcompat guarantees and perhaps something like a beta feature could be
> a
> > way to get things out there a little bit more freely and allow for a
> > feedback loop.
> >
> > Thanks again,
> >
> > Daniel
> >
>