Trigger now if workflow paused #6499

hjoliver · 2024-11-28T02:03:16Z

Close #5963
Supesede #6192 (it was getting messy)
Close #6398

New behaviour for triggering when the workflow is paused

DEFAULT: run triggered tasks NOW even if the workflow is paused

supports the common use case "I want to pause the workflow and just trigger individual tasks"
(NIWA priority raised by both ops and research users in early Cylc 8 migration meetings)

OPTION: --on-resume - run triggered tasks only when the paused workflow is resumed

this supports the broadcast-to-past-tasks workaround until broadcast is fixed (prob. 8.5.0)
the option can be removed in the future, once broadcast is fixed, if there are no other use cases

Implementation:

separate start of job submission from queue release, in the scheduler
manual trigger adds tasks to tasks_to_trigger_(now|on_resume) lists, after state reset and queue wrangling
if these lists are populated, the scheduler will release tasks from them as appropriate
(queues are not processed if the scheduler is paused)

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 Update docs for trigger-when-paused. cylc-doc#778
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

hjoliver · 2024-11-28T03:38:26Z

Status of this PR:

I can easily remove the --on-resume option, if we have a viable alternative for the broadcast problem at 8.4.0
if we keep the option, I just need to extend the tests to cover it

dwsutherland · 2024-12-02T01:47:52Z

(queues are not processed if the scheduler is paused)

Does this mean if I trigger when paused, and a queue limit is reached, that it will enter a queue but not be released when the queue "empties" of running tasks?

Will test.

hjoliver · 2024-12-02T02:04:55Z

(queues are not processed if the scheduler is paused)

Does this mean if I trigger when pause, and a queue limit is reached, that it will enter a queue but not be released when the queue "empties" of running tasks?

Yes. Pausing the scheduler halts all automatic job submission, so that means queued tasks won't automatically run. But if you want the queued task to run before resuming the workflow you can just trigger it again before resuming.

hjoliver · 2024-12-02T02:34:45Z

