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

Implement "cylc set" command #5658

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Implement "cylc set" command #5658

merged 1 commit into from
Mar 13, 2024

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 31, 2023

Implement accepted proposal A New Command for Manual Task Forcing

Supersede #5412 (fix expire triggers)

TODO

  • Rename set-outputs to set
  • Update the CLI and GraphQL schema
  • Custom outputs: prohibit "all", "required", "optional" keywords, "," character, and "_cylc" prefix.
  • Reproduce old set-outputs behaviour with new command options and query
  • Implement the new logic for setting prerequisites [FIRST CUT done]
  • Implement the new logic for setting outputs
  • Test new schema works as expected in the UI
  • Tests for all new functionality
  • Update documentation -> Punted to document: cylc set cylc-doc#688
  • This is a breaking change, add an entry into the cylc-doc changes page -> Punted to document: cylc set cylc-doc#688

Side effects:

  • rename cylc set-verbosity to cylc verbosity to avoid command ambiguity apocalypse
  • command UUIDs
  • cleaner log message prefix for tasks

Follow-up:

  • cylc set --pre=cycle

PR 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

@hjoliver hjoliver added this to the cylc-8.3.0 milestone Jul 31, 2023
@hjoliver hjoliver self-assigned this Jul 31, 2023
@hjoliver hjoliver changed the title Implement new "cylc set task" command Implement new "cylc set-task" command Jul 31, 2023
@hjoliver hjoliver force-pushed the cylc-set-task branch 2 times, most recently from 2b67dc7 to 1901c8c Compare August 1, 2023 11:01
@hjoliver
Copy link
Member Author

hjoliver commented Sep 4, 2023

Rebased. Tests fixed. Will finish this off ASAP.

@hjoliver hjoliver force-pushed the cylc-set-task branch 3 times, most recently from 50221af to a476e7d Compare September 5, 2023 13:40
@dwsutherland dwsutherland mentioned this pull request Sep 6, 2023
7 tasks
@hjoliver hjoliver changed the title Implement new "cylc set-task" command Implement "cylc set" task intervention command Sep 6, 2023
@hjoliver hjoliver mentioned this pull request Sep 7, 2023
10 tasks
@hjoliver hjoliver force-pushed the cylc-set-task branch 2 times, most recently from 756b52f to ed7358f Compare September 11, 2023 06:55
@hjoliver hjoliver changed the title Implement "cylc set" task intervention command Implement "cylc set" command Sep 11, 2023
@hjoliver
Copy link
Member Author

hjoliver commented Sep 19, 2023

Unrelated (?) mypy error from the Py 3.10 and 3.11 fast tests:

/opt/hostedtoolcache/Python/3.11.5/x64/lib/python3.11/site-packages/matplotlib/pyplot.py:2700:
 error: Positional-only parameters are only supported in Python 3.8 and greater  [syntax]
Found 1 error in 1 file (errors prevented further checking)

[update: fixed by #5735]

cylc/flow/id.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

@oliver-sanders - I just took a quick look at your "Extension" part of the proposal:

When setting prereqs/outputs it would be useful to provide visual confirmation of the result.
We could display the cylc show output (which can be generated Scheduler side without the requirement for a second API call).

Correct me if I'm wrong, but to do this we need to take the set call out of the asynchronous command queue in the scheduler, and instead call it directly via a wrapper method in the resolver.

The trouble is, the set call updates task DB tables, and I get this:

self.conn.executemany(stmt, stmt_args_list)
    sqlite3.ProgrammingError: SQLite objects created in a thread can only be used in that same thread. 

@hjoliver
Copy link
Member Author

@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.

@hjoliver
Copy link
Member Author

@oliver-sanders - I think there's a small error in the proposal example 5

$ cylc set //foo --out=a,succeeded --wait

The database will record:

foo: state=waiting, outputs={a}

The DB should record state=succeeded and outputs={a, submitted, started, succeeded} because:

  • completed outputs affect task status
  • we set succeeded, and it has implied outputs

Agree?

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 11, 2024

I've gone back over all the unresolved comments, pretty sure everything has been addressed now

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):

