clear cylc options at reload

[wxtim]

Closes #5968 and #6058 (Which was an accidental by product of #5996

I've tried to include as much info as possible here, to prevent reviewers having to re-hash the investigation or handle a web of issues and PRs for what turns out to be a simple problem with complex ramifications.

Full statement of the problem.

The original problem (#5968 ) was that on reload CLI rose options (-S, -D and -O) are already stored on the scheduler, and because reload doesn't take those arguments they do not get updated. In Cylc VR the user can update the stored copy, but the reload cannot override the variables stored on the scheduler.

The first fix (#5996) cleared -S and -D, but not -O which led to inconsistencies on reload, and an apparent change in priority between variables set in different manner.

Testing

This issue is a complex interaction between Cylc and the Cylc Rose plugin. Unfortunately the plugin is not as stand-alone as I would like. I think that there are 3 possible ways of testing this change:

1. (Test -S/--rose-template-variable cylc-rose#310) Functional and End-to-End testing in Cylc Rose. This allows me to demonstrate the actual cases where this is a problem, but is ugly because Cylc Rose does not currently have infrastructure equivalent to Cylc for creating schedulers.
2. Functional testing in Cylc - Cylc has all the infrastructure to carry out these tests neatly, except for a requirement for Cylc Rose to be present. I chose not to because I didn't want to add that requirement, not even for testing.
3. Integration testing in Cylc Rose - Chose not to do this because I'd have to either copy a big pile of infrastructure from Cylc, which would then be something we would have to maintain, or spin the Cylc Test utilities into a new repo or do something strange and complex to make the tests available from Cylc flow if it's an editable install...
4. Unit testing in Cylc: I chose not to do this because it doesn't help document why this change is being made, and ends up mocking so much of what's going on it seems nearly pointless.

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 in a companion cylc rose PR Test -S/--rose-template-variable cylc-rose#310
• 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.

[oliver-sanders]

@wxtim, tagged as 8.3.0 since this was raised on master.

[wxtim]

@wxtim, tagged as 8.3.0 since this was raised on master.

#5968 was raised on 8.2.x - The problem reported recently was caused by a bugfix made to 8.2.x, which was then ported to master, so I think it should be against 8.2.x

[oliver-sanders]

This explanation doesn't make much sense to me as the play command used to start the workflow in the first place can also have template variables attached?

[wxtim]

If you do

cylc play x --pause -S 'foo=1'
cylc stop x
cylc reinstall x -S foo=2
cylc play x -S 'foo=3'

Do you want foo=1 or 2 or 3?

This is also safe against

cylc play x --pause -S 'foo=1'
cylc stop x
cylc reinstall x -S foo=2
cylc play x

because the restart involves re-reading the rose config files too.

[oliver-sanders]

so I think it should be against 8.2.x

Ok, you'll need to change the base branch to 8.2.x.

Remember to check the branch when ticking the:

[ ] If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Line in the description.

[MetRonnie]

While you're here, I think it would be worth improving the description for -S. I think the current description is confusing?

  -S ROSE_TEMPLATE_VARS, --rose-template-variable=ROSE_TEMPLATE_VARS, --define-suite=ROSE_TEMPLATE_VARS
                        As `--define`, but with an implicit `[SECTION]` for
                        workflow variables.

[MetRonnie]

I'm a bit confused

• If the user has used cylc vr running-workflow -S (reinstall + reload), how are the changed values getting through to the scheduler then?
• What do you mean by "reload on restart"? If the user has used cylc vr stopped-workflow -S (reinstall + restart), then there is no reload, right?

[wxtim]

If the user has used cylc vr running-workflow -S (reinstall + reload), how are the changed values getting through to the scheduler then?

The reinstall updates the rose-suite-cylc-install.conf which the reload reads. The reload doesn't have the -S option so clearing that here only affects options set on the scheduler by an earlier Cylc play.

Your question has however revealed a horrid case where this doesn't work:

cylc vip -S 'VAR=1' --pause
cylc vr bugs/5968

Do you think that the correct options might be to allow Cylc Reload to set -S and it's friends, becuase to me that looks like the simplest way of doing things?

What do you mean by "reload on restart"? If the user has used cylc vr stopped-workflow -S (reinstall + restart), then there is no reload, right?

Oliver described it as an implicit reload.

[MetRonnie]

Your question has however revealed a horrid case where this doesn't work

What does happen with this example, and what is meant to happen?

[wxtim]

After much thought, and an offline discussion with @oliver-sanders I'm closing this: We now think that the correct solution to this problem is rather different.