Fixed 3 related parsec/template var issues #4462
Conversation
cylc/flow/parsec/fileparse.py
Outdated
| >>> merge_template_vars(a, b) | ||
| {'FOO': 42, 'BAR': 'Hello World', 'ROSE_SUITE_VARIABLES': {}} | ||
|
|
||
| Templating hasn't been detected by a plugin, so we don't do anything: |
There was a problem hiding this comment.
This seems like an obvs test case I'd previously missed.
cylc/flow/parsec/fileparse.py
Outdated
| """ | ||
| fpath = Path(fpath) |
There was a problem hiding this comment.
Not strictly necessary, but I like pathlib, and feel that this is safer in the long run.
cylc/flow/parsec/fileparse.py
Outdated
| if plugin_result['templating_detected'] is not None: | ||
| if ( | ||
| plugin_result['templating_detected'] is not None | ||
| and 'ROSE_SUITE_VARIABLES' not in native_tvars |
There was a problem hiding this comment.
Is this not a bug in cylc-rose?
There was a problem hiding this comment.
I'm not sure why I thought otherwise, although there may be a reason - Will investigate
There was a problem hiding this comment.
I think that this needs to be here because the plugin is not given any knowledge of template variables already in Cylc, so the plugin knows not whether it has been run before.
There was a problem hiding this comment.
Hmmm, had a look at cylc-rose, couldn't find any way for it to set ROSE_SUITE_VARIABLES twice for one pre_configure call.
Jammed some print statements into cylc-flow, seems the pre_configure plugins are being called twice, once in cylc.flow.parsec.fileparse and once in cylc.flow.templatevars which means we would hit this issue with any plugin that sets templatevars.
I'm not sure why we do this in two places, perhaps there's some odd functionality relying on it being done in templatevars?
There was a problem hiding this comment.
Jammed some print statements into cylc-flow, seems the
pre_configureplugins are being called twice, once incylc.flow.parsec.fileparseand once incylc.flow.templatevarswhich means we would hit this issue with any plugin that sets templatevars.
Sorry, I could have told you this without you having to investigate.
I think that I made this happen when I enabled Cylc (conf/view/list/..) work on uninstalled workflows at #4293 : The correct answer is probably to differentiate between installed and uninstalled workflows in the scripts, and only call it in the script if the workflow isn't installed.
What do you think?
There was a problem hiding this comment.
I've converted this PR back into a draft until I've actually fixed the main problem.
wxtim
force-pushed
the
parsec.run.twice.bug
branch
from
3f4bf93
to
7740c46
Compare
This is a small change with no associated Issue.
Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.