No spawn on manual remove of parentless RH and/or SX tasks

[dwsutherland]

supersedes #7134

This change makes the default spawning behavior on manual (command) remove of parentless runahead (RH) and sequential xtriggered (SX) tasks.. They no longer spawn their next instance.

Users can always use cylc set --pre=all . . . to reintroduce the tasks.
And of course, not remove but, set (xtrig or outputs) or skip to spawn forward otherwise

This removes the need to reinstate the --no-spawn option in #7134

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests are included (or explain why tests are not needed).
• Changelog entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

Hillary's parentless spawning changes are now merged to master so we should be able get this hashed out quick, will need a rebase.

I think the main things to check here are going to be implications on interactions. Off the top of my head:

• Presumably, removing one instance of a parentless task shouldn't cancel all future spawning of the task (perhaps because of the runahead instance?, does this with/without sequential xtriggers?)?
• But that future spawning shouldn't happen instantly any more so cylc remove */task should remove all instances of task no if's no but's?
• Should run through the remove use cases to make sure we haven't broken any: https://github.com/cylc/cylc-admin/blob/master/docs/proposal-remove.md#removing-tasks-in-different-n-window-cases
• And obvs the R1/trigger use case.
• Any others?

[dwsutherland]

Hillary's parentless spawning changes are now merged to master so we should be able get this hashed out quick, will need a rebase.

Done.

I think the main things to check here are going to be implications on interactions. Off the top of my head:

The actual spawning mechanics haven't changed, only that a manual removal bypasses the self.spawn_next_parentless(itask) call in pool.remove (which was only ever run for RH or SX types) by using a spawn flag.

I've included a variety of scenarios with the test_sequential_xtriggers.py::test_remove test to cover what's effected by this change (including some of what you've mentioned).

[hjoliver]

@oliver-sanders - this new PR gets rid of the original PR's --no-spawn option by simply making "no spawn" the default for manual remove.

Looking back, David might have done this in response to your final comment on the other PR, which could be interpreted as saying "yes, remove is the natural solution to this problem, but it should work without needing a command option". However, on reflection, we doubt that you meant to just make "no spawn" the default for manual remove.

[OS] Presumably, removing one instance of a parentless task shouldn't cancel all future spawning of the task ...

This comment also suggests you hadn't read the PR description (or it wasn't clear to you) because on this branch removing one unspawned instance obviously will "cancel all future spawning of the task".

So I'm guessing you're not going to like this PR. In fact, you'll probably like it even less than the previous one, because with no command option needed this one just makes it easier to do what the other one does but is otherwise the same.

Can you confirm that is the case (or not) so we can get back to hashing out a viable solution?

[hjoliver]

To move this forward, this is my opinion:

Generally, if we need to NOT RUN a particular parentless sub-graph in some upcoming cycles, then we need both gap-filling (skip or set outputs on ALL the tasks) and graph-cutting ("ignore" the sub-graph) tools in the box, because which is best depends and is very much arguable. E.g. gap-filling is no good if you need to skip a large number of cycles in a large graph.

Less generally, given the current implementation of parentless task spawning in Cylc, the ability to remove parentless tasks without spawning seems to be the easiest way to address the real problems that have repeatedly afflicted ESNZ ops (and I ran into a nasty case myself that corroborates that). So I think we should do that, and then continue to develop other potentially better and potentially more general solutions in the background.

My main reservation about "remove without spawning" is that it depends entirely on the current SOS-bodged parentless spawning mechanism. When and/or if we do parentless spawning properly (SOD) then "remove without spawning" will cease to solve the aforementioned real problems! However, doing this properly has been and may still be a long time coming, for good reasons. So we should not avoid providing a real solution now on that basis. (And in the future, whenever that comes, we can solve the same problem another way).

[dwsutherland]

Context from Element (admin channel):
Oliver:

[dwsutherland]

The only contention (from a chat with Hilary) is whether the default no-spawn-manual-removal is acceptable on the UKMO end?

It introduces a danger that if RH/PSX tasks are removed then no future instances will be spawned.. However, they can always be added back in (set --pre=all)..

IMHO - The no-spawn-manual-removal default is actually the more natural behavior and more in line with SoD:

• For PSX tasks the satisfaction of sequential xtrigger is the prerequisite for the next instance to spawn, and so if the prerequisite is not satisfied at the time of removal then it shouldn't spawn.
• At present, for parentless RH tasks, the prerequisite for the next instance to spawn is the RH => active change, and so if framed in this way the same logic as the previous point applies (it shouldn't spawn).

Thoughts?

[oliver-sanders]

This comment also suggests you hadn't read the PR description (or it wasn't clear to you) because on this branch removing one unspawned instance obviously will "cancel all future spawning of the task".

