Make `cylc remove` flow-aware and extend to historical tasks #6370

MetRonnie · 2024-09-11T15:38:26Z

Note

Superseded by #6472

Partially addresses #5643

Summary

This mostly implements the "Cylc Remove Extension" proposal.

Flow numbers

cylc remove now has a --flow option for removing a task from specific flows.

If not used, it will remove the task from all flows that it belongs to.

If the removed task is active/waiting, if it is removed from a subset of flows that it belongs to, it will remain in the task pool; if it is removed from all flows that it belongs to, it will be removed from the task pool (as is the current behaviour).

If a task is removed from all flows that it belongs to, it will become a no-flow task (flow=None).

For ease of reviewing, you can use my UI branch that displays flow numbers: https://github.com/MetRonnie/cylc-ui/tree/flow-nums ¹.

Historical tasks

cylc remove now can remove tasks that are no longer active, making it look like they never ran. It does this by removing the task from the specified flows in the database (in the task_states and task_outputs tables)², and un-setting any prerequisites of active tasks that the removed task had naturally satisfied³. If a task is removed from all flows that it belongs to, a no-flow task is left in the DB for provenance.

The above also applies to active/waiting tasks that cylc remove is used on.

What's left to do

When removing an active task from all its flows, kill the task.
Should probably add a functional test with the --flow option.
Need to check this one:

If removing a task causes all of the prerequisites on a downstream task to be unset (i.e. if the downstream task was spawned as a result of outputs from this task alone) then the downstream task shall be removed from the pool.

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).
No dependency changes
Tests are included
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.

Waiting tasks that are not yet in the pool have greyed out flow numbers at the moment. ↩
If removing flows would result in two rows in the DB no longer being unique, the SQLite UPDATE OR REPLACE statement is used, so the first entry will be removed and the most recent entry will remain. ↩
Prerequisites manually satisfied by cylc set --pre are not affected by cylc remove. ↩

Wrong PR, sorry.

cylc/flow/scheduler.py

oliver-sanders

Code looks good!

tests/conftest.py

tests/integration/conftest.py

cylc/flow/workflow_db_mgr.py

cylc/flow/task_pool.py

cylc/flow/command_validation.py

tests/integration/tui/conftest.py

oliver-sanders · 2024-10-11T15:03:00Z

Have been trying this out for sub-graph re-run use cases. The remove functionality is all working correctly 👍, I am able to re-run sub-graphs cleanly without using new flows 🚀.

I have encountered some hitches (not related to this PR, all for consideration in follow-on work):

I've been hitting this issue a lot (pool: check prereqs on task spawn #6143) with any dependencies which span more than one "rank" in the graph. In these situations, the GUI and DB say one thing, but the task pool another which is highly confusing. Restarting (or possibly reloading?) the workflow should fix it, but we need a solution to this before remove really becomes a viable solution to re-running sub-graphs. Note this is a very similar limitation to the need to satisfy off-flow prereqs when re-running via new flows.
Removing a task doesn't necessarily change anything in the GUI (i.e. there is no visual feedback). This was anticipated when the proposal was written and is why visual filtering cylc-ui#470 is on the Cylc roadmap. I wonder whether a global change to the appearance of n=0 tasks (e.g. making them translucent as originally proposed) would provide an easier workaround and also help with other things?
Removed tasks retain their state in the GUI. E.G. if a task succeeded and was removed, it continues to be shown with a filled circle. It makes sense to preserve the job state, however, I wonder if we should reset the task state back to waiting when we remove the task to make it clearer?

oliver-sanders · 2024-10-17T12:38:34Z

What's the status on these TODO items from the OP:

What's left to do

When removing an active task from all its flows, kill the task.

Should probably add a functional test with the --flow option.

Need to check this one:

MetRonnie · 2024-10-17T13:25:09Z

Will either come in a follow-up PR or this one depending on how soon @hjoliver reviews this

…fied prereqs

…am tasks

- Update data store with changed prereqs - Don't un-queue downstream task if: - the task is already preparing - the task exists in flows other than that being removed - the task's prereqs are still satisfied overall - Remove the downstream task from the pool if it no longer has any satisfied prerequisite tasks

hjoliver · 2024-11-10T21:38:28Z

Getting in review mode here - sorry for the delay!

hjoliver · 2024-11-10T21:53:15Z

cylc/flow/prerequisite.py

-    def get_resolved_dependencies(self) -> List[str]:
+    def get_satisfied_dependencies(self) -> List[str]:


[Vaguely interesting historical aside: I think use of "resolved" here goes all the way back to Cylc 2, when we first got the ability to visualize a graph - not from the workflow definition because there was no workflow definition, just a bunch of task definition files with inputs and outputs - from dependencies "resolved" at runtime.]

cylc/flow/scripts/remove.py

hjoliver · 2024-11-11T01:53:04Z

cylc/flow/task_proxy.py

+        NOTE: If `flow_nums` is empty, it means 'all', whereas
+        if `self.flow_nums` is empty, it means this task is in the 'none' flow


OK nice way to handle that.

hjoliver · 2024-11-11T01:54:56Z

cylc/flow/task_state.py

