Chiming in on a few of the topics discussed so far: - Context managers: I found most of the context manager syntax proposals a little hard to understand, but some better than others. Ultimately if I put my DAG author hat on, I find this declaration the most straightforward, clear and it's easy to update existing code: chain(create_notification_channel.as_setup(), ... other tasks ... delete_notification_channel.teardown_for(create_notification_channel),...)
- Short Circuit Behaviour Again putting my DAG author hat on, the example Daniel gave about skipping teardown steps in the event of a critical failure (so that we can debug files, clusters, etc left behind) is exactly something my team is interested in, and we're trying to build these things ourselves. Having the ShortCircuitOperator support this (although perhaps not by default) would be fantastic and as a user I would like to see this functionality maintained. - Multiple setup/teardown I also agree that whether we ship them now or later, we must ensure we don't walk through any one-way doors for future implementation. It's great that we're having these discussions early. Cheers, Niko ________________________________ From: Daniel Standish <daniel.stand...@astronomer.io.INVALID> Sent: Monday, March 27, 2023 9:43:07 AM To: dev@airflow.apache.org Subject: RE: [EXTERNAL][DISCUSS] AIP-52 updates - setup / teardown tasks CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > 1) I am not sure if we should make it private (I am not even sure what > it would mean to be private:) ). But If it means that setting the rule > type for non-teardown task should raise an error (and of course > documenting this rule as only (and automatically) being applied to > teardown task - I am good with it. Yeah I have no concerns with throwing error if this rule is used "manually" by user. Like, at init time though, keeping it simple. I.e. I think we shouldn't boil the ocean to prevent a creative user from finding a way to use it in some weird way. 2) Yes. The teardown must run in any case. I think documenting the > behaviour in ShortCircuit as a special case and - especially - adding > some examples showing that would be enough Clarify Jarek, are you saying that we should allow ShortCircuit to "short circuit" the teardowns [either by default or perhaps optionally with a skip_teardowns_too param] and document with examples? Or are you saying shortcircuit operator should be updated so it's not possible to use it to skip a teardown? On Mon, Mar 27, 2023 at 12:30 AM Jarek Potiuk <ja...@potiuk.com> wrote: > My view: > > 1) I am not sure if we should make it private (I am not even sure what > it would mean to be private:) ). But If it means that setting the rule > type for non-teardown task should raise an error (and of course > documenting this rule as only (and automatically) being applied to > teardown task - I am good with it. > > 2) Yes. The teardown must run in any case. I think documenting the > behaviour in ShortCircuit as a special case and - especially - adding > some examples showing that would be enough > > And yeah while it is indeed an implementation detail, it is somewhat > "visible" as the public API is being concerned. So no wonder it came > as a bit of a surprise while implementing setup/teardown (happens that > something we consider as a detail for those deeply involved is a bit > of a surprise for those that came to look at it at review time). I > guess this is somewhat a consequence of the way we operate in a > distributed environment and some details are being discussed in > smaller groups that are more focused on getting things done (we had a > few of those for AIP-44 for one). > > But eventually we are discussing it now, so I think it is cool. > > J. > > On Mon, Mar 27, 2023 at 8:43 AM Ash Berlin-Taylor <a...@apache.org> wrote: > > > > If the set-up ran then the tear down _must_ run. No question. > > > > Nothing should be able to change this fact. If you can, then they don't > fulfill the stated purpose of tear down tasks in the AIP: to tidy up > resources created by a set up task. > > > > On 27 March 2023 06:22:47 BST, Daniel Standish > <daniel.stand...@astronomer.io.INVALID> wrote: > > >> > > >> When user set setup/teardown he has no idea unique trigger rule is set > > >> under the hood. The user also has no idea that trigger rules are even > > >> involved. That is not something he sees unless he checks the code of > > >> teardown and setup decorators. > > > > > >This means that users of ShortCircuitOperator will not even know they > need > > >> to take action (until it wont work as expexted) and they will > propbably > > >> start as asking questions. > > > > > > > > >Yeah, short circuit operator is a special operator that, if you're > going to > > >use it, you ought to know how it works. And we can easily add a note in > > >the docs that emphasizes its behavior on this point. But I should point > > >out, the same would be true if setup and teardown were not implemented > with > > >trigger rules; unless you investigate, read the docs, or look at the > code, > > >you would not know whether teardowns would be skipped by short > circuit. So > > >either way it's something that we'd have to lean on docs for. It's just > > >not something that a prudent user would just make an assumption about. > > > > > >If the set-up ran then the short circuit shouldn't be able to skip it > > >> > > > > > >I think this is overly and unnecessarily opinionated and limiting. I > would > > >not be concerned with having the default be one way or another, but to > say > > >that "you should not be able to skip it", I disagree with the notion > that > > >skipping a teardown with an operator specifically designed for the > purpose > > >should be *forbidden,* as your comment suggests. > > > > > >take for example creating a cluster - you still want to delete it at the > > >> end even if you skipped all the other tasks! > > >> > > > > > >Yeah that's a perfectly fine example. And, normally this is precisely > how > > >this feature works: if your task throws a skip exception, its downstream > > >work tasks will be skipped but the teardown will run. But what we're > > >talking about is a special operator whose unique purpose is to > > >short circuit the dag. And as we know, every kind of dag you can > imagine, > > >there's a user who needs it. So someone will want to be able to skip a > > >teardown. Teardowns aren't always going to be deleting a cluster. They > > >might be deleting *files*, let's say. Well, what if you have this > special > > >operator in there to detect when something very bad happens and in that > > >case you want to stop all processing and leave the current state alone > so > > >you can debug it. Is there any good reason to forbid this? I think no. > > >As stewards of this pipeline writing language, we should have sensible > > >defaults and maintain a good authoring interface, while allow users the > > >power to write what they need. > > > > > > > > > > > > > > >On Sun, Mar 26, 2023 at 9:19 PM Ash Berlin-Taylor <a...@apache.org> > wrote: > > > > > >> If the set-up ran then the short circuit shouldn't be able to skip it: > > >> take for example creating a cluster - you still want to delete it at > the > > >> end even if you skipped all the other tasks! > > >> > > >> This is precisely what I mean by set up and tear down tasks being > special! > > >> > > >> On 27 March 2023 04:02:32 BST, Elad Kalif <elad...@apache.org> wrote: > > >> >Thanks Daniel, > > >> >Let me clarify my concern. > > >> > > > >> >When user set setup/teardown he has no idea unique trigger rule is > set > > >> >under the hood. The user also has no idea that trigger rules are even > > >> >involved. That is not something he sees unless he checks the code of > > >> >teardown and setup decorators. > > >> > > > >> >This means that users of ShortCircuitOperator will not even know > they need > > >> >to take action (until it wont work as expexted) and they will > propbably > > >> >start as asking questions. > > >> > > > >> >I'm not saying this should modify the plan just raising it as a > potential > > >> >source for pitfall. > > >> > > > >> >בתאריך יום ב׳, 27 במרץ 2023, 05:50, מאת Daniel Standish > > >> ><daniel.stand...@astronomer.io.invalid>: > > >> > > > >> >> Thanks Elad for the feedback. > > >> >> > > >> >> re 1. i don't really see a problem with the trigger rule being > public. > > >> The > > >> >> way I see it, it's another trigger rule like any other trigger > rule. > > >> Every > > >> >> trigger rule behaves differently, that's true here too. This one > > >> happens to > > >> >> be relied upon for teardown tasks. That said, I don't think I > would > > >> >> necessarily be opposed to making it private. > > >> >> > > >> >> re 2, personally I kindof think it's a good thing. My > understanding > > >> from > > >> >> your comments is that with ShortCircuitOperator you can set it to > skip > > >> all > > >> >> downstream or just skip the direct relatives. To me this seems > great > > >> cus > > >> >> it provides a way to either skip everything (including teardowns) > or > > >> just > > >> >> the next task (thus potentially allowing teardowns to run). To me > this > > >> is > > >> >> another way in which by staying within the existing dependency and > > >> trigger > > >> >> rule paradigm we have more consistent, predictable, and > configurable > > >> >> behavior. E.g. if we were not using normal deps and trigger > rules, then > > >> >> surely someone would have the opposite concern: "i want to use > short > > >> >> circuit operator to just skip all tasks including teardowns" and we > > >> might > > >> >> not be able to grant that wish, or at least not without more > > >> development. > > >> >> When you use an operator like this, you simply need to know what > it does > > >> >> and configure it in a manner appropriate for your use case. > > >> >> > > >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > >
