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.

Sign up for GitHub

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

Jump to bottom

Reconstruct xtriggers at reload time. #6263

Merged
merged 3 commits into from
Oct 23, 2024
Merged

Reconstruct xtriggers at reload time. #6263

merged 3 commits into from
Oct 23, 2024

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 25, 2024 •
edited
Loading

Close #6259
Close #6260

User-defined xtriggers (including wall-clock) need to be reconstructed after reload (to pick up changes), but auto-defined ones (retries) must be retained.

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 added the bug Something is wrong :( label Jul 25, 2024
@hjoliver hjoliver added this to the 8.3.4 milestone Jul 25, 2024
@hjoliver hjoliver self-assigned this Jul 25, 2024
@MetRonnie MetRonnie linked an issue Jul 25, 2024 that may be closed by this pull request
reload after xtrigger arg change #6259
Closed
@oliver-sanders oliver-sanders modified the milestones: 8.3.4, 8.3.5 Sep 11, 2024
@oliver-sanders oliver-sanders modified the milestones: 8.3.5, 8.3.6 Oct 4, 2024
@oliver-sanders oliver-sanders linked an issue Oct 16, 2024 that may be closed by this pull request
reload fails on adding a new xtrigger #6260
Closed
@oliver-sanders
Copy link
Member

This looks good to me.

Stuck together an integration test to help bump this along: hjoliver#58

@hjoliver
Copy link
Member Author

Thanks (sorry I let this one languish unfinished!)

@oliver-sanders
Copy link
Member

(no probs, just running through bugs, working out what I can help push forward)

@hjoliver
Copy link
Member Author

(Just working on the one failing integration test ....should be an easy fix)

@oliver-sanders oliver-sanders modified the milestones: 8.3.6, 8.3.7 Oct 18, 2024
@hjoliver
Copy link
Member Author

(Just working on the one failing integration test ....should be an easy fix)

Somewhat surprisingly, that revealed a problem, now fixed - see the new xtrigger purge_user_defined method on this branch.

@hjoliver hjoliver marked this pull request as ready for review October 22, 2024 00:13
@hjoliver hjoliver modified the milestones: 8.3.7, 8.3.6 Oct 22, 2024
@hjoliver
Copy link
Member Author

Tentatively pushing forward to 8.3.6 since it's ready to go now.

dwsutherland
dwsutherland approved these changes Oct 22, 2024
View reviewed changes
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.

Great, much needed!

Tested it locally with a workflow_state xtrigger.. Found working 🍻

oliver-sanders
oliver-sanders approved these changes Oct 22, 2024
View reviewed changes
changes.d/6263.fix.md Outdated Show resolved Hide resolved
cylc/flow/xtrigger_mgr.py Outdated Show resolved Hide resolved
tests/integration/test_task_pool.py Outdated Show resolved Hide resolved
@hjoliver @oliver-sanders
Apply suggestions from code review [skip ci]
72b663a 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@hjoliver hjoliver merged commit 28ea786 into cylc:8.3.x Oct 23, 2024
@hjoliver hjoliver deleted the reload-xtriggers branch October 23, 2024 05:36
This was referenced Nov 11, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders approved these changes

@dwsutherland dwsutherland dwsutherland approved these changes

Assignees

@hjoliver hjoliver

Labels
bug Something is wrong :(
Projects
None yet
Milestone
8.3.6
Development

Successfully merging this pull request may close these issues.

reload fails on adding a new xtrigger reload after xtrigger arg change
3 participants
@hjoliver @oliver-sanders @dwsutherland