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

Task expire optionality. #6020

Closed
wants to merge 1 commit into from
Closed

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Mar 13, 2024

Salvaged from the aborted #5934, in case it's still useful.

validation:

  • clock-expire must be flagged as optional in the graph
  • and :expired must be optional if used

runtime expire:

  • can't expire an active task
  • manual expire: stall with incomplete task if expire was not optional
  • otherwise remove the expired task as complete

@oliver-sanders dunno if you've already re-implemented this functionality on your new post cylc set branch? If so, apologies for not posting this sooner.

There may be a couple of small errors in extracting this code from the original branch, but I won't bother with that, or tests, unless we decide to keep this.

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 this to the cylc-8.3.0 milestone Mar 13, 2024
@oliver-sanders
Copy link
Member

I'm currently working towards this on my branch, but still useful to see what you've done.

@hjoliver
Copy link
Member Author

OK, given that :expired already works as proposed on this branch, do you want to merge this (and rebase your new branch onto it), or close it as superseded by your new branch?

@hjoliver
Copy link
Member Author

hjoliver commented Mar 14, 2024

BTW note my use of TaskOutputs._expire_ok here. That's because :expired is either optional or not allowed, and if not allowed then completing the output makes the task incomplete 🤯

To fit the model better there should really be an opposite (probably just internal) :not-expired output.

  • by default, :not-expired is required
  • if :expired? is optional, then so is :not-expired?
  • if you manually expire a task that was not flagged in the graph as expire-able then it is incomplete because it did not complete its required :not-expired output

This would allow the same internals to be used for all outputs.

(Arguably :submitted is opposite to :expired but I'm not sure we can use it both way - , i.e. with :submit-failed as well).

@hjoliver
Copy link
Member Author

See also #6001 (comment)

@oliver-sanders
Copy link
Member

My branch does things a little differently as I've centralised some of the validation and requirement logic into the completion expression so I would rather superseed this branch.

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 14, 2024

completing the output makes the task incomplete

There are a couple of issues with the definition of completion as written, I have a cleaner solution that keeps the intended behaviour but fixes the edge cases, no opposite is required the way I've done it. I've not put it up yet as I want to get a bit further on with validation and testing to make sure it's watertight.

Sidenote, the opposites of expired are succeeded, failed and submit-failed (i.e. the other three "final" outputs). Due to the incremental accumulation of task outputs through multiple submissions, we can't really do not logic in Cylc (tangentially mentioned in proposal point 5).

Comment on lines +238 to +239
# Avoid clock-expiring an active task:
itask.expire_time = None
Copy link
Member

@oliver-sanders oliver-sanders Mar 14, 2024

Choose a reason for hiding this comment

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

I haven't handled clock-expiry of active tasks (I'm implementing the opt outputs proposal which is just about outputs).

Presumably if a task (submit-)fails and has [submission retries configured, then it should be allowed to expire between retries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Member

Choose a reason for hiding this comment

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

Created an issue for the clock-expire of active tasks thing: #6025

@hjoliver
Copy link
Member Author

hjoliver commented Mar 14, 2024

Sidenote, the opposites of expired are succeeded, failed and submit-failed

I would add submitted to that, since we don't plan to allow active tasks to expire.

Then my suggestion, to use an internal not-expired output is logically equivalent as it (not-expired) would only be "completed" by job submission, But I'll wait to see what you've got.

Due to the incremental accumulation of task outputs through multiple submissions, we can't really do not logic in Cylc

Agreed, but I don't think that's relevant here. I only meant "opposite" in the same sense that succeeded and failed are opposite, for a given job.

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 15, 2024

I would add submitted to that, since we don't plan to allow active tasks to expire.

It makes sense to disable (or postpone) clock-expires for active tasks. However, users can always set the "expired" output on a task, the same way they can set succeeded or failed.

(not-expired) would only be "completed" by job submission

Still not mutually exclusive.

@hjoliver
Copy link
Member Author

Closing as superseded by @oliver-sanders yet-to-posted optional outputs extension PR.

@hjoliver hjoliver closed this Mar 20, 2024
@hjoliver hjoliver removed this from the cylc-8.3.0 milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants