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

vr: plugin options not respected when workflow is running #5968

Closed
oliver-sanders opened this issue Feb 14, 2024 · 5 comments · Fixed by #5996
Closed

vr: plugin options not respected when workflow is running #5968

oliver-sanders opened this issue Feb 14, 2024 · 5 comments · Fixed by #5996
Assignees
Labels
Milestone

Comments

@oliver-sanders
Copy link
Member

The -S option does not take effect with the cylc vr command in the situation where the workflow is already running (i.e. the reload case).

Reproducible Example:

#!Jinja2

[scheduling]
    [[graph]]
        R1 = a

[runtime]
    [[a]]
        script = echo {{ VAR }}
$ cylc vip --pause -S VAR=1
$ cylc vr <id> -S VAR=2
$ cd ~/cylc-run/<id>

VAR is updated in the Rose files:

$ cat opt/rose-suite-cylc-install.conf
...
[template variables]
VAR=2

But not in the Cylc files:

$ cat log/config/02-reload-01.cylc
...
[runtime]
    [[a]]
        script = echo 1
$ cat log/config/flow-processed.cylc
...
[runtime]
    [[a]]
        script = echo 1

Example derived from: https://cylc.discourse.group/t/jinja2-variable-differences-in-config-files-new-ones-are-not-being-respected/885/6

How it's supposed to work:

  • The -S option is provided by cylc-rose and applies to (re)install.
  • When you provide the -S option, it writes the input to the cylc-install optional config (~/cylc-run/<workflow>/opt/rose-suite-cylc-install.conf).
  • When the workflow is reloaded, the cylc-rose plugin is automatically invoked.
  • It reads these inputs from the cylc-install optional config and makes them available for the parsing of the workflow configuration.

Ongoing Work

Note, there have been recent changes (unreleased) to the reinstall code and there are pending changes to the way the plugins are run: #5868

Suggest reproducing and fixing on top of that branch.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 14, 2024

The root cause of this bug is that the CLI options are held in memory, they are subsequently re-used to re-load the workflow config:

return WorkflowConfig(
self.workflow,
self.flow_file,
self.options,
self.template_vars,
xtrigger_mgr=self.xtrigger_mgr,
mem_log_func=self.profiler.log_memory,
output_fname=os.path.join(
self.workflow_run_dir, 'log', 'config',
workflow_files.WorkflowFiles.FLOW_FILE_PROCESSED
),
run_dir=self.workflow_run_dir,
log_dir=self.workflow_log_dir,
work_dir=self.workflow_work_dir,
share_dir=self.workflow_share_dir,
)

Inspecting the options at this point will reveal the previous value:

opts.rose_template_vars
['VAR=1', 'ROSE_ORIG_HOST=vld601.cmpd1.metoffice.gov.uk']

This makes it look like the plugin as if -S VAR=1 had been specified on the command line which overrides the VAR=2 value present in the optional configuration.

This diff fixes the problem:

diff --git a/cylc/flow/scheduler.py b/cylc/flow/scheduler.py
index ec3ba6f0c..f6bda8ca6 100644
--- a/cylc/flow/scheduler.py
+++ b/cylc/flow/scheduler.py
@@ -1269,6 +1269,7 @@ class Scheduler:
     def load_flow_file(self, is_reload=False):
         """Load, and log the workflow definition."""
         # Local workflow environment set therein.
+        self.options.rose_template_vars = []
         return WorkflowConfig(
             self.workflow,
             self.flow_file,

But it's a bit ugly!

Not sure what nicer options might be available, things to consider:

  • Do any other cylc-rose options need clearing?
  • Do any cylc-rose options need to be preserved, e.g. for auto-restart?

@wxtim wxtim self-assigned this Feb 20, 2024
@wxtim
Copy link
Member

wxtim commented Feb 22, 2024

  • Do any other cylc-rose options need clearing?

I'd guess that -D should be checked, although I suspect that from Cylc's point of view it might well be stored in the same place as -S & Should be fixed by this change.

Worth checking, but logically -O shouldn't be a problem, since that works entirely in the plugin layer, and isn't really used by Cylc.

  • Do any cylc-rose options need to be preserved, e.g. for auto-restart?

I should check whether -O is preserved on restart. It ought to be.

@oliver-sanders
Copy link
Member Author

I should check whether -O is preserved on restart. It ought to be.

This will be tested in cylc-rose.

@wxtim
Copy link
Member

wxtim commented Feb 22, 2024

Have checked. Definately need to do the same with -D

@oliver-sanders
Copy link
Member Author

Closed by #5996 and cylc-rose changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants