Hello everyone,
TL;DR; I also read Constance's point and I wholeheartedly agree with her.
And after thinking about it my answer is slightly changed: *"we should keep
the option, but having it per-pool is wrong, it should be defined at
operator level".*
> I think the confusion comes from who’s responsible for what
I think that's an excellent point. I have not thought about it before, but
this is a key to answer the question of what we do with "pools
and deferrables". We often forget that we have different "actors" in
Airflow - and not necessarily one actor has the same knowledge and need as
other actors.
In case of Deferrable operators (which currently only really works for
"classic"/ "non-taskflow" case) we have essentially two actors:
* Deployment Manager who defines the pool
* Operator's Author who implements "Operator"
* DAG Author who uses the operator in their DAG
I think it's reasonable to assume that:
* Deployment Manager has no idea how the pool will be used. Setting the
"pool" to include_deferreed means nothing to the Deployment Manager.
* Operator's Author who implements the operator should be able to
understand whether the "deferrable" and "running" part of the task should
both take the pool slot when pool is assigned or not - and they should be
able to describe the expectations ("This operator should have a pool
assigned which should correspond to a number of GPUS" for example -
following my previous case). Also it's quite possible that other operators
using the same pool will have different scenarios of use - and maybe they
will need the same pool also in a deferred state.
* DAG Author who "uses" the operator in DAG| does not have to know all the
details on when the pool slot should be allocated, they should just know
that if they want to start the task, they should have. a "pool" allocated
or not. Whether the pool is taken while being deferred is something they
should not really think about.
Looking at the above reasoning I think our implementation of
"include_deferred" is wrong. It should be defined in the "operator", not in
the pool IMHO (at least to serve the use case I described above).
J.
On Sun, Oct 20, 2024 at 2:58 PM Jarek Potiuk <ja...@potiuk.com> wrote:
> TL;DR; I think we should keep what we have.
>
> (Daniel and just to clarify - I am not trying to derail the discussion on
> what we should do with pool + deferrable - just trying to understand what
> use cases it serves now and maybe what use cases it should serve in the
> future world where ML/AI and particularly GPU tasks will become more
> important).
>
> I think (but I might be wrong) that the assumption you made "well the
> point of deferrable task is, the task spends most of it's lifecycle in
> deferred state." is not always true. What you describe is mostly what
> "sensors" used to do, but deferrable tasks are way more flexbile, and allow
> to combine "waiting" and "doing" in a single task in many more ways. I
> think there are different types of deferrable tasks - and there are some
> tasks that have both - time while being deferred and time while executing
> "work" equally important. And their interaction with pools can be a little
> more nuanced.
>
> As I understand - pools server several purposes:
>
> 1) they prevent external systems from being overloaded with a number of
> parallel, distributed tasks accessing such resources - and then quickly do
> what you explained (i.e. most of the time they spend in deferred state).
>
> I think in this case, the "desired" pool behaviour in this case depends on
> the way the task is deferred.
>
> a) There is a class of deferrable tasks that access the same external
> resource while running and being deferred. There are some external
> resources that need to be polled continuously while deferring - on top of
> actively using those resources while running. In this case - one can argue
> that pools should apply to both states. It's not as easy though - because
> by definition, triggerer accesses only one such deferred task at a time -
> which means that in a number of cases even if there are multiple triggers
> for the same external resource - only one of them is used at a time if they
> are all run in the same triggerer. So "ideal" pool occupancy in this case
> is : *num running tasks + num triggerers we have where the tasks can get
> deferred to*. But such a pool occupancy might be difficult to
> model/explain/calculate. We currently don't do that. Instead we can do what
> b) case is most suitable for.
>
> b) The second case where deferred tasks are supposed to take the resources
> even if they are "sleeping" between the time the triggerer gives them
> time-slot to perform the check. In this case, each trigger occupies the
> resource anyway - regardless if it is actually "sleeping" or "running" in
> triggerer (say each trigger has an open HTTP connection to the same
> external server. In this case the pool occupancy is *num running tasks +
> num deferred tasks. *This is essentially what you propose as the only way
> we should leave for Airflow 3.
>
> 2) But there is another use case for pools - they prevent tasks from
> necessarily running when we know that we have a limited number of resources
> (Say GPUs). Pools can be effectively used for that kind of resource
> limiting. Those tasks might wait in deferred state for other conditions
> (say model availability) - but this "deferred" waiting might not need GPU
> at all. Imagine the task waits for a specific external AI model to be
> prepared - but after coming back from deferral, they will load the model
> to GPU and execute a substantial, and long running work then. They need GPU
> resource, and it makes no sense to start running such task after it leaves
> the deferred state - so it should be just scheduled to be executed, but it
> should only run when one of the GPUS become available (which can be
> modelled by Pool called "gpus" for example)
>
> In this case the typical "deferrable" task I can imagine is a bit
> different than what you observed - it will spend some of the time in a
> deferred state, but then also quite some time in a "running" state. And
> having it take "pool" while in deferred state will be blocking such GPU
> from being used by other tasks (remember that the deferred state does not
> need GPU at all !). In this case desired "pool occupancy" is : *num
> running tasks in non-deferred state. * I believe the case where we are
> only using pools for "non-deferred" state is very nicely fitting that kind
> of use case. And if we get rid of it, our users will lose that flexibility.
>
> Of course we can always tell the user "split your task into two", but I
> have a feeling that having the logic of "wait for model" and "execute a
> model" in a single task, makes perfect sense and it makes the DAGs simpler
> and more logical. But maybe it's just me :)
>
> So I'd say: answering directly your question - I think we should keep that
> flexibility of pools and deferrals we have now. I think removing it will
> remove an important use case.
>
> I do not think we should implement 1a) case, because it is too complex to
> explain and reason about. I think (and that's a little tangential - but
> related) we should implement more sophistication by external tooling - we
> do not have to implement in airlfow. Following the discussion about
> YuniKorn - I think we should not expand Airflow features but we should
> "stand on the shoulders of giants" - and involve external
> resources/task/quota scheduling tools that have much more sophisticated
> algorithms and scheduling scenarios. Combining "simpler" logic in pure
> Airflow (which works with Celery and Local Executor as well) with "more
> complex" scheduling logic via YuniKorn executor (which would only work with
> K8S) - that implements much more sophisticated and flexible ways to manage
> what our pool implementation does in a simplistic way, would be great
> eventually and provide even more flexibility to Airflow user, without
> complicating Airflow.
>
> J.
>
>
>
>
>
>
> On Sat, Oct 19, 2024 at 4:41 PM Daniel Standish
> <daniel.stand...@astronomer.io.invalid> wrote:
>
>> >
>> > I’m wondering why do we want to remove this. The design seems to be
>> > reasonable, but yep, it might not be as helpful as mentioned.
>> >
>> > > is it useful to have it take up a slot at the first and last couple
>> > seconds
>> > of its lifecycle? methinks no.
>> >
>> > Some edge cases “might” be helpful. I guess? A deferrable operator
>> that’s
>> > not (or could not) implemented well probably need it. My main question
>> is
>> > whether this configuration is causing confusion or complicating logic in
>> > the code base? Otherwise, we probably could just keep it?
>> >
>>
>> Ok so why...
>>
>> Airflow is complicated and in some cases provides too many configuration
>> choices to user, or too-confusing configuration choices. Airflow 3 is the
>> time to clean these things up.
>>
>> If we only *add* configurations and features and params, we can see where
>> that goes. Just increasing complexity, which makes airflow more confusing
>> and frustrating to use, and harder to maintain.
>>
>> There is a strong impulse to "just make it configurable", when considering
>> one behavior or another. We need to fight this impulse.
>>
>> And to be fair sometimes it's done to introduce a behavior change or
>> improvement in a backward compatible way. And that may be in part what's
>> going on here. But now it's 3.0 and we can make the change.
>>
>> I think this option on pools is an instance where, we should just decide
>> that tasks count against the pool for the full "running phase" of the task
>> (incl on worker and triggerer and in between).
>>
>> It's not like this param, if left on pools, will eat your children. But I
>> am saying I don't really think it makes sense there. So I would be in
>> favor of simplifying it and removing it, and making a good choice for
>> the user, and reducing the complexity in airflow just a little bit.
>>
>