I was somewhat aware of this potential, so posed this question above:

Presumably, removing one instance of a parentless task shouldn't cancel all future spawning of the task (perhaps because of the runahead instance?, does this with/without sequential xtriggers?)?
-- #7340 (comment)

Which got a like, so I guessed that you'd found some clever way of both having the cake and eating it.

So I'm guessing you're not going to like this PR

Sadly I think you're probably right.

• @hjoliver, from #7413 it seems you're in that boat too.
• @dwsutherland, you've been in this boat in the past (but are welcome to change position).

Unfortunately, if removing one instance of a task removes all future instances, then parentless tasks would behave rather inconsistently with parented ones.

I haven't got deep into thinking through use cases, but I suspect this gets reallyyyyyyy messy when it's combined with group logic, e.g, I removed a family in an old cycle, anddddd 90% of the tasks disappeared, the workflow stalled and I have no idea what I've done or how to fix it.

It's a tricky problem, I'm not sure it's really solvable under the Cylc model, the best we can do is to remove the need for such advanced interventions in the first place and de-escalate the complexity.

IMHO - The no-spawn-manual-removal default is actually the more natural behavior and more in line with SoD:

From the perspective of your use case, I can see how this might be preferable. But I'm not convinced that this is more "natural" in the general case, it does present inconsistencies, it makes cylc remove much more dangerous.

---

Aside: (agree or disagree, but lets not haggle this here)

Now, this part might indeed be a contentious point, but we don't actually do SoD in Cylc! Pure SoD turned out not to be a particularly suitable model for executing graphs written in a scheduling paradigm. So, we've spent the past few years slowly evolving the event-based SoD model into something which better represents Cylc's schedule-based paradigm (e.g, required outputs, completion expressions, parentless spawning, retained n=0 tasks, workflow stall, etc). This is the hybrid model by which Cylc 8 works, it's related to / a modification of SoD, but it's not SoD.

My reason for making this potentially contentious point is that "what's logical by SoD" is not necessarily "logical" by Cylc or its users. This is why I keep dragging things back to user expectations and use cases. I think if we get our heads too far inside the SoD model, we will draw rational logic to irrational conclusions.

Bring this back to the point, we're in a situation where cylc remove <task> can either:

• Remove an instance of a task.
• Remove all instances of a task.
• Shutdown the workflow (as completed).

And the only way the user can tell which outcome they're going to get is to review the n=0 window and the workflow's static graph (checking for sequential=True in xtrigger defs, but also remembering that wallclock sets this implicitly).

None of that's "natural"!

[hjoliver]

Which got a like, so I guessed that you'd found some clever way of both having the cake and eating it.

My "like" was on the basis of this comment, which sounded like you'd changed your stance a bit and agreed with this approach (presumably - I imagined - as a result of some prior discussion with David):
:

Hillary's parentless spawning changes are now merged to master so we should be able get this hashed out quick, will need a rebase.

Your first two questions after that were a bit confusing, but I was saving that for when I came back for a proper review.

[DS] Context from Element (admin channel):

I think that discussion was mainly about the flag in the code, in the original PR, that made it look as if removal might be delayed and hence potentially non-deterministic. I didn't think that flag should be necessary, and indeed David managed to remove it and do the removal at once. [Update: that was my impression from a chat on Element in mid June, but maybe the flag was not removed on the original PR after all.]

[hjoliver]

@hjoliver, from #7413 it seems you're in that boat too.

Nice try 😁 - it's different however:

• that PR is about removing a failed task, which (given the current parentless spawning mechanism) should have spawned the next instance already
  • there is no circumstance in which we should expect removing a failed task to cut off future instances (regardless of spawning mechanism)
• this one is about removing a waiting (runahead) task, which (given the current parentless spawning mechanism) has not spawned the next instance yet
  • so (given said mechanism) we should expect that removing it would (or at least could) cut off future instances
  • (unless, that is, we deliberately spawn it on removal - which we do)

[hjoliver]

Aside: (agree or disagree, but lets not haggle this here)

I'm not sure it's fair to make a contentious point and then expect others not to give their perspective! But anyhow I don't find that particularly contentious at all. It's just semantics. We define what "spawn on demand" means. It's not really a widely applicable concept like "event-driven" (although the two are related). Compared to Cylc 7, tasks really are now spawned "on demand", but exactly what "demands" their spawning can be quite subtle, and that's OK. And for this discussion, parentless tasks notably stand out as NOT being spawned on demand in any sense of the term.

