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

Allow manual trigger when workflow paused. #6192

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

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 4, 2024

Close #5963

I've added the "question" label because there's some disagreement on the issue itself:

@oliver-sanders has argued that manual triggering (i.e., with immediate effect) should only be allowed with cylc hold */* (hold all tasks individually) and not cylc pause (pause the workflow).

However:

  • we haven't implemented cylc hold */* yet, and it may not be trivial to do it
  • the only solution at the moment is "hold after cycle point", which is not intuitive at all
  • two primary Cylc sites have highlighted this as a must-have for operational migration - @dwsutherland, @ColemanTom
  • this is very easy to implement (+13/-7 code lines without tests) so at the very least it provides a working interim solution
  • and finally, I cannot see any problem with allowing manual triggering in a paused workflow - there's nothing wrong with defining workflow pause as pausing all automatic job submission

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 added small question Flag this as a question for the next Cylc project meeting. could be better Not exactly a bug, but not ideal. labels Jul 4, 2024
@hjoliver hjoliver added this to the 8.3.1 milestone Jul 4, 2024
@hjoliver hjoliver self-assigned this Jul 4, 2024
@oliver-sanders oliver-sanders modified the milestones: 8.3.1, 8.3.2 Jul 4, 2024
@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 9, 2024

I would prefer to have a clean separation of play/pause and hold/release rather than crossing the concepts. I'm also not convinced by the "operational blocker" statement, especially as there is a documented workaround (i.e. this is a preference not a requirement).

However, I am quite happy to concede if this is desired. Removing the question label.

This change shouldn't really go into a bugfix release, but it's small enough that I think it's ok to sneak it in if we really want to.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

From a quick functional test, this works as expected:

  • cylc trigger causes a task to submit
  • cylc set --pre=all does not.

Comment on lines +59 to +62
if (
itask.state.is_held or
(is_paused and not itask.is_manual_submit)
):
Copy link
Member

@oliver-sanders oliver-sanders Jul 9, 2024

Choose a reason for hiding this comment

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

We would expect this functionality to work the same across all queue implementations, so we could really do with abstracting it into the base class rather than relying on each queue to implement it separately.

Note: If we could bypass the queue entirely that would be ideal.

Idea (for future work), write some abstract tests that should hold for all queue implementations.

@@ -1225,8 +1225,7 @@ def release_queued_tasks(self) -> bool:

"""
if (
not self.is_paused
and self.stop_mode is None
self.stop_mode is None
Copy link
Member

@oliver-sanders oliver-sanders Jul 9, 2024

Choose a reason for hiding this comment

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

there's nothing wrong with defining workflow pause as pausing all automatic job submission

This has gone a little further than that one word change above as we are now continuing to process the queues whilst the workflow is paused making the paused state a little more dynamic than before.

I don't think we actually need to process the entire queue to achieve this, we just need to skip any manually triggered tasks through the queue (ideally bypassing the queue itself if possible).

If we pass self.is_paused through to release_queued_tasks we could reduce the scope of this override such that it only impacts manually triggered tasks.

@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Jul 9, 2024
@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 9, 2024

Needs tests (could do with covering the behaviour w/ set --pre=all and trigger), CLI help and a cylc-doc PR to update the interventions page & glossary.

@wxtim wxtim modified the milestones: 8.3.2, 8.3.3 Jul 10, 2024
@MetRonnie MetRonnie modified the milestones: 8.3.3, 8.4.0 Jul 11, 2024
@hjoliver hjoliver marked this pull request as draft August 15, 2024 02:53
@hjoliver
Copy link
Member Author

(Will undraft again soon, once I've tried your suggestion re queue processing @oliver-sanders )

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. small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow trigger-now in a paused workflow?
4 participants