Skip to content

services: validate-reinstall#746

Open
oliver-sanders wants to merge 3 commits into
cylc:masterfrom
oliver-sanders:526
Open

services: validate-reinstall#746
oliver-sanders wants to merge 3 commits into
cylc:masterfrom
oliver-sanders:526

Conversation

@oliver-sanders
Copy link
Copy Markdown
Member

@oliver-sanders oliver-sanders commented Oct 29, 2025

Closes #526

I somehow forgot that we have cylc vr support in the Tui, but not the GUI!

Trivial to add support. Supporting the cylc install command is also possible, however, that will require #512.

UI sibling: cylc/cylc-ui#2347

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.

args: Dict[str, Any],
workflows_mgr: 'WorkflowsManager',
log: 'Logger',
) -> List[Union[bool, str]]:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed from list[bool | str] to tuple[bool, str] as this is safer for the way we are trying to use it (('msg', False) would have been acceptable before).

Comment on lines +356 to +364
try:
ret_code = proc.wait(timeout=timeout)
except TimeoutExpired as exc:
proc.kill()
ret_code = 124 # mimic `timeout` command error code
# NOTE: preserve any stderr that the command produced this
# far as this may help with debugging
out, err = proc.communicate()
err = str(exc) + (('\n' + err) if err else '')
Copy link
Copy Markdown
Member Author

@oliver-sanders oliver-sanders Oct 30, 2025

Choose a reason for hiding this comment

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

So, it turns out this timeout logic was bunk. You have to actually .kill() to process if you want it to stop.

https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate

Additionally, this will now log the command's stderr which might be useful in the event of a timeout.


