Allow cylc hold to hold future tasks that haven't spawned yet #4238

MetRonnie · 2021-06-02T16:11:32Z

These changes close #3743

You can now make Cylc hold future tasks when they spawn (without having to use the --after option).

Note this only works for "exact" task identifiers, e.g. cylc hold my_flow mytask.1234; using globs (e.g. 'mytask.*') or status qualifiers (e.g. mytask.1234:failed) will only hold active tasks, not unspawned ones.

Requirements 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.py and
conda-environment.yml.
Appropriate tests are included (unit and functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at Update docs for cylc hold cylc-doc#249.

cylc/flow/task_pool.py

cylc/flow/task_queues/independent.py

hjoliver

You missed one call to TaskPool.queue_tasks() that actually queues multiple tasks at once - at the end of config reload - hence the broken functional tests. It's probably fine to queue those tasks one at a time rather than revert your change.

hjoliver · 2021-06-03T03:58:13Z

Generally looks great and works well though 🎉

We probably need a way to show what future tasks are in the hold list. Maybe just add the info to cylc dump output. Or cylc hold --show (but that would make hold an instant-gratification query command as well as an asynchronous mutation...).

Not just active tasks

- Ensure globs & status can only be used for holding active tasks; to hold future tasks, must use explicit task id - Add unit/integration tests

MetRonnie · 2021-06-03T10:02:16Z

You missed one call to TaskPool.queue_tasks() that actually queues multiple tasks at once - at the end of config reload - hence the broken functional tests. It's probably fine to queue those tasks one at a time rather than revert your change.

Agh, wasn't picked up by mypy because the method calling it wasn't type annotated. Fixed and rebased

cylc/flow/task_pool.py

MetRonnie · 2021-06-03T10:18:15Z

cylc/flow/workflow_db_mgr.py

+    def put_tasks_to_hold(
+        self, tasks: Set[Tuple[str, 'PointBase']]
+    ) -> None:
+        """Replace the tasks in the tasks_to_hold table."""
+        # As we are replacing the maps each time this is called, there
+        # shouldn't be much cost to calling this multiple times between
+        # processing of the db queue
+        self.db_deletes_map[self.TABLE_TASKS_TO_HOLD] = [{}]
+        self.db_inserts_map[self.TABLE_TASKS_TO_HOLD] = [
+            {"name": name, "cycle": str(point)}
+            for name, point in tasks
+        ]


Hmm, not sure why I did it this way. There may have been a reason why I didn't only insert/delete the differing tasks, but I can't see it

I think this might be the approach used for other tables?

For the moment I've got my fingers in my ears going lalala, the DB usage needs some major rationalisation:

Repeated wiping and rebuilding of tables.

Repeated creation and destruction of the DB connection.

Un-normalised tables.

See #3872, #3666.

At the moment it's messy but working and we have bigger things to worry about so I think this work will slide back a while to come.

oliver-sanders

Looks great.

cylc/flow/task_pool.py

oliver-sanders · 2021-06-03T10:12:09Z

cylc/flow/task_pool.py

+            if point_str is None:
+                LOG.warning(f"No active instances of task: {item}")
+                n_warnings += 1
+                continue


This function is matching future tasks not active ones?

It's a bit confusing I know. This function relies on you having previously filtered out the matching active tasks using self.filter_task_proxies(). So if the point is not specified e.g. mytask, equivalent to mytask.*, and there is no active instance of mytask, then I say that is the correct warning to give.

oliver-sanders · 2021-06-03T10:21:07Z

cylc/flow/task_pool.py

+    def hold_active_task(self, itask: TaskProxy) -> None:
+        if itask.state.reset(is_held=True):
+            self.data_store_mgr.delta_task_held(itask)
+        self.tasks_to_hold.add((itask.tdef.name, itask.point))


Active tasks don't need to go into the tasks_to_hold set, if they were in the set before I think they should come out?

They don't need to, but I think it's easier to deal with if everything that is held or is going to be held is in the set. They should only need to come out when released.

cylc/flow/task_pool.py

hjoliver

Nice, this one is quite important, for users 👍

oliver-sanders · 2021-06-04T11:01:21Z

Testing is going well, I've hit one issue with the n=1 data store which I had a poke at as I wasn't sure how easy/possible it was to shim, turns out actually quite easy. Here's a PR, feel free to use it or re-work it MetRonnie#6.

Update task state in the n=1 window (data store delta)

oliver-sanders · 2021-06-04T12:14:57Z

I think some tests have been angered by the recent conftest.py change?

oliver-sanders

Code looks good, testing went fine, DB housekeeping ok.

Found the following quirks:

Completed tasks remain in tasks_to_hold.

E.G. If you hold a task then trigger it it will remain in tasks_to_hold even after it has passed out of the n=1 window. These stuck tasks can be removed by cylc release <flow> --all.

It's a quirk not a bug, I don't think it's a problem or worth the fuss handling differently.
Behaviour with reflow.

I think we will want to add the "flow label" into the tasks_to_hold set in order to handle the situation where we have the same task from different flows. E.G. the user might want to hold one flow but the another @hjoliver

Easy change to make, happy to do now or later.
Need to bodge TUI and GUI to handle future families

Follow-on not for this PR

Because globing/matching is not allowed in the n>0 window holding/releasing tasks in the n>0 window from Tui/Gui doesn't work. Easy fix, we should get Tui/Gui to expand the cycle/family to the full list of tasks in the selection.

Opened:
- tui: expand cycles/families in mutations to the visible selection #4246
- aoft: expand cycles/families in mutations to the visible selection cylc-ui#686

oliver-sanders · 2021-06-07T09:07:34Z

(will leave @hjoliver with the reflow comment above)

MetRonnie · 2021-06-07T10:14:21Z

I have also just noticed:

If you start a simple workflow (P1 = foo) paused, then hold all active tasks, then resume; foo.1 will apparently ignore the held state and will run

INFO - Processing 1 queued command(s)
INFO - Command succeeded: hold(['foo.*'])
INFO - Processing 1 queued command(s)
INFO - RESUMING the workflow now
INFO - Command succeeded: resume()
INFO - Queue released: foo.1
INFO - [foo.1] -submit-num=01, host=hostymchostface
INFO - [foo.1] -triggered off []
INFO - [foo.1] status=preparing (held): (internal)submitted at 2021-06-07  for job(01) flow(E)
INFO - [foo.1] -health check settings: submission timeout=None, polling intervals=PT15M,...
INFO - [foo.1] status=submitted (held): (received)started at 2021-06-07  for job(01) flow(E)
INFO - [foo.1] -health check settings: execution timeout=None, polling intervals=PT15M,...
INFO - [foo.1] status=running (held): (received)succeeded at 2021-06-07  for job(01) flow(E)

I think this can be fixed in a separate bugfix PR

MetRonnie · 2021-06-07T10:15:32Z

E.G. If you hold a task then trigger it it will remain in tasks_to_hold even after it has passed out of the n=1 window. These stuck tasks can be removed by cylc release <flow> --all.

Hmm, perhaps it should be that triggering a task also releases it?

oliver-sanders · 2021-06-07T10:18:58Z

I think we don't do that so that tasks can be triggered in a way that prevents retries from occurring?

There are other ways we could end up with held tasks drifting outside of the n=1 window (e.g. suicide triggers). I think the solution would be to remove the task from tasks_to_hold when the task is removed from the pool.

MetRonnie · 2021-06-07T11:48:59Z

Another issue, which I realised from Oliver's comment #4246 (comment):

If you have a family A that has at cycle point 1:
- a (n=0)
- b (n=1)
then doing cylc hold myflow A.1 will only hold a.1, and not set b.1 to be held. (If a.1 was n=1 instead, then holding A.1 would set both a.1 and b.1 to be held)

Update: we have decided to only expand families in the n=0 window. Resolved by 3b8cc40.

When holding e.g. FAM.2, only expand into tasks for active tasks, not future ones

MetRonnie · 2021-06-21T09:46:08Z

(Bash test failure is due to Docker container initialisation stalling, don't think it's worth re-running)

hjoliver · 2021-06-22T03:50:45Z

Behaviour with reflow.

I think we will want to add the "flow label" into the tasks_to_hold set in order to handle the situation where we have the same
task from different flows. E.G. the user might want to hold one flow but the another @hjoliver

Yeah, I'll post a follow-up issue .... #4277

hjoliver

Looks great, no problems found 🎉

MetRonnie added the sod-follow-up label Jun 2, 2021

MetRonnie added this to the cylc-8.0b2 milestone Jun 2, 2021

MetRonnie requested review from hjoliver and oliver-sanders June 2, 2021 16:11

MetRonnie self-assigned this Jun 2, 2021

hjoliver reviewed Jun 3, 2021

View reviewed changes

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

hjoliver reviewed Jun 3, 2021

View reviewed changes

cylc/flow/task_queues/independent.py Show resolved Hide resolved

hjoliver requested changes Jun 3, 2021

View reviewed changes

MetRonnie added 4 commits June 3, 2021 10:55

Allow future tasks to be held

6819d6b

Not just active tasks

Custom task matching logic for holding future tasks

fe2d54e

- Ensure globs & status can only be used for holding active tasks; to hold future tasks, must use explicit task id - Add unit/integration tests

Tidy log message

92cef14

Store set of tasks to hold in database

81d084d

MetRonnie force-pushed the hold branch from 208e1cb to edef696 Compare June 3, 2021 10:00

MetRonnie commented Jun 3, 2021

View reviewed changes

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

MetRonnie commented Jun 3, 2021

View reviewed changes

MetRonnie added 2 commits June 3, 2021 11:30

Update functional tests for cylc hold

cbf0f4b

Update changelog

e4d9d93

MetRonnie force-pushed the hold branch from edef696 to e4d9d93 Compare June 3, 2021 10:40

oliver-sanders reviewed Jun 3, 2021

View reviewed changes

Address code review

e2cc819

hjoliver approved these changes Jun 3, 2021

View reviewed changes

oliver-sanders and others added 2 commits June 4, 2021 12:04

hold: update task state in the n=1 window

d841193

Merge pull request #6 from oliver-sanders/hold

b04bfcf

Update task state in the n=1 window (data store delta)

MetRonnie added 2 commits June 4, 2021 13:20

Merge branch 'master' into hold

14291a6

Update unit tests with renamed fixture

3d271aa

oliver-sanders approved these changes Jun 7, 2021

View reviewed changes

This was referenced Jun 7, 2021

tui: expand cycles/families in mutations to the visible selection #4246

Open

aoft: expand cycles/families in mutations to the visible selection cylc/cylc-ui#686

Open

MetRonnie mentioned this pull request Jun 7, 2021

Update docs for cylc hold cylc/cylc-doc#249

Merged

MetRonnie added 2 commits June 8, 2021 10:44

cylc hold: only expand family for active tasks

3b8cc40

When holding e.g. FAM.2, only expand into tasks for active tasks, not future ones

Merge branch 'master' into hold

bd4b2fe

MetRonnie requested a review from hjoliver June 21, 2021 12:00

hjoliver mentioned this pull request Jun 22, 2021

Task hold/release needs to be flow-specific #4277

Open

hjoliver approved these changes Jun 22, 2021

View reviewed changes

hjoliver merged commit f906a0f into cylc:master Jun 22, 2021

MetRonnie deleted the hold branch June 22, 2021 08:14

This was referenced Jun 22, 2021

Resuming after play --pause can make held tasks run #4278

Closed

Completed tasks ought to get removed from tasks_to_hold set #4279

Open

MetRonnie added the db change Change to the workflow database structure label Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cylc hold to hold future tasks that haven't spawned yet #4238

Allow cylc hold to hold future tasks that haven't spawned yet #4238

MetRonnie commented Jun 2, 2021 •

edited

Loading

hjoliver left a comment

hjoliver commented Jun 3, 2021 •

edited

Loading

MetRonnie commented Jun 3, 2021

MetRonnie Jun 3, 2021

oliver-sanders Jun 3, 2021

oliver-sanders left a comment

oliver-sanders Jun 3, 2021

MetRonnie Jun 3, 2021

oliver-sanders Jun 3, 2021

MetRonnie Jun 3, 2021

hjoliver left a comment •

edited

Loading

oliver-sanders commented Jun 4, 2021

oliver-sanders commented Jun 4, 2021

oliver-sanders left a comment •

edited

Loading

oliver-sanders commented Jun 7, 2021

MetRonnie commented Jun 7, 2021

MetRonnie commented Jun 7, 2021

oliver-sanders commented Jun 7, 2021

MetRonnie commented Jun 7, 2021 •

edited

Loading

MetRonnie commented Jun 21, 2021

hjoliver commented Jun 22, 2021

hjoliver left a comment

Allow cylc hold to hold future tasks that haven't spawned yet #4238

Allow cylc hold to hold future tasks that haven't spawned yet #4238

Conversation

MetRonnie commented Jun 2, 2021 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Jun 3, 2021 • edited Loading

MetRonnie commented Jun 3, 2021

MetRonnie Jun 3, 2021

Choose a reason for hiding this comment

oliver-sanders Jun 3, 2021

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders Jun 3, 2021

Choose a reason for hiding this comment

MetRonnie Jun 3, 2021

Choose a reason for hiding this comment

oliver-sanders Jun 3, 2021

Choose a reason for hiding this comment

MetRonnie Jun 3, 2021

Choose a reason for hiding this comment

hjoliver left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Jun 4, 2021

oliver-sanders commented Jun 4, 2021

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Jun 7, 2021

MetRonnie commented Jun 7, 2021

MetRonnie commented Jun 7, 2021

oliver-sanders commented Jun 7, 2021

MetRonnie commented Jun 7, 2021 • edited Loading

MetRonnie commented Jun 21, 2021

hjoliver commented Jun 22, 2021

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie commented Jun 2, 2021 •

edited

Loading

hjoliver commented Jun 3, 2021 •

edited

Loading

hjoliver left a comment •

edited

Loading

oliver-sanders left a comment •

edited

Loading

MetRonnie commented Jun 7, 2021 •

edited

Loading