vr: restart the workflow with no changes according to user prompt #6354

oliver-sanders · 2024-09-03T17:35:41Z

Closes cylc vr - restart with no changes? #6261
If there are no changes to reinstall AND the workflow is stopped, rather than just exiting, the cylc vr command will now prompt the user to give them the option of restarting the workflow.
This makes them aware there are no changes to reinstall (which may be an error on their part) but still gives them the option of restart without having to edit or rerun the command.

An odd feature to introduce at a bugfix release, however, it's kinda a UX type issue so it might make sense.

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.

hjoliver · 2024-09-12T08:04:31Z

(Looks good, but I haven't done a proper review yet).

oliver-sanders · 2024-09-30T15:57:13Z

(rebased)

* Closes cylc#6261 * If there are no changes to reinstall AND the workflow is stopped, prompt the user to see whether they want to restart it anyway. * This makes `cylc vr` more useful as the "I want to restart my workflow" command. * But it also ensures that they are aware if no changes are present as they might have forgotten to press save or run the command on the wrong workflow or whatever.

oliver-sanders · 2024-10-22T08:24:49Z

(rebased)

changes.d/6354.feat.md

tests/integration/scripts/test_validate_reinstall.py

Co-authored-by: Tim Pillinger <[email protected]>

cylc/flow/scripts/validate_reinstall.py

wxtim

Alles Gut!

hjoliver

I'm not sure the logic is quite right here.

hjoliver · 2024-11-05T00:20:49Z

cylc/flow/scripts/validate_reinstall.py

+            # the are no changes to install and the workflow is running
+            # => there is nothing for us to do here
+            LOG.warning(
+                f'{NO_CHANGES_STR}: No reinstall or'


Suggested change

f'{NO_CHANGES_STR}: No reinstall or'

f'{NO_CHANGES_STR}: no'

The STR already says that no reinstall is needed.

hjoliver · 2024-11-05T00:36:46Z

no changes, workflow running

$  cylc vr twt 
...
WARNING - No changes to reinstall: No reinstall or reload required. ✔️

good: warns there are no changes, does nothing

$ cylc vr twt --yes
...
# no warning that there are no changes to reinstall ❌ 
Successfully reinstalled.  ❌

use of --yes should not make any difference here as there is no prompt to respond to
there are no changes, so reinstalling and reloading is pointless

no changes, workflow stopped

$ cylc vr twt
...
WARNING - No changes to reinstall ✔️ 
Restart anyway? [y/n]  ✔️

good: warns there are no changes to reinstall, asks if we want to restart anyway

$ cylc vr twt --yes
...
Successfully reinstalled. ❌ 
$ cylc play twt ✔️

we should warn there are no changes, and not do a pointless reinstall
(--yes is for the "restart anyway" prompt)

oliver-sanders · 2024-11-06T11:06:39Z

I think that behaviour is as intended (and mostly as before), the --yes argument is not just for the restart-anyway prompt, it also behaves like --force.

I'm guessing you would rather have two separate options for these?

Either way, bumping.

hjoliver · 2024-11-06T22:11:28Z

I think that behaviour is as intended (and mostly as before), the --yes argument is not just for the restart-anyway prompt, it also behaves like --force.

Command help only says: --yes [reinstall] Skip interactive prompts.
I'm not aware of any precedent for --yes behaving like --force, so that's a bit surprising
What's the point of supporting forced reinstall when we know there are no changes to update? It can't make any difference.

I'm guessing you would rather have two separate options for these?

Not unless you have a good argument for 3. above - is there any need for forced reinstall of no changes? If there is a need, then yes we should have a separate --force option.

BTW, --force could possibly be taken to imply --yes but not vice versa IMO.

oliver-sanders · 2024-11-21T10:04:46Z

What's the point of supporting forced reinstall when we know there are no changes to update? It can't make any difference.

The main use case would be forcing Rose file-installation to be reevaluated. This may cause newer versions of external dependencies to be brought into the workflow.

However, it's also just an easy way to achieve #6261

hjoliver · 2024-11-25T21:56:34Z

However, it's also just an easy way to achieve #6261

(Which is: "cylc vr: restart with no changes")

Not entirely sure I got my point across. I wasn't saying to not reinstall and not to restart if there are no changes, just not to reinstall.

The main use case would be forcing Rose file-installation to be reevaluated.

If that's true, fine, but:

the command should still generate the same no-source-changes-detected message whether or not a forced reinstall is done.
the option should be --force, not --yes which doesn't make sense when there are not prompts to answer to

oliver-sanders · 2024-11-26T10:36:46Z

Ok, can I transfer this PR over to you to implement your desired behavior (might be easier than trying to explain).

FYI: I don't have an issue with --yes covering --force as I doubt users would every want one and not the other, but happy to leave this to your judgement.

Test harness is already in place so it's trivial work to fiddle CLI options.

hjoliver · 2024-11-26T22:44:18Z

Fair enough, I'll tweak the options. Assigning myself...

oliver-sanders added small could be better Not exactly a bug, but not ideal. labels Sep 3, 2024

oliver-sanders added this to the 8.3.4 milestone Sep 3, 2024

oliver-sanders self-assigned this Sep 3, 2024

oliver-sanders force-pushed the 6261 branch from b50f4e2 to 8021ce9 Compare September 4, 2024 09:10

oliver-sanders marked this pull request as ready for review September 4, 2024 09:29

oliver-sanders requested a review from hjoliver September 11, 2024 13:21

oliver-sanders modified the milestones: 8.3.4, 8.3.5 Sep 11, 2024

oliver-sanders changed the base branch from master to 8.3.x September 30, 2024 15:56

oliver-sanders force-pushed the 6261 branch from 8021ce9 to c3230e9 Compare September 30, 2024 15:56

oliver-sanders modified the milestones: 8.3.5, 8.3.6 Oct 14, 2024

oliver-sanders requested a review from wxtim October 17, 2024 08:36

oliver-sanders force-pushed the 6261 branch from c3230e9 to 5af333d Compare October 22, 2024 08:24

wxtim reviewed Oct 23, 2024

View reviewed changes

changes.d/6354.feat.md Outdated Show resolved Hide resolved

wxtim reviewed Oct 23, 2024

View reviewed changes

tests/integration/scripts/test_validate_reinstall.py Outdated Show resolved Hide resolved

wxtim reviewed Oct 23, 2024

View reviewed changes

tests/integration/scripts/test_validate_reinstall.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Apply suggestions from code review

2505de9

Co-authored-by: Tim Pillinger <[email protected]>

oliver-sanders force-pushed the 6261 branch from e87b179 to 2505de9 Compare October 23, 2024 14:02

oliver-sanders requested a review from wxtim October 23, 2024 14:03

wxtim reviewed Oct 24, 2024

View reviewed changes

cylc/flow/scripts/validate_reinstall.py Show resolved Hide resolved

vr: test reinstallation abort

397e656

oliver-sanders requested a review from wxtim October 25, 2024 10:42

wxtim approved these changes Oct 28, 2024

View reviewed changes

hjoliver requested changes Nov 5, 2024

View reviewed changes

oliver-sanders modified the milestones: 8.3.6, 8.3.7 Nov 6, 2024

hjoliver self-assigned this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vr: restart the workflow with no changes according to user prompt #6354

vr: restart the workflow with no changes according to user prompt #6354

oliver-sanders commented Sep 3, 2024 •

edited

Loading

hjoliver commented Sep 12, 2024

oliver-sanders commented Sep 30, 2024

oliver-sanders commented Oct 22, 2024

This comment was marked as resolved.

wxtim left a comment

hjoliver left a comment

hjoliver Nov 5, 2024

hjoliver commented Nov 5, 2024 •

edited

Loading

oliver-sanders commented Nov 6, 2024

hjoliver commented Nov 6, 2024 •

edited

Loading

oliver-sanders commented Nov 21, 2024

hjoliver commented Nov 25, 2024 •

edited

Loading

oliver-sanders commented Nov 26, 2024 •

edited

Loading

hjoliver commented Nov 26, 2024

vr: restart the workflow with no changes according to user prompt #6354

Are you sure you want to change the base?

vr: restart the workflow with no changes according to user prompt #6354

Conversation

oliver-sanders commented Sep 3, 2024 • edited Loading

hjoliver commented Sep 12, 2024

oliver-sanders commented Sep 30, 2024

oliver-sanders commented Oct 22, 2024

This comment was marked as resolved.

wxtim left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Nov 5, 2024

Choose a reason for hiding this comment

hjoliver commented Nov 5, 2024 • edited Loading

no changes, workflow running

no changes, workflow stopped

oliver-sanders commented Nov 6, 2024

hjoliver commented Nov 6, 2024 • edited Loading

oliver-sanders commented Nov 21, 2024

hjoliver commented Nov 25, 2024 • edited Loading

oliver-sanders commented Nov 26, 2024 • edited Loading

hjoliver commented Nov 26, 2024

oliver-sanders commented Sep 3, 2024 •

edited

Loading

hjoliver commented Nov 5, 2024 •

edited

Loading

hjoliver commented Nov 6, 2024 •

edited

Loading

hjoliver commented Nov 25, 2024 •

edited

Loading

oliver-sanders commented Nov 26, 2024 •

edited

Loading