-
Notifications
You must be signed in to change notification settings - Fork 93
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
Implement "cylc set" command #5658
Conversation
2b67dc7
to
1901c8c
Compare
1901c8c
to
ca382da
Compare
53adcb9
to
3812182
Compare
Rebased. Tests fixed. Will finish this off ASAP. |
50221af
to
a476e7d
Compare
756b52f
to
ed7358f
Compare
Unrelated (?) mypy error from the Py 3.10 and 3.11 fast tests:
[update: fixed by #5735] |
5a175a0
to
bdf5f29
Compare
@oliver-sanders - I just took a quick look at your "Extension" part of the proposal:
Correct me if I'm wrong, but to do this we need to take the The trouble is, the
|
0c091e9
to
84ad390
Compare
@oliver-sanders - this now incorporates (and supersedes) #5412 because I needed to handle (and test consequences of, such as triggering) set to expired. So there will be a bit of overlap and conflict with #5651, but hopefully not too much. |
@oliver-sanders - I think there's a small error in the proposal example 5
The DB should record
Agree? |
6806086
to
a0fc1cc
Compare
Yep all happy, can leave the lint stuff, just making a last pass... Sorry, found another issue :(, hopefully a quick one: If I pause a workflow (any graph where success is required):
Then set the failed output on any queued task:
Then resume the workflow:
The task submits a job and goes from failed to running:
Whereas, if I set
Then the workflow stalls as expected:
I think setting a finished output on a queued task should cause it to be removed from the queue? Curiously, the same doesn't appear to happen if I set |
I also spotted that the Another one for validation? |
👍 Added a comment to #6013 |
Yes it should.
That's because completed tasks leave the task pool (and the queue). I've pushed a small fix. Still needs a test, and to check if I overdid the new DB updates, but I'm out of time for today. It also cleans up the |
I'll have a go at that: hjoliver#48
I'm not quite sure which updates these are... else I'd test for them too. |
Two things going on here:
|
tokens_list: t.Optional[t.List[Tokens]], | ||
): | ||
"""List task outputs.""" | ||
return (await _list_prereqs_and_outputs(tokens_list))[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to offer this suggestion?
return (await _list_prereqs_and_outputs(tokens_list))[1] | |
return (await _list_prereqs_and_outputs(tokens_list))[1] + ['required'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required
applies only to outputs, this interface returns outputs AND prerequisites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that [1]
is just the outputs:
prereqs, outputs = (await _list_prereqs_and_outputs(tokens_list))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this to you two to agree as a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another play today, I couldn't find any ways to break it 😓 🎉 !
Already building the next part on top of this branch, expect a PR in a week or so, distractions permitting.
Might want a rebase-squash, much as a love commit messages like "Fix screwed conflict resolution".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do a full review but all my points have been addressed
Long, messy dev history squashed flat like a bug.
Squashed. Although I do genuinely love commit messages like that. |
If the tests pass, this bad boy is going in. |
Phew 😌 |
Implement accepted proposal A New Command for Manual Task Forcing
Supersede #5412 (fix expire triggers)
TODO
set-outputs
toset
Side effects:
cylc set-verbosity
tocylc verbosity
to avoid command ambiguity apocalypseFollow-up:
cylc set --pre=cycle
PR Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users