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

Suicide trigger prevents later cycles starting #6602

Open
wants to merge 1 commit into
base: 8.4.x
Choose a base branch
from
Open

Suicide trigger prevents later cycles starting #6602

wants to merge 1 commit into from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Feb 10, 2025 •
edited
Loading

Closes #6594

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.

@wxtim wxtim marked this pull request as draft February 10, 2025 09:31
@wxtim wxtim changed the title ICP Bug Suicide trigger prevents later cycles starting Feb 10, 2025
@wxtim wxtim marked this pull request as ready for review February 11, 2025 10:40
@wxtim wxtim changed the base branch from master to 8.4.x February 11, 2025 10:44
@wxtim wxtim force-pushed the fix.icp-fail branch 2 times, most recently from 23bc488 to 0700c90 Compare February 11, 2025 10:45
oliver-sanders
oliver-sanders reviewed Feb 11, 2025
View reviewed changes
cylc/flow/taskdef.py
# Don't count self-suicide as a normal trigger.
dep.suicide and trig.task_name == self.name
# Don't count suicide as a normal trigger:
dep.suicide
Copy link
Member

@oliver-sanders oliver-sanders Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This undoes a change @hjoliver made in #4970

5e7ffad#diff-b46dc58446bab522ef576c6d2e32a4276f57d60ff62e930e6f47762d47bf9507R296

I think this change makes sense, but will need to be reviewed in the context of whatever #4970 was trying to resolve.

Copy link
Member

Choose a reason for hiding this comment

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

Good spotting ...

Copy link
Member

@oliver-sanders oliver-sanders Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(leaving you two to resolve the differences between these changes)

Copy link
Member Author

Choose a reason for hiding this comment

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

@hjoliver what do you think we should do about this?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I got distracted on the way to looking at that...

@oliver-sanders oliver-sanders added this to the 8.4.x milestone Feb 13, 2025
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 left review comments

@hjoliver hjoliver Awaiting requested review from hjoliver

At least 1 approving review is required to merge this pull request.

Assignees

@wxtim wxtim

Labels
None yet
Projects
None yet
Milestone
8.4.x
Development

Successfully merging this pull request may close these issues.

Suicide trigger prevents cycles starting
3 participants
@wxtim @hjoliver @oliver-sanders