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

Improved cylc release task matching. #5752

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Oct 2, 2023

The release command doesn't need to do generic matching of future tasks, because we can only release tasks that are already held - and those are all recorded in the tasks-to-hold list.

That being the case, we can glob-match in both the task pool and the tasks-to-hold list, to identify tasks to release.

Close #5750

Bonus - cylc remove now discards removed tasks from the tasks-to-hold list.

Motivation: one way of handling graph rewind is:

  • hold queued tasks and kill/hold active tasks in the current flow
  • then remove them all before triggering the new flow

The new flow should not get held when it reaches the previously-removed tasks from the original flow.

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).
  • CHANGES.md 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 marked this pull request as ready for review October 3, 2023 05:24
@hjoliver
Copy link
Member Author

hjoliver commented Oct 3, 2023

Reviewers, an easy way to test this branch:

[scheduling]
   cycling mode = integer
   initial cycle point = 1
   final cycle point = 3
   [[graph]]
      P1 = """
         holder => a & b & c => d & e & f
      """
[runtime]
   [[holder]]
      script = """
         P=${CYLC_TASK_CYCLE_POINT}
         cylc hold ${CYLC_WORKFLOW_ID} //$P/a //$P/b //$P/c //$P/d //$P/e //$P/f
      """
   [[FAM, BAM]]
   [[a,b]]
      inherit = FAM
   [[d,e]]
      inherit = BAM
   [[c,f]]

So, a, b, c end up held in the task pool, and d, e, f flagged as future tasks to be held, in every cycle.

Now mess about with cylc release globs to release 'em.

@oliver-sanders
Copy link
Member

Bonus - cylc remove now discards removed tasks from the tasks-to-hold list.

Good spot!

@oliver-sanders oliver-sanders added this to the cylc-8.3.0 milestone Oct 11, 2023
cylc/flow/config.py Outdated Show resolved Hide resolved
@hjoliver hjoliver marked this pull request as draft March 11, 2024 04:36
@hjoliver
Copy link
Member Author

(I've just come back to this, will un-draft when done)

@hjoliver hjoliver modified the milestones: cylc-8.3.0, cylc-8.4.0 Mar 14, 2024
@hjoliver hjoliver added the could be better Not exactly a bug, but not ideal. label Mar 26, 2024
@hjoliver hjoliver force-pushed the release-match-list branch 2 times, most recently from 538a70f to a027e52 Compare March 26, 2024 08:40
@hjoliver hjoliver modified the milestones: cylc-8.4.0, cylc-8.3.0 Mar 26, 2024
@hjoliver hjoliver marked this pull request as ready for review March 26, 2024 08:43
@hjoliver
Copy link
Member Author

hjoliver commented Mar 26, 2024

Ready for review (if tests are good). I've pushed it forward to 8.3.0 - not critical but useful and probably easy to review and test.

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, works for me:
before

2024-04-08T20:22:07+12:00 INFO - Command "release" received. ID=4294af93-ed58-461f-b262-b432641f4fc7
    release(tasks=['1/d'])
2024-04-08T20:22:08+12:00 INFO - Command "release" actioned. ID=4294af93-ed58-461f-b262-b432641f4fc7
2024-04-08T20:22:57+12:00 INFO - Command "release" received. ID=08777829-a3c4-458e-9ef8-b7eeb4fa0529
    release(tasks=['*/d'])
2024-04-08T20:22:58+12:00 WARNING - No active tasks matching: */d
2024-04-08T20:22:58+12:00 INFO - Command "release" actioned with 1 warnings. ID=08777829-a3c4-458e-9ef8-b7eeb4fa0529

after

2024-04-08T20:25:28+12:00 INFO - Command "release" received. ID=0f774513-6307-437d-890a-807c846ea142
    release(tasks=['1/d'])
2024-04-08T20:25:29+12:00 INFO - Task hold list:
     * 3/b
     * 2/f
     * 1/b
     * 2/e
     * 1/e
     * 3/e
     * 3/c
     * 3/a
     * 3/d
     * 3/f
     * 2/c
     * 2/a
     * 2/d
     * 1/c
     * 1/a
     * 1/f
     * 2/b
2024-04-08T20:25:29+12:00 INFO - Command "release" actioned. ID=0f774513-6307-437d-890a-807c846ea142
2024-04-08T20:26:11+12:00 INFO - Command "release" received. ID=256e25a9-ff87-4639-a1d1-3aecc70c1263
    release(tasks=['*/d'])
2024-04-08T20:26:11+12:00 INFO - Task hold list:
     * 3/b
     * 2/f
     * 1/b
     * 2/e
     * 1/e
     * 3/e
     * 3/c
     * 3/a
     * 3/f
     * 2/c
     * 2/a
     * 1/c
     * 1/a
     * 1/f
     * 2/b

quite verbose .. but not really an issue I suppose, maybe for large workflows it might be a bit much (?)..

cylc/flow/task_hold_mgr.py Show resolved Hide resolved
@hjoliver
Copy link
Member Author

quite verbose .. but not really an issue I suppose, maybe for large workflows it might be a bit much (?)..

We don't have another way to expose the complete list of all held and to-be-held tasks to users yet, so I figured the log was best for the moment. Could put it to DEBUG mode I guess.

@oliver-sanders oliver-sanders self-requested a review April 29, 2024 15:16
@hjoliver hjoliver modified the milestones: 8.3.0, 8.4.0 May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc release: only need to match in the tasks-to-hold list
3 participants