"what's logical by SoD" is not necessarily "logical" by Cylc or its users. This is why I keep dragging things back to user expectations and use cases. I think if we get our heads too far inside the SoD model, we will draw rational logic to irrational conclusions.

I don't necessarily disagree with that statement, but I don't think I am doing that. To me the "SoD model" is just a convenient conceptual label for the way we spawn the workflow forward now, in comparison with how we used to do it, not some kind of implementation-based religion. Conceptually and rather loosely it just means we try (imperfectly, to date) to bring future tasks into existence only when (or minimally before) they are needed. And regardless of the model, the way we currently do parentless task spawning is notably sub-optimal: there's an implicit dependence on previous-instance runahead-release, which is wrong, and which has been a generator of bugs whenever we do something that potentially skips over that point.

[Looking back, I think your comment was mainly a response to David's on "...more in line with SoD" - which I don't agree with unless he was sort of alluding to the fact that parentless tasks should not be spawned in this way at all, in principle].

[hjoliver]

Bring this back to the point, we're in a situation where cylc remove can either:

• Remove an instance of a task. [A]
• Remove all instances of a task. [B]
• Shutdown the workflow (as completed). [C]

And the only way the user can tell which outcome they're going to get is to review the n=0 window and the workflow's static graph (checking for sequential=True in xtrigger defs, but also remembering that wallclock sets this implicitly).

None of that's "natural"!

I entirely agree on this @oliver-sanders ! In principle remove should ONLY remove the targeted instance(s), i.e. [A]. And I've noted that IF we spawned parentless tasks in a more sensible way then [B] and [C] would go away and "remove without spawning" would no longer help with our problem use cases.

Nevertheless, we have real problems afflicting a core site, with a demonstrated easy fix that works for the current implementation. And other potential approaches we've discussed are more difficult or require more thought and development before they provide a usable solution.

Unless we can do something better very quickly, I propose we go back to the explicit remove --no-spawn option:

• document that this is a low-level intervention that should not be used lightly
• this is not dangerous because it is not the default and so it ~can't be done by accident (don't deliberately add an arcane-sounding --no-spawn option if you don't know what it does!)

This provides an immediate solution to real problems we have. Yes it will be a temporary solution if we move to a better parentless spawning mechanism in the future, and yes we need good reasons to make temporary command API changes - but we do have good reasons, and we don't currently have other easy solutions.

[hjoliver]

It's a tricky problem, I'm not sure it's really solvable under the Cylc model, the best we can do is to remove the need for such advanced interventions

Sure, great to aim to remove the need for advanced interventions, but it is by no means clear that that is fully achievable (I strongly doubt it) and we're certainly not there yet. In the meantime we need interventions that work for real problems, and it's OK if some of those interventions require more expertise than some users have (show me a software system that has to handle anything half as complicated as our workflows, where that is not the case!).

[dwsutherland]

• @dwsutherland, you've been in this boat in the past (but are welcome to change position).

Having a look at that:

Restart/reload and remove are now handled..

Remove will cause the next occurrence to spawn, the argument being:

• the non-sequential case would be spawned, so be consistent.

I guess I was trying not to make too big of a distinction between parentless tasks, that are just spawned in all the way to the RH limit, and PSX tasks... So I was trying to make it consistent with parentless tasks (not SoD), even though (in a way), PSX tasks do have parents/prerequisites (xtriggers).

• otherwise the workflow would shutdown if it's the only task (and we have a stop button for that).

At this point in time the workflow wouldn't give you time to reintroduce tasks, it use to just shutdown immediately and not give you a chance (even on restart).. So we were in a bit of a bind.

IMHO - The no-spawn-manual-removal default is actually the more natural behavior and more in line with SoD:

From the perspective of your use case, I can see how this might be preferable. But I'm not convinced that this is more "natural" in the general case, it does present inconsistencies, it makes cylc remove much more dangerous.

Unfortunately, if removing one instance of a task removes all future instances, then parentless tasks would behave rather inconsistently with parented ones.

Bring this back to the point, we're in a situation where cylc remove <task> can either:

• Remove an instance of a task.
• Remove all instances of a task.
• Shutdown the workflow (as completed).

And the only way the user can tell which outcome they're going to get is to review the n=0 window and the workflow's static graph (checking for sequential=True in xtrigger defs, but also remembering that wallclock sets this implicitly).

Well, as Hilary mentioned:

Conceptually and rather loosely it just means we try (imperfectly, to date) to bring future tasks into existence only when (or minimally before) they are needed.

