Trigger now if workflow paused #6499

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

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

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

Simplify integration test

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

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

MetRonnie · 2024-12-04T15:06:16Z

cylc/flow/scheduler.py

@@ -1703,7 +1740,7 @@ async def _main_loop(self) -> None:
                self.broadcast_mgr.check_ext_triggers(
                    itask, self.ext_trigger_queue)

-            if itask.is_ready_to_run():
+            if itask.is_ready_to_run() and not itask.is_manual_submit:


What is the reason for this change?

I think because manual submits may now bypass the queue, they get pushed through the submission pipeline (lines 1440-1450 above).

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

hjoliver · 2024-12-05T03:00:34Z

(I ran out of time today, but I just need to tweak the tests as requested by @MetRonnie ... )

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 2 times, most recently from f3587d1 to 55c71a9 Compare November 29, 2024 01:05

Manual trigger: run now even when paused.

0372d91

hjoliver force-pushed the trigger-when-paused_2 branch 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

Fix command help text [skip ci]

cb03ae0

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

Simplify integration test

dedc290

MetRonnie reviewed Dec 3, 2024

View reviewed changes

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

hjoliver and others added 2 commits December 4, 2024 10:49

Merge pull request #60 from MetRonnie/paused-trigger-aux

bc23b2e

Simplify integration test

Apply suggestions from code review

5830f3b

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

hjoliver and others added 2 commits December 4, 2024 11:00

Update cylc/flow/task_queues/independent.py

d16367e

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

Remove a redundant queue method.

0ea2a10

hjoliver mentioned this pull request Dec 4, 2024

2024 meetings cylc/cylc-admin#188

Open

MetRonnie reviewed Dec 4, 2024

View reviewed changes

Apply suggestions from code review

20e9906

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

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

MetRonnie Dec 4, 2024

oliver-sanders Dec 4, 2024 •

edited

Loading

hjoliver commented Dec 5, 2024

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

MetRonnie Dec 4, 2024

Choose a reason for hiding this comment

oliver-sanders Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

hjoliver commented Dec 5, 2024

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 Dec 4, 2024 •

edited

Loading