if command == 'clean': # noqa: SIM116
return await Services.clean(
self.workflows_mgr,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switched arg order for consistency.

@oliver-sanders
Copy link
Copy Markdown
Member Author

FYI: style tests will fail until #748

@oliver-sanders oliver-sanders marked this pull request as ready for review October 30, 2025 16:03
@oliver-sanders oliver-sanders requested a review from wxtim October 30, 2025 16:03
Comment thread cylc/uiserver/schema.py Outdated
Copy link
Copy Markdown
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Read Code
  • Tried in practice.

n.b. I think that VR should be on the shortened list of commands, but that's a Cylc UI change

@oliver-sanders
Copy link
Copy Markdown
Member Author

oliver-sanders commented Nov 3, 2025

n.b. I think that VR should be on the shortened list of commands, but that's a Cylc UI change

There'll be a cylc-ui and cylc-doc counterpart.

@oliver-sanders oliver-sanders force-pushed the 526 branch 2 times, most recently from f8f0f51 to f6a6e5e Compare November 3, 2025 16:31
@oliver-sanders
Copy link
Copy Markdown
Member Author

@wxtim, since last review:

  • Fixed tests (hopefully)
  • Added support for the "global" option (BoM might want to use this)

oliver-sanders added a commit to oliver-sanders/cylc-ui that referenced this pull request Nov 3, 2025
* The `reload` mutation isn't the most helpful command post
  `cylc install`.
* Swap it out in the workflow command shortlist for the
  [newly added](cylc/cylc-uiserver#746)
  `validateReinstall` mutation which has wider scope.
Comment thread cylc/uiserver/resolvers.py
Comment thread cylc/uiserver/schema.py
Comment thread cylc/uiserver/schema.py Outdated
Comment thread cylc/uiserver/tests/test_resolvers.py Outdated
@oliver-sanders
Copy link
Copy Markdown
Member Author

(drafting whilst I look at how hard it will be to combine ALL play and reload opts)

@oliver-sanders oliver-sanders marked this pull request as draft November 5, 2025 13:00
* Closes cylc#526
* Add basic support for the `cylc vr` command.
* We were applying a timeout to the `Popen.wait` method, however, this
  doesn't actually kill the process (as one might expect) when the
  timeout elapses, it just stops waiting for it?!
Copy link
Copy Markdown
Member Author

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

This is now the "full" solution which should support all relevant usages/args.

Reviewers, please give attention to the newly-added arguments to ensure they are working as intended:

https://github.com/cylc/cylc-uiserver/pull/746/changes#r2833158708

Comment thread cylc/uiserver/schema.py
Set the Cylc version that the workflow starts with.
''')
)
# cylc-rose
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Cylc-rose arguments (all new):

  • opt_conf_key
  • clear_rose_install_options
  • define
  • rose_template_variable

Comment thread cylc/uiserver/schema.py
''')
resolver = partial(mutator, command='validate_reinstall')

class Arguments(Reinstall.Arguments, Reload.Arguments):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reload arguments (all new):

  • reload_global

@oliver-sanders oliver-sanders force-pushed the 526 branch 2 times, most recently from e1ea902 to 2dbceab Compare February 20, 2026 13:33
@oliver-sanders oliver-sanders marked this pull request as ready for review February 20, 2026 13:43
oliver-sanders added a commit to oliver-sanders/cylc-ui that referenced this pull request Feb 20, 2026
* The `reload` mutation isn't the most helpful command post
  `cylc install`.
* Swap it out in the workflow command shortlist for the
  [newly added](cylc/cylc-uiserver#746)
  `validateReinstall` mutation which has wider scope.
Comment thread cylc/uiserver/resolvers.py Outdated
Comment thread cylc/uiserver/resolvers.py Outdated
Comment thread cylc/uiserver/schema.py
Comment thread cylc/uiserver/schema.py Outdated
Copy link
Copy Markdown
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 tried adding opt conf keys in the command form but they were not passed to the cylc reinstall command

Comment thread cylc/uiserver/schema.py
Comment on lines +248 to +252
cylc_version = CylcVersion(
description=sstrip('''
Set the Cylc version that the workflow starts with.
''')
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this applies to reinstall, surely?

Suggested change
cylc_version = CylcVersion(
description=sstrip('''
Set the Cylc version that the workflow starts with.
''')
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is implemented for all commands, see def run_command.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But why? I can't see the need to do this for reinstall?

Copy link
Copy Markdown
Member Author

@oliver-sanders oliver-sanders Feb 25, 2026

Choose a reason for hiding this comment

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

It's not just for reinstall (which isn't actually implemented), the cylc vr command will also (re)start a stopped workflow, so the CYLC_VERSION argument is just as important here as for the Play mutation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But vr/ReinstallReload inherits Play's options which includes cylc_version

Comment thread cylc/uiserver/schema.py Outdated
Comment thread cylc/uiserver/schema.py Outdated
Comment thread cylc/uiserver/schema.py Outdated
Comment thread cylc/uiserver/schema.py Outdated
@oliver-sanders
Copy link
Copy Markdown
Member Author

oliver-sanders commented Feb 25, 2026

https://github.com/cylc/cylc-uiserver/compare/2dbceab899de0bdcef9b5a097c2d364dc4781ea0..57b44ec5ab8d24e46f2b811ceb11c81d81b5a474

  • Hardcoded mappings from the GraphQL (plural) to CLI (singular) fields.
  • Renamed field.
  • Applied requested docs changes.

c8523a1

  • Prefix Rose fields with rose_.

* Provide full coverage for the `cylc vr` command.
* Separate the command into two GraphQL mutations:
  * [validate-]reinstall-reload.
  * [validate-]reinstall-restart.
@oliver-sanders
Copy link
Copy Markdown
Member Author

(tests should pass now)

Copy link
Copy Markdown
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.

Still have the duplicated cylc version option (#746 (comment)) but otherwise I think this is all good

Comment thread cylc/uiserver/schema.py
class ReinstallReload(graphene.Mutation):
class Meta:
description = sstrip('''
Validate, reinstall, then reload workflows.
Copy link
Copy Markdown
Member

@MetRonnie MetRonnie Apr 23, 2026

Choose a reason for hiding this comment

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

To match how other commands are written in the schema

Suggested change
Validate, reinstall, then reload workflows.
Validate, reinstall, then reload the workflow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This mutation can operate on multiple workflows right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but singular would be consistent with all other mutations' descriptions

Comment thread cylc/uiserver/schema.py
class ReinstallRestart(graphene.Mutation):
class Meta:
description = sstrip('''
Validate, reinstall, then restart workflows.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Validate, reinstall, then restart workflows.
Validate, reinstall, then restart the workflow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This mutation can operate on multiple workflows right?

@oliver-sanders oliver-sanders modified the milestones: 1.9.0, 1.10.0 May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

services: vr (validate-reinstall-apply)

3 participants