So by SoD "natural" I mean the satisfaction of the xtrigger is needed to spawn the next PSX task... But really the "unnatural" thing is something Hilary mentioned:

My main reservation about "remove without spawning" is that it depends entirely on the current SOS-bodged parentless spawning mechanism. When and/or if we do parentless spawning properly (SOD) then "remove without spawning" will cease to solve the aforementioned real problems!

The fact that the xtrigger is also removed when associated PSX task(s) is/are removed, instead hanging around to spawn the next xtrigger and downstream task.. i.e. if the xtrigger was represented by a TaskProxy like object it would be clearer to the user that removing the sequential xtrigger would remove the spawning e.g:

1/xtrig => 1/foo
  ||
  V
2/xtrig => . . .

so removing 1/xtrig would mean 2/xtrig doesn't spawn...
Whereas with this PR removing 1/foo removes the task from the pool, but also under the hood removes 1/xtrig and any future ?/xtrig without any visual indication that it does.

Or in the same way, perhaps, the RH limit should also be represented as a Task like object and not bound to the tasks that have reached this limit.. ????

If there was a clear visual representation, then removing the future spawning or "creating a gap" would be more acceptable... Because, yes, you've essentially got the same thing (you've "kicked the can down the road"), xtrigger like objects are your n=0 and you don't have the downstream PSX tasks anymore, however you are now acting on some thing that says "I'm a sequential Xtrigger that spawns downstream including the next xtrigger" instead of it happening under the hood...

So "naturally" under SoD if you remove the thing that spawns the next thing, the next thing isn't spawned (the conditions haven't-arisen/can't-arise to satisfy spawning).

So two options:

• I can resurrect the original PR reinstating the --no-spawn flag.
• I could modify this PR to only work on non-RH PSX tasks

Both would solve our issue until a better solution more inline with our long term ideals renders it redundant.. At which point it can be easily undone.

Of course a third, and I would hope, unthinkable option:

• Ignore the practical needs of ESNZ to keep the project pure of contaminants 😆

As mentioned we've discussed two long-term ways forward (re-outputs don't spawn, and isolated graph sections) that would reduce the need for this (and at which point maybe it can be removed)... The only scenarios outside of ICP spawned would be:

• The ability to create gaps and reintroduce some task here and there (maybe someone would like to work this way).
• Undo human error (oops I spawned a PT5M PSX task in 2 years in the past where it hadn't run before)

[oliver-sanders]

Ignore the practical needs of ESNZ to keep the project pure of contaminants 😆

I am working with you on this, honest.

Also keep in mind that the severe orphan task bugs (#7209 - on hold since Feb) cannot be solved until we have a way of removing all instances of a PSX task definition from the workflow. This is an 8.7.0 release blocker, that has hit us in production, so a solution here is a high priority for that reason too.

As mentioned we've discussed two long-term ways forward (re-outputs don't spawn, and isolated graph sections) that would reduce the need for this (and at which point maybe it can be removed)... The only scenarios outside of ICP spawned would be:

Reminder, while isolated graphs would help your ICP/re-trigger use case, they do not solve the problem of historical instances of newly added tasks entering the workflow as this can happen at any cycle (not just the ICP) and doesn't require the involvement of PSX tasks.

Also possible approaches to avoid the spawning of historical instances of newly added tasks have been discussed which could remove the need for the remove.

I can resurrect the original #7134 reinstating the --no-spawn flag.

This is what I was expecting?!

Ideally we wouldn't need this CLI flag long-term, however, we will still need this as an internal functionality (for as long as PSX spawning is a thing) for removing orphan tasks.

Reminder, the blocker here is the use of flags not backed-up by perpetual data storage. I think Hillary's PSX spawning changes mean that we should now be able to perform the removal synchronously now (because the spawning happens in subsequent main-loop iterations), avoiding the asynchronous removal problem which created the need for the TaskProxy flag.

[hjoliver]

@dwsutherland - OK here's where we are at, I think. Can you please:

1. Reinstate the --no-spawn command option
2. Ensure no internal itask.no_spawn flag is used, so there is not delayed removal going on
3. Document (CLI help) that the option is a low-level intervention that may cut off future parentless graphs

Plus:

4. (We acknowledge the various caveats raised about this method of solving these problems in general, but nevertheless it is the easiest solution in the short term, for the current system)
5. (Further, "remove no-spawn" will cease to be meaningful at all in the future when/if we eventually remove the implicit previous-instance dependence of parentless tasks - so we need to work on more general solutions before then).

Then (with 1-3) we should be good to go on this 🎉

[dwsutherland]

Superseding PR up: #7419

[oliver-sanders]

Superseded by #7419