Stem: Make defines contain `:suite.rc` #248

wxtim · 2023-08-22T14:32:25Z

Closes #246

Ensure that a :suite.rc is added to the template language when creating defines.

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).
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.
This is a bug fix but there are no features on master requiring a minor release?

oliver-sanders

Currently the code breaks if [jinja2:suite.rc] or [empy:suite.rc] are specified in the rose-suite.conf file, but works if [template vars] is used instead.

These changes switch this the other way around so that [jinja2:suite.rc] and [empy:suite.rc] work but [template vars] is broken.

MetRonnie

LGTM, but haven't tested

cylc/rose/stem.py

oliver-sanders

The original error can still be reproduced on this branch:

$ rose stem --source=mysource=$PWD -S QUESTION='"7 x 6"'

$ cat runN/rose-suite.conf && echo -e '\n\n-----------------\n\n' && cat runN/opt/rose-suite-cylc-install.conf
ROSE_STEM_VERSION=1
# Config Options ' (cylc-install)' from CLI appended to options already in `rose-suite.conf`.
opts=(cylc-install)

[jinja2:suite.rc]
ANSWER=42


-----------------


# This file records CLI Options.

!opts=

[env]
# ROSE_ORIG_HOST set by cylc install.
ROSE_ORIG_HOST=<host>

[jinja2]
...
SITE="meto"
SOURCE_MYSOURCE_MIRROR=""
SOURCE_MYSOURCE_REV=""

[jinja2:suite.rc]
QUESTION="7 x 6"
# ROSE_ORIG_HOST set by cylc install.
ROSE_ORIG_HOST=<host>

wxtim · 2023-09-27T10:16:37Z

@oliver-sanders - I'm terribly sorry that I asked you to review something which was very clearly broken.

cylc/rose/utilities.py

tests/unit/test_config_node.py

MetRonnie

Code looks good, still haven't tested

oliver-sanders

Tested. Got conflicts. Please squash.

…ting defines.

wxtim · 2023-10-09T08:28:23Z

I think this requires a 5 minute check from @MetRonnie or @oliver-sanders that I haven't messed up the rebase, then merge.

MetRonnie · 2023-10-09T09:41:16Z

Rebase is fine. I'm unclear on how to test this. I haven't understood the example from the original issue.

oliver-sanders · 2023-10-09T11:51:23Z

set -eu                                          
                                                 
# create an example workflow                     
cd $(mktemp -d)                                  
mkdir rose-stem                                  
cat > rose-stem/rose-suite.conf <<__HERE__       
ROSE_STEM_VERSION=1                              
                                                 
[jinja2:suite.rc]                                                                                                                  
ANSWER=42            
__HERE__             
touch rose-stem/flow.cylc    
                     
# install it         
rose stem --source=mysource=$PWD --workflow-name=foo    
                     