- ~me
   - sequential/run1 - paused
      - ̿○ 1
           ̿○ a
      - ○ 2
           ○ a

Then set the failed output on any queued task:

cylc set //1/a --out=failed
- ~me
   - sequential/run1 - paused 1■
      - ̿⊗ 1
           ̿⊗ a
      - ○ 2
           ○ a

Then resume the workflow:

cylc play

The task submits a job and goes from failed to running:

- ~me
   - sequential/run1 - running 1■
      - ⊙ 1
         + ⊙ ■ a
      - ○ 2
           ○ a
      - ○ 3
           ○ a

Whereas, if I set 2/a:failed instead:

- ~me
   - sequential/run1 - paused 1■
      - ̿○ 1
           ̿○ a
      - ⊗ 2
           ⊗ a

Then the workflow stalls as expected:

- ~me
   - sequential/run1 - running 1■ 1■
      - ● 1
         + ● ■ a
      - ⊗ 2
           ⊗ a
      - ○ 3
           ○ a

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 1/a:succeeded so presumably there's already some logic protecting against a completed task being (re)run?

@oliver-sanders
Copy link
Member

I also spotted that the --pre option doesn't appear to work with the --wait option, which makes sense.

Another one for validation?

@hjoliver
Copy link
Member Author

I also spotted that the --pre option doesn't appear to work with the --wait option, which makes sense.

Another one for validation?

👍 Added a comment to #6013

@hjoliver
Copy link
Member Author

hjoliver commented Mar 12, 2024

I think setting a finished output on a queued task should cause it to be removed from the queue?

Yes it should.

Curiously, the same doesn't appear to happen if I set 1/a:succeeded so presumably there's already some logic protecting against a completed task being (re)run?

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 failed(queued) and failed(runahead) bits, which I'd been meaning to come back to anyway.

@wxtim
Copy link
Member

wxtim commented Mar 12, 2024

needs a test

I'll have a go at that: hjoliver#48

and to check if I overdid the new DB updates

I'm not quite sure which updates these are... else I'd test for them too.

@wxtim wxtim self-requested a review March 12, 2024 12:43
@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 12, 2024

The workflow shuts down after 1069, rather than jumping the 1070 cycle and going to 1071. Is this expected behaviour?

Two things going on here:

  1. The handling of task expiry.

    On this branch expired tasks are removed from the pool instantly, expiry can not count towards task completion.

    This will change with optional outputs: implement new proposal #5640 (also 8.3.0). After this change your example should stall rather than shutting down as completed.

  2. The handling of parentless tasks.

    This part is working as expected, parentless tasks work by a somewhat similar logic to SoS in that task proxies spawn their successors (or more technically the task pool spawns parentless successors according to what is in the pool).

    E.G. If you remove all instances of a parentless task in the pool then you are also removing future instances from the workflow. SoD spawns on upstream outputs, parentless tasks have no upstream outputs so they don't fit the model great, it's not perfect, but it's good enough and we haven't come up with a better approach.

tokens_list: t.Optional[t.List[Tokens]],
):
"""List task outputs."""
return (await _list_prereqs_and_outputs(tokens_list))[1]
Copy link
Member

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?

Suggested change
return (await _list_prereqs_and_outputs(tokens_list))[1]
return (await _list_prereqs_and_outputs(tokens_list))[1] + ['required']

Copy link
Member

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.

Copy link
Member

@wxtim wxtim Mar 12, 2024

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))

Copy link
Member Author

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.

Copy link
Member

@oliver-sanders oliver-sanders left a 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".

Copy link
Member

@MetRonnie MetRonnie left a 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.
@hjoliver
Copy link
Member Author

Might want a rebase-squash, much as a love commit messages like "Fix screwed conflict resolution".

Squashed. Although I do genuinely love commit messages like that.

@hjoliver
Copy link
Member Author

If the tests pass, this bad boy is going in.

@hjoliver
Copy link
Member Author

Phew 😌

@hjoliver hjoliver merged commit d5788a7 into cylc:master Mar 13, 2024
36 of 38 checks passed
@hjoliver hjoliver deleted the cylc-set-task branch March 13, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment