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

Fix expire triggers. #5412

Closed
wants to merge 4 commits into from
Closed

Fix expire triggers. #5412

wants to merge 4 commits into from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Mar 15, 2023

And require ? (optional) on all expire triggers.

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 Tweak expire trigger docs. cylc-doc#596
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@hjoliver hjoliver added the bug Something is wrong :( label Mar 15, 2023
@hjoliver hjoliver added this to the cylc-8.1.3 milestone Mar 15, 2023
@hjoliver hjoliver self-assigned this Mar 15, 2023
@hjoliver hjoliver force-pushed the expire-trigger branch 3 times, most recently from a21b13d to 22def64 Compare March 16, 2023 02:15
Update change log.
Add new func test.
@hjoliver hjoliver marked this pull request as ready for review March 16, 2023 03:32
@hjoliver hjoliver changed the title Fix expire trigger. Fix expire triggers. Mar 16, 2023
@MetRonnie MetRonnie self-requested a review March 16, 2023 10:29
@hjoliver hjoliver marked this pull request as draft March 17, 2023 03:32
@hjoliver
Copy link
Member Author

hjoliver commented Mar 17, 2023

Converted back to draft temporarily, in light of today's project meeting.

I think have have nailed the expire stall/no-stall situation, and how expire relates to success (etc.) optionality. But I ran out of time to write it up this afternoon. Stand by ...

@hjoliver
Copy link
Member Author

hjoliver commented Mar 24, 2023

PR blocked pending resolution of the cylc set and task expiry proposals and discussion.

We will still need the ability to trigger tasks off of task expiry, but I have argued against continuing to treat :expire as a task output: https://cylc.github.io/cylc-admin/proposal-task-expire.html#expired-should-really-be-a-task-attribute-not-a-task-state

@oliver-sanders oliver-sanders modified the milestones: cylc-8.1.3, cylc-8.1.4 Apr 20, 2023
@oliver-sanders oliver-sanders modified the milestones: cylc-8.1.4, cylc-8.1.x May 4, 2023
@hjoliver hjoliver modified the milestones: cylc-8.1.x, cylc-8.2.0 Jun 13, 2023
@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.0, cylc-8.3.0 Jul 11, 2023
@MetRonnie MetRonnie changed the base branch from 8.1.x to master July 25, 2023 16:08
Comment on lines +745 to +748
if output == TASK_OUTPUT_EXPIRED and not optional:
raise GraphParseError(
f"Expired-output {name}:{output} must be optional")

Copy link
Member

Choose a reason for hiding this comment

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

This is now covered by the optional outputs stuff.

Suggested change
if output == TASK_OUTPUT_EXPIRED and not optional:
raise GraphParseError(
f"Expired-output {name}:{output} must be optional")

@hjoliver hjoliver mentioned this pull request Sep 25, 2023
16 tasks
@hjoliver
Copy link
Member Author

Closing as superseded by #5658

@hjoliver hjoliver closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( superseded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expiry: broken interaction with runahead limit :expire trigger broken graph parser: expire-all ?
2 participants