(I've just tweaked the pause and trigger command help in an attempt to make that clearer @dwsutherland )

dwsutherland

LGTM, reacts as expect with queued/unqueued 👍

    [[queues]]
        [[[THIS]]]
            limit = 1
            members = baz, zip, toz
    [[graph]]
        R1 = "prep => foo"
        PT12H = "foo[-PT12H] => foo => baz & zip & toz => bar"

cylc/flow/network/schema.py

tests/functional/restart/58-waiting-manual-triggered.t

cylc/flow/network/schema.py

changes.d/6499.feat.md

cylc/flow/scripts/pause.py

cylc/flow/scripts/trigger.py

cylc/flow/task_queues/independent.py

oliver-sanders · 2024-12-03T15:52:02Z

@hjoliver, by bypassing queue processing for manual trigger, this PR may close #6398

cylc/flow/scripts/pause.py

cylc/flow/scripts/trigger.py

tests/integration/conftest.py

MetRonnie · 2024-12-03T16:32:01Z

tests/integration/test_flow_assignment.py


+        # set --pre=all should not cause a task to submit in a paused workflow
+        assert len(submitted_tasks) == 0
+        # but triggering it should
+        schd.pool.force_trigger_tasks(['1/a'], [])
+        schd.release_tasks_to_run()
+        assert len(submitted_tasks) == 1


Please could you put this new bit in a separate test and revert the splitting of trigger and set from this test; a key feature of this test was that is checks equity between trigger and set for the same flow nums

The reason I split that into separate tests is that this PR changes that equivalence.

Trigger and set (in a paused workflow) are more similar on master, because a triggered task does not become "active" until the workflow is resumed, so it can be triggered multiple times to absorb multiple flow numbers. On this branch, a triggered task becomes active immediately and so subsequent triggers get ignored.

In light of that, do you still want me to revert that? I suppose we could have a combined test for what is common, and separate tests for the behavior that differs.

In light of that, do you still want me to revert that? I suppose we could have a combined test for what is common, and separate tests for the behavior that differs.

👆 That

tests/integration/test_force_trigger.py

tests/integration/test_scheduler.py

hjoliver · 2024-12-03T22:53:56Z

@oliver-sanders -

@hjoliver, by bypassing queue processing for manual trigger, this PR may close #6398

That issue is confusing. I think the "bug" initially reported by Tom is just the expected double-trigger requirement to get past a queue. In his description, despite the tasks being held, they did run on the second trigger attempt - but he had expected that manual trigger would skip queues to run immediately after just one attempt.

So I presume you're referring to the presumably-actual-bug that you commented on part way through the discussion: #6398 (comment)

... I've commented there, as I'm not sure this PR is relevant: #6398 (comment)

oliver-sanders · 2024-12-04T10:35:12Z

I think the "bug" initially reported by Tom is just the expected double-trigger requirement to get past a queue.

No, there is definitely a bug lurking here.

I have added an example to the issue in which task doesn't run when triggered (no queue limiting in effect), however, on this branch the task does run (entering an infinite kill/trigger loop).

So it looks like this closes #6398 (congrats on fixing a bug you didn't think existed!).

MetRonnie · 2024-12-04T14:56:54Z

Test added on master failing - need to merge master into this branch

cylc/flow/scheduler.py

hjoliver · 2024-12-05T09:58:01Z

So it looks like this closes #6398 (congrats on fixing a bug you didn't think existed!).

Great, well done me. I'll add a close directive to the description above...

oliver-sanders · 2024-12-05T10:11:20Z

Great, well done me. I'll add a close directive to the description above...

(will see if I can get a test for this, ..., hjoliver#61)

One recently added test is failing:

FAILED tests/integration/run_modes/test_skip.py::test_doesnt_release_held_tasks

cylc/flow/commands.py

cylc/flow/scripts/trigger.py

cylc/flow/task_pool.py

tests/functional/cylc-trigger/07-kill-trigger/reference.log

cylc/flow/task_pool.py

MetRonnie

Consider this approved pending addressing of comments (including #6499 (comment))

cylc/flow/network/schema.py

oliver-sanders · 2024-12-10T16:19:22Z

(have manually run the auto-restart tests on this branch, they all pass)

cylc/flow/scheduler.py

oliver-sanders · 2024-12-10T16:37:59Z

cylc/flow/scheduler.py

+                if self.pool.tasks_to_trigger_on_resume:
+                    # and manually triggered tasks to run once workflow resumed
+                    pre_prep_tasks.update(self.pool.tasks_to_trigger_on_resume)
+                    self.pool.tasks_to_trigger_on_resume = set()


I think codecov is right, these lines are untested.

Correct, I was waiting for agreement to add the option before adding tests for it, then failed to come back to the PR till now.

Co-authored-by: Ronnie Dutta <[email protected]>

oliver-sanders

Approved pending resolution of Ronnie's comment above and, ideally, some kind of test for the --on-resume option.

hjoliver added this to the 8.4.0 milestone Nov 28, 2024

hjoliver self-assigned this Nov 28, 2024

hjoliver force-pushed the trigger-when-paused_2 branch from 98d500e to 3dc032c Compare November 28, 2024 02:05

hjoliver requested review from oliver-sanders and dwsutherland November 28, 2024 02:06

hjoliver mentioned this pull request Nov 28, 2024

Update docs for trigger-when-paused. cylc/cylc-doc#778

Open

2 tasks

hjoliver changed the title ~~Trigger --now for workflow pause and queues.~~ Trigger now if workflow paused Nov 28, 2024

hjoliver force-pushed the trigger-when-paused_2 branch 3 times, most recently from 55c71a9 to 0372d91 Compare November 29, 2024 01:07

MetRonnie self-requested a review November 29, 2024 17:35

hjoliver force-pushed the trigger-when-paused_2 branch from 645707c to 02f1a22 Compare December 2, 2024 02:34

hjoliver force-pushed the trigger-when-paused_2 branch from 02f1a22 to cb03ae0 Compare December 2, 2024 02:36

dwsutherland approved these changes Dec 2, 2024

View reviewed changes

cylc/flow/network/schema.py Outdated Show resolved Hide resolved

tests/functional/restart/58-waiting-manual-triggered.t Show resolved Hide resolved

MetRonnie reviewed Dec 3, 2024

View reviewed changes

oliver-sanders reviewed Dec 3, 2024

View reviewed changes

cylc/flow/scripts/pause.py Outdated Show resolved Hide resolved

cylc/flow/scripts/trigger.py Outdated Show resolved Hide resolved

tests/integration/conftest.py Show resolved Hide resolved

MetRonnie added the schema change Change to the Cylc GraphQL schema label Dec 3, 2024

MetRonnie reviewed Dec 3, 2024

View reviewed changes

tests/integration/test_force_trigger.py Outdated Show resolved Hide resolved

MetRonnie reviewed Dec 3, 2024

View reviewed changes

tests/integration/test_scheduler.py Outdated Show resolved Hide resolved

hjoliver mentioned this pull request Dec 4, 2024

2024 meetings cylc/cylc-admin#188

Open

MetRonnie reviewed Dec 4, 2024

View reviewed changes

cylc/flow/scheduler.py Show resolved Hide resolved

hjoliver mentioned this pull request Dec 5, 2024

Held tasks need multiple triggers to run? #6398

Open

MetRonnie reviewed Dec 9, 2024

View reviewed changes

cylc/flow/commands.py Show resolved Hide resolved

cylc/flow/scripts/trigger.py Outdated Show resolved Hide resolved

MetRonnie reviewed Dec 10, 2024

View reviewed changes

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

MetRonnie reviewed Dec 10, 2024

View reviewed changes

tests/functional/cylc-trigger/07-kill-trigger/reference.log Show resolved Hide resolved

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

MetRonnie reviewed Dec 10, 2024

View reviewed changes

oliver-sanders reviewed Dec 10, 2024

View reviewed changes

cylc/flow/network/schema.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Dec 10, 2024

View reviewed changes

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

oliver-sanders reviewed Dec 10, 2024

View reviewed changes

hjoliver and others added 13 commits December 10, 2024 21:47

Manual trigger: run now even when paused.

68ce6a1

Fix command help text [skip ci]

8a1d7e3

Simplify integration test

7d6c18e

Apply suggestions from code review

02c9f01

Co-authored-by: Ronnie Dutta <[email protected]>

Update cylc/flow/task_queues/independent.py

374fe0b

Co-authored-by: Ronnie Dutta <[email protected]>

Remove a redundant queue method.

3bd6040

Apply suggestions from code review

cce3c8a

Co-authored-by: Ronnie Dutta <[email protected]>

test cylc#6398

da04357

Update cylc/flow/commands.py

fae72ed

Co-authored-by: Ronnie Dutta <[email protected]>

Update cylc/flow/scripts/trigger.py

9cf0fb6

Co-authored-by: Ronnie Dutta <[email protected]>

Update cylc/flow/task_pool.py

9c5b722

Co-authored-by: Ronnie Dutta <[email protected]>

Update cylc/flow/task_pool.py

00f9c39

Co-authored-by: Ronnie Dutta <[email protected]>

Update queue info in trigger help.

bdbcaa0

hjoliver force-pushed the trigger-when-paused_2 branch from d00915e to bdbcaa0 Compare December 10, 2024 21:48

hjoliver added 3 commits December 11, 2024 00:49

Update a test.

38e5ab9

Fix new functional test.

c4ef803

Add a comment.

7d6d82e

oliver-sanders approved these changes Dec 11, 2024

View reviewed changes

Merge branch 'master' into trigger-when-paused_2

3d75bd2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger now if workflow paused #6499

Trigger now if workflow paused #6499

hjoliver commented Nov 28, 2024 •

edited

Loading

hjoliver commented Nov 28, 2024 •

edited

Loading

dwsutherland commented Dec 2, 2024 •

edited

Loading

hjoliver commented Dec 2, 2024

hjoliver commented Dec 2, 2024 •

edited

Loading

dwsutherland left a comment

oliver-sanders commented Dec 3, 2024

MetRonnie Dec 3, 2024

hjoliver Dec 4, 2024

MetRonnie Dec 4, 2024

hjoliver commented Dec 3, 2024 •

edited

Loading

oliver-sanders commented Dec 4, 2024 •

edited

Loading

MetRonnie commented Dec 4, 2024

hjoliver commented Dec 5, 2024

oliver-sanders commented Dec 5, 2024 •

edited

Loading

MetRonnie left a comment

oliver-sanders commented Dec 10, 2024

oliver-sanders Dec 10, 2024

hjoliver Dec 10, 2024

oliver-sanders left a comment

Trigger now if workflow paused #6499

Are you sure you want to change the base?

Trigger now if workflow paused #6499

Conversation

hjoliver commented Nov 28, 2024 • edited Loading

hjoliver commented Nov 28, 2024 • edited Loading

dwsutherland commented Dec 2, 2024 • edited Loading

hjoliver commented Dec 2, 2024

hjoliver commented Dec 2, 2024 • edited Loading

dwsutherland left a comment

Choose a reason for hiding this comment

oliver-sanders commented Dec 3, 2024

MetRonnie Dec 3, 2024

Choose a reason for hiding this comment

hjoliver Dec 4, 2024

Choose a reason for hiding this comment

MetRonnie Dec 4, 2024

Choose a reason for hiding this comment

hjoliver commented Dec 3, 2024 • edited Loading

oliver-sanders commented Dec 4, 2024 • edited Loading

MetRonnie commented Dec 4, 2024

hjoliver commented Dec 5, 2024

oliver-sanders commented Dec 5, 2024 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders commented Dec 10, 2024

oliver-sanders Dec 10, 2024

Choose a reason for hiding this comment

hjoliver Dec 10, 2024

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

hjoliver commented Nov 28, 2024 •

edited

Loading

hjoliver commented Nov 28, 2024 •

edited

Loading

dwsutherland commented Dec 2, 2024 •

edited

Loading

hjoliver commented Dec 2, 2024 •

edited

Loading

hjoliver commented Dec 3, 2024 •

edited

Loading

oliver-sanders commented Dec 4, 2024 •

edited

Loading

oliver-sanders commented Dec 5, 2024 •

edited

Loading