Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group trigger #6395

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Group trigger #6395

wants to merge 5 commits into from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Oct 6, 2024

Implement group trigger proposal: cylc/cylc-admin#197

(Not approved yet, but it's a clear winner and a relatively small change, and I wanted to finish coding it up - after initial experiments - while still fresh in the brain).

On current master:

  • on triggering one or more tasks: run them all immediately regardless of prerequisites.

On this branch:

  • on triggering one or more tasks, satisfy any off-group (aka off-flow) prerequisites

Result: we can trigger a sub-graph "naturally" by specifying all of its member tasks:

  • tasks with only off-group prerequisites will trigger immediately
  • the subsequent flow will respect in-group prerequisites
  • no stalling due to unsatisfied off-group prerequisites

This provides an alternative, easier for some cases, to triggering just the initial task(s) of the sub-graph and setting off-flow prerequisites (if any) manually.


example

[scheduler]
    allow implicit tasks = True
[scheduling]
    [[graph]]
        R1 = """
            foo => a => b => c => bar
            off => b
        """
[runtime]
    [[root]]
        pre-script = sleep 4
    [[bar]]
        script = "test $CYLC_TASK_SUBMIT_NUMBER != 1"

Run it and wait for bar to fail and stall the workflow.

$ cylc trigger grp //1/a //1/b //1/c --flow=new
  # 1/a runs immediately in flow 2 - its only prerequisite (foo => a) is off-group 
  # 1/b gets its off-group prerequisite satisfied (off => b) and spawns to wait on in-group (a => b)
  # flow 2 traverses a => b => c, then flows on to merge with and run bar to complete the workflow

caveats?

Triggering a future sub-graph, or a past sub-graph with --flow=new, is safe and easy.

But triggering a past sub-graph in the original flow comes with a gotcha: setting off-group prerequisites spawns the target task to wait on other prerequisites, which aren't going to get satisfied when the original flow does not re-traverse the graph.

This isn't technically incorrect. The exact same thing would happen if we attempted to trigger the same past sub-graph the spawn-on-demand "native" way in the same flow: trigger the initial task and set the off-flow prerequisite.

But maybe we can do something to mitigate the problem if users try to do this.


interaction with the upcoming cylc remove extension

On this branch now, to re-run a past sub-graph you have to use --flow=new.

After the remove extension we'll be able to erase the flow history and re-run a past sub-graph in the original flow. We may want to add an option to cylc trigger to do that automatically, or even make it the default.


interaction with upcoming inactive task matching developments

We can't trigger a group of inactive tasks by family name or glob yet, but that's a general problem, not specific to this branch.


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.

@hjoliver hjoliver self-assigned this Oct 6, 2024
@hjoliver
Copy link
Member Author

hjoliver commented Oct 7, 2024

The test battery passes, so this does not break existing trigger and set functionality 🎉 I just need to get some more coverage of the new code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant