Rose conf.read jinja2 #3913

wxtim · 2020-11-03T10:56:13Z

These changes address #3819
Summary of Changes

Added ability to use cylc.configure entry point to parse variables for Cylc in cylc/flow/parsec/fileparse.py.
Written a template cylc install which installs files listed in rose-suite.conf using cylc.install entry point.
Written functional tests (this seems pretty odd, since they will only work if the rose-cylc plugin is installed, but it's probably ok, for now)
Added CYLC_VERSION jinja2 variable (not to be confused with env var environ['CYLC_VERSION'] (which might not be set at all))

I have not raised a warning for absence of root-dir - I think that this needs to be in the plugin.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Appropriate tests are included (unit and/or functional).
- Unit test all functionality in new python code
- Functional test basic rose-suite.conf reading functionality, but cannot check the optional config reading until it's plumbed into the CLI
Appropriate change log entry included.
No documentation update required: Full documentation of all sections of the rose-suite.conf work after all done.

cylc/flow/parsec/jinja2support.py

TomekTrzeciak · 2020-11-04T07:51:41Z

Do you plan to get this working for [empy:suite.rc] too (currently it doesn't)?

Also, rose suite-run adds a few extra variables (ROSE_ORIG_HOST, ROSE_SITE, ROSE_VERSION and ROSE_SUITE_VARIABLES metomi/rose#2338).

cylc/flow/parsec/jinja2support.py

wxtim · 2020-11-04T10:02:56Z

Do you plan to get this working for [empy:suite.rc] too (currently it doesn't)?

Yes.

cylc/flow/config.py

wxtim · 2020-11-11T08:28:58Z

I've converted this back to a draft PR having decided to steam 🚂 on into dealing with empy, fileinstall and environment variables.

.github/workflows/test_fast.yml

cylc/flow/parsec/fileparse.py

cylc/flow/scripts/install.py

cylc/flow/parsec/fileparse.py

oliver-sanders · 2020-12-17T10:11:27Z

tests/functional/rose-conf/fileinstall_data/lion.py

+
+
+def lion():
+    raise TerriblePunException("This is a Lion's Main script")


wait until you hear about

count_of = 0 for i in monte_christo: count_of += 1

oliver-sanders · 2020-12-17T10:24:02Z

@TomekTrzeciak some follow on work captured in these issues otherwise this should be done.

Note: The cylc install command (replacement for rose suite-run) is not yet implemented hence this PR adds a quick dummy command which doesn't make much sense in of itself. The src/run dir separation and the rest of the install functionality will come in #4000.

oliver-sanders · 2020-12-17T10:24:37Z

Code looks good, need to go through and test again but not expecting any surprises.

TomekTrzeciak · 2020-12-17T11:47:52Z

@TomekTrzeciak some follow on work captured in these issues otherwise this should be done.

Thanks everyone for perseverance on this one, I think the final result is better for it.

One more thing: I think the suggestion to consolidate jinja2/empy code paths #3913 (comment) is still worth doing at some point, but it would require altering cylc view options. It was my mistake to add --empy option there alongside --jinja2 rather than replace it with something more generic. Since you can't have more than one templating engine on the shebang line, it makes no sense to have more than one option for it. Perhaps it's worth to capture this in a separate ticket?

oliver-sanders · 2020-12-17T12:32:32Z

Can do, tag it against 8.x.

TomekTrzeciak · 2020-12-21T12:37:46Z

What's the plan for replacement of -O command line option: #3819 (comment). Is it this PR or follow on?

oliver-sanders · 2020-12-21T12:41:25Z

Will stay as -O, not to be done in this PR (needs a plugin interface that can mutate the CLI)

Detail:

-O, -S and -D options will be implemented in cylc install as they were for rose suite-run, however,
cylc install will now record these settings in a new optional config (called cylc install).

The Rose config will be installed into the run dir and the rose config command will be able to load
this picking up all specified optional configs.

There will be a new --clear-rose-install-options option to cylc reinstall that removes this opt config
before reinstalling (equiv to removing the file manually before reinstallation).

cylc/flow/parsec/fileparse.py

tests/functional/rose-conf/01-empy.t

tests/functional/rose-conf/01-empy/processed.conf.control

tests/functional/rose-conf/04-opts-set-from-env.t

tests/functional/rose-conf/05-opts-set-from-rose-suite.conf.t

tests/functional/rose-conf/06-jinja2.thorough/flow.cylc

tests/functional/rose-conf/06-jinja2.thorough/suite.rc

tests/functional/rose-conf/08-template-engine-conflict.t

MetRonnie

LGTM assuming tests pass

oliver-sanders · 2020-12-22T10:29:46Z

Tests now passing, will have a final test ~~today~~ ~~tomorrow~~ next week :(.

oliver-sanders · 2020-12-22T10:39:44Z

Commits will need rationalising pre-merge.

stub tests for rose fileinstall added cylc install and functional test added check that hash is in flow.cylc if reqd. added rose vars to environment for scheduler.

Removed literal_eval - it's now in the Rose-plugin. Removed writing Environment vars to scheduler environment as the plugin now does this. Assertation Error -> Assertion Error. Added Oliver's through Jinja2 functional test. added the pre_install step to the install script

…empy added Oliver's suggestion about multiple plugins fail if empy/jinja2 section in suite.conf and wrong shebang line refactor fileparse logic added export XYZ to jinja2 test

make sure that template_vars is always available attempt to make template variables safe Update cylc/flow/parsec/fileparse.py Co-authored-by: Oliver Sanders <[email protected]> Apply suggestions from code review added validate step to rose conf tests re-add warning if templates vars will be over-written fromt he CLI fail horribly if multiple plugins clash added test to demonstrate that you can't conflict the templating engine in the plugin. Fixed failure to provide env vars in install pre_configure: test entry point fixes to review items

wxtim · 2020-12-22T10:53:46Z

Commits will need rationalising pre-merge.

@oliver-sanders down to 9 ok?

oliver-sanders

🥳

wxtim requested a review from oliver-sanders November 3, 2020 11:00

TomekTrzeciak reviewed Nov 3, 2020

View reviewed changes

cylc/flow/parsec/jinja2support.py Outdated Show resolved Hide resolved

wxtim force-pushed the rose-conf.read_jinja2 branch from 49c5d78 to b7df1ce Compare November 3, 2020 11:34

TomekTrzeciak reviewed Nov 4, 2020

View reviewed changes

cylc/flow/parsec/jinja2support.py Outdated Show resolved Hide resolved

wxtim force-pushed the rose-conf.read_jinja2 branch 3 times, most recently from a89ea8d to f37b424 Compare November 6, 2020 07:48

wxtim requested a review from datamel November 6, 2020 11:19

wxtim self-assigned this Nov 6, 2020

wxtim force-pushed the rose-conf.read_jinja2 branch 2 times, most recently from 42e834f to 699343e Compare November 10, 2020 08:22

TomekTrzeciak reviewed Nov 10, 2020

View reviewed changes

cylc/flow/config.py Outdated Show resolved Hide resolved

wxtim force-pushed the rose-conf.read_jinja2 branch 2 times, most recently from fafcee2 to 84703f8 Compare November 10, 2020 14:52

wxtim marked this pull request as draft November 11, 2020 08:25

wxtim force-pushed the rose-conf.read_jinja2 branch from ec3596d to 4e15e5e Compare November 11, 2020 08:27

wxtim force-pushed the rose-conf.read_jinja2 branch 2 times, most recently from 771c256 to 0aef19d Compare November 13, 2020 14:03

wxtim commented Nov 17, 2020

View reviewed changes

.github/workflows/test_fast.yml Outdated Show resolved Hide resolved

wxtim commented Nov 17, 2020

View reviewed changes

cylc/flow/parsec/fileparse.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Nov 17, 2020

View reviewed changes

cylc/flow/scripts/install.py Outdated Show resolved Hide resolved

wxtim requested a review from oliver-sanders November 17, 2020 16:30

wxtim marked this pull request as ready for review November 17, 2020 16:30

wxtim force-pushed the rose-conf.read_jinja2 branch 2 times, most recently from 5f443ab to 58fb759 Compare November 18, 2020 16:42

TomekTrzeciak reviewed Nov 18, 2020

View reviewed changes

cylc/flow/parsec/fileparse.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Nov 24, 2020

View reviewed changes

cylc/flow/parsec/fileparse.py Outdated Show resolved Hide resolved

wxtim requested a review from oliver-sanders December 17, 2020 09:44

oliver-sanders reviewed Dec 17, 2020

View reviewed changes

wxtim requested review from hjoliver and oliver-sanders December 17, 2020 11:23

MetRonnie requested changes Dec 21, 2020

View reviewed changes

wxtim requested a review from MetRonnie December 21, 2020 23:18

MetRonnie approved these changes Dec 22, 2020

View reviewed changes

wxtim added 4 commits December 22, 2020 10:49

added cylc install and functional test

acefe82

stub tests for rose fileinstall added cylc install and functional test added check that hash is in flow.cylc if reqd. added rose vars to environment for scheduler.

changed to templating and removing explicit refereneces to jinja and …

64e2f9e

…empy added Oliver's suggestion about multiple plugins fail if empy/jinja2 section in suite.conf and wrong shebang line refactor fileparse logic added export XYZ to jinja2 test

wxtim force-pushed the rose-conf.read_jinja2 branch from ff33ce1 to 955eca2 Compare December 22, 2020 10:51

Merge branch 'master' into rose-conf.read_jinja2

9f9e898

oliver-sanders approved these changes Dec 30, 2020

View reviewed changes

oliver-sanders removed request for hjoliver and datamel December 30, 2020 09:23

oliver-sanders merged commit 9b9dd86 into cylc:master Dec 30, 2020

TomekTrzeciak mentioned this pull request Jan 11, 2021

Cli setting of rose opts #4023

Merged

3 tasks

hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021

wxtim deleted the rose-conf.read_jinja2 branch March 22, 2022 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rose conf.read jinja2 #3913

Rose conf.read jinja2 #3913

wxtim commented Nov 3, 2020 •

edited by MetRonnie

Loading

TomekTrzeciak commented Nov 4, 2020

wxtim commented Nov 4, 2020

wxtim commented Nov 11, 2020 •

edited

Loading

oliver-sanders Dec 17, 2020

wxtim Dec 17, 2020 •

edited

Loading

oliver-sanders commented Dec 17, 2020

oliver-sanders commented Dec 17, 2020

TomekTrzeciak commented Dec 17, 2020

oliver-sanders commented Dec 17, 2020

TomekTrzeciak commented Dec 21, 2020

oliver-sanders commented Dec 21, 2020 •

edited

Loading

MetRonnie left a comment

oliver-sanders commented Dec 22, 2020 •

edited

Loading

oliver-sanders commented Dec 22, 2020

wxtim commented Dec 22, 2020

oliver-sanders left a comment



		def lion():
		raise TerriblePunException("This is a Lion's Main script")

Rose conf.read jinja2 #3913

Rose conf.read jinja2 #3913

Conversation

wxtim commented Nov 3, 2020 • edited by MetRonnie Loading

TomekTrzeciak commented Nov 4, 2020

wxtim commented Nov 4, 2020

wxtim commented Nov 11, 2020 • edited Loading

oliver-sanders Dec 17, 2020

Choose a reason for hiding this comment

wxtim Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Dec 17, 2020

oliver-sanders commented Dec 17, 2020

TomekTrzeciak commented Dec 17, 2020

oliver-sanders commented Dec 17, 2020

TomekTrzeciak commented Dec 21, 2020

oliver-sanders commented Dec 21, 2020 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders commented Dec 22, 2020 • edited Loading

oliver-sanders commented Dec 22, 2020

wxtim commented Dec 22, 2020

oliver-sanders left a comment

Choose a reason for hiding this comment

wxtim commented Nov 3, 2020 •

edited by MetRonnie

Loading

wxtim commented Nov 11, 2020 •

edited

Loading

wxtim Dec 17, 2020 •

edited

Loading

oliver-sanders commented Dec 21, 2020 •

edited

Loading

oliver-sanders commented Dec 22, 2020 •

edited

Loading