# inspect the installed config    
echo -e '\n\n# rose-suite.conf'    
cat ~/cylc-run/foo/runN/rose-suite.conf    
echo -e '\n\n# cylc-install opt conf'    
cat ~/cylc-run/foo/runN/opt/*

Would produce:

# rose-suite.conf
ROSE_STEM_VERSION=1
# Config Options ' (cylc-install)' from CLI appended to options already in `rose-suite.conf`.
opts=(cylc-install)

[jinja2:suite.rc]
ANSWER=42


# cylc-install opt conf
# This file records CLI Options.

!opts=

[env]
# ROSE_ORIG_HOST set by cylc install.
ROSE_ORIG_HOST=vld601.cmpd1.metoffice.gov.uk

[jinja2]
HOST_SOURCE_MYSOURCE="vld601:/var/tmp/tmp.AImdjdV0Zm"
HOST_SOURCE_MYSOURCE_BASE="vld601:/var/tmp/tmp.AImdjdV0Zm"
SITE="meto"
SOURCE_MYSOURCE="/var/tmp/tmp.AImdjdV0Zm"
SOURCE_MYSOURCE_BASE="/var/tmp/tmp.AImdjdV0Zm"
SOURCE_MYSOURCE_MIRROR=""
SOURCE_MYSOURCE_REV=""

[jinja2:suite.rc]
# ROSE_ORIG_HOST set by cylc install.
ROSE_ORIG_HOST=vld601.cmpd1.metoffice.gov.uk

Note, the correct [jinja2:suite.rc] and incorrect [jinja2] sections.

To test, try this with each of:

template variables
jinja2:suite.rc
empy:suite.rc

And ensure that the resulting sections match the input name.

MetRonnie

Ok, think I've got it. With this branch, if I have in rose-suite.conf

[jinja2:suite.rc]
ANSWER=42

then in opt/rose-suite-cylc-install.conf I get everything in [jinja2:suite.rc], (unlike on 1.3.x where I get some things in [jinja2]).

And if I have

[template variables]
ANSWER=42

then in opt/rose-suite-cylc-install.conf I get everything in [template variables]

wxtim · 2023-10-09T13:09:44Z

Any reason not to hit the Big Green Button™

wxtim force-pushed the fix.template_section_name_for_defines branch from fd2d3b0 to 6e57e1c Compare August 22, 2023 14:42

wxtim self-assigned this Aug 22, 2023

wxtim requested review from oliver-sanders and MetRonnie August 22, 2023 14:43

wxtim added bug Something isn't working small labels Aug 22, 2023

wxtim changed the base branch from master to 1.3.x August 22, 2023 14:44

wxtim force-pushed the fix.template_section_name_for_defines branch from 6e57e1c to b539443 Compare August 22, 2023 14:46

oliver-sanders added this to the 1.3.1 milestone Aug 23, 2023

MetRonnie approved these changes Aug 23, 2023

View reviewed changes

oliver-sanders requested changes Aug 24, 2023

View reviewed changes

wxtim requested a review from oliver-sanders September 11, 2023 12:23

MetRonnie approved these changes Sep 11, 2023

View reviewed changes

oliver-sanders reviewed Sep 12, 2023

View reviewed changes

cylc/rose/stem.py Outdated Show resolved Hide resolved

wxtim requested a review from oliver-sanders September 21, 2023 09:19

oliver-sanders requested changes Sep 22, 2023

View reviewed changes

wxtim requested a review from oliver-sanders September 27, 2023 10:16

MetRonnie reviewed Oct 2, 2023

View reviewed changes

cylc/rose/utilities.py Outdated Show resolved Hide resolved

cylc/rose/utilities.py Outdated Show resolved Hide resolved

tests/unit/test_config_node.py Show resolved Hide resolved

MetRonnie approved these changes Oct 4, 2023

View reviewed changes

oliver-sanders approved these changes Oct 5, 2023

View reviewed changes

wxtim force-pushed the fix.template_section_name_for_defines branch from 24d5024 to 54b0696 Compare October 9, 2023 08:26

Ensure that a :suite.rc is added to the template language when crea…

98b3129

…ting defines.

wxtim force-pushed the fix.template_section_name_for_defines branch from 54b0696 to 98b3129 Compare October 9, 2023 08:27

oliver-sanders requested a review from MetRonnie October 9, 2023 11:52

MetRonnie approved these changes Oct 9, 2023

View reviewed changes

oliver-sanders merged commit 6ca48b2 into cylc:1.3.x Oct 9, 2023
3 checks passed

wxtim deleted the fix.template_section_name_for_defines branch October 9, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stem: Make defines contain `:suite.rc` #248

Stem: Make defines contain `:suite.rc` #248

wxtim commented Aug 22, 2023 •

edited

Loading

oliver-sanders left a comment

MetRonnie left a comment

oliver-sanders left a comment

wxtim commented Sep 27, 2023

MetRonnie left a comment

oliver-sanders left a comment •

edited

Loading

wxtim commented Oct 9, 2023

MetRonnie commented Oct 9, 2023

oliver-sanders commented Oct 9, 2023 •

edited

Loading

MetRonnie left a comment

wxtim commented Oct 9, 2023

Stem: Make defines contain :suite.rc #248

Stem: Make defines contain :suite.rc #248

Conversation

wxtim commented Aug 22, 2023 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

wxtim commented Sep 27, 2023

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

wxtim commented Oct 9, 2023

MetRonnie commented Oct 9, 2023

oliver-sanders commented Oct 9, 2023 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

wxtim commented Oct 9, 2023

Stem: Make defines contain `:suite.rc` #248

Stem: Make defines contain `:suite.rc` #248

wxtim commented Aug 22, 2023 •

edited

Loading

oliver-sanders left a comment •

edited

Loading

oliver-sanders commented Oct 9, 2023 •

edited

Loading