+        """Return True if any of this task's prerequisite tasks are
+        satisfied."""


Suggested change

"""Return True if any of this task's prerequisite tasks are

satisfied."""

"""Return True if any of this task's prerequisites are satisfied."""

Doesn't fit

(my main point was the wording ... I'll transfer to the superseding PR)

hjoliver · 2024-11-11T03:02:18Z

cylc/flow/task_pool.py

-            # we can infer it was deliberately removed, so don't respawn it.
+            # we can infer it has just been deliberately removed (N.B. not
+            # by `cylc remove`), so don't immediately respawn it.
            # TODO (follow-up work):
            # - this logic fails if task removed after some outputs completed
-            # - this is does not conform to future "cylc remove" flow-erasure
-            #   behaviour which would result in respawning of the removed task
-            # See github.com/cylc/cylc-flow/pull/6186/#discussion_r1669727292
            LOG.debug(f"Not respawning {point}/{name} - task was removed")


I think I wrote this comment myself, some time ago. Presumably it does not refer to prevention of "conditional respawning" because those tasks would have been removed with completed outputs. The only other only non-cylc-remove case is to prevent respawning of a task removed by suicide trigger, I think. Can we adapt the new machinery on this branch to handle that properly too - i.e. record in the DB that the task was removed by suicide trigger, to avoid having to imperfectly infer it? Could be follow-up work of course, if I'm not barking up the wrong tree.

(I'll transfer this to the superseding PR...)

hjoliver

I've been through almost all of the non-test code, looks good. I still need to read the task_pool changes more carefully and do some testing.

hjoliver · 2024-11-11T06:41:17Z

@MetRonnie - is this still in draft because of the two "what's left to do" items at the top?

MetRonnie · 2024-11-11T10:17:40Z

Yes, because Oliver wanted to wait for the whole proposal to be addressed before merging. (FYI I'm going to close this PR and open a new one to stop GitHub collapsing recent review comments)

MetRonnie · 2024-11-11T11:59:10Z

Superseded by #6472

MetRonnie added this to the 8.4.0 milestone Sep 11, 2024

MetRonnie self-assigned this Sep 11, 2024

MetRonnie marked this pull request as draft September 11, 2024 15:42

MetRonnie force-pushed the cylc-remove branch from 3b5fab1 to ef5ea12 Compare September 11, 2024 15:42

MetRonnie changed the title ~~cylc remove: make flow aware and extend to historical tasks~~ Make cylc remove flow-aware and extend to historical tasks Sep 12, 2024

oliver-sanders previously approved these changes Sep 12, 2024

View reviewed changes

MetRonnie requested a review from oliver-sanders September 13, 2024 16:34

MetRonnie marked this pull request as ready for review September 13, 2024 16:43

oliver-sanders reviewed Sep 24, 2024

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Sep 24, 2024

View reviewed changes

MetRonnie marked this pull request as draft September 25, 2024 16:23

MetRonnie force-pushed the cylc-remove branch from ef5ea12 to 4f3aabe Compare September 27, 2024 16:15

MetRonnie marked this pull request as ready for review September 27, 2024 16:28

This comment was marked as resolved.

Sign in to view

oliver-sanders reviewed Sep 30, 2024

View reviewed changes

cylc/flow/command_validation.py Outdated Show resolved Hide resolved

tests/integration/tui/conftest.py Show resolved Hide resolved

MetRonnie marked this pull request as draft October 2, 2024 14:56

MetRonnie force-pushed the cylc-remove branch from 4f3aabe to b6b3cc0 Compare October 10, 2024 15:13

This comment was marked as outdated.

Sign in to view

MetRonnie marked this pull request as ready for review October 10, 2024 16:13

MetRonnie requested a review from hjoliver October 10, 2024 16:13

MetRonnie mentioned this pull request Oct 11, 2024

Add type annotations & fix possible type error #6405

Merged

5 tasks

MetRonnie force-pushed the cylc-remove branch from 5c362a0 to 40045e3 Compare October 15, 2024 11:19

MetRonnie requested a review from oliver-sanders October 15, 2024 14:00

MetRonnie force-pushed the cylc-remove branch 2 times, most recently from fb49bcf to 8f72cb4 Compare October 16, 2024 11:13

MetRonnie requested a review from wxtim October 17, 2024 15:35

MetRonnie added 7 commits November 1, 2024 18:30

Integration tests: shorter workflow IDs

572af1f

Changelog

3afe077

Ensure cylc remove unqueues tasks if it caused them to have unsatis…

8461e71

…fied prereqs

cylc remove: more efficiently unset prereq satisfaction of downstre…

841321e

…am tasks

Add test for suicide prereqs

f541091

Merge 'upstream/master' into cylc-remove

39be53f

MetRonnie force-pushed the cylc-remove branch from f6a7d29 to 39be53f Compare November 1, 2024 18:46

MetRonnie marked this pull request as ready for review November 1, 2024 18:48

MetRonnie marked this pull request as draft November 4, 2024 15:08

MetRonnie mentioned this pull request Nov 7, 2024

Functional tests: allow grep_fail to accept options #6463

Merged

6 tasks

hjoliver reviewed Nov 10, 2024

View reviewed changes