Reduce Rose-Cylc interaction part 1 #2239

matthewrmshin · 2018-10-04T14:56:57Z

This is mainly a companion change for cylc/cylc-flow#2693:

It recognises that the --host=HOST option for most Rose-wrapped-Cylc commands should now be redundant.
It uses and loads the suite contact file to determine whether a suite is running or not.
It finds suite contact files to list running suites.
It modifies rose suite-gcontrol --all to launch cylc gscan (instead of launching many cylc gui which can cause a machine to run out of resource).
It modifies rose suite-scan to simply invoke cylc scan directly.
It writes (Rose) automatic and custom environment variables to head of suite.rc.
- Introduce a new ROSE_SITE environment/jinja2 suite variable + site=SITE setting - useful for site portable suites.

lib/python/rose/config_processors/jinja2.py

oliver-sanders · 2018-10-10T09:41:42Z

etc/rose.conf.example

@@ -19,6 +19,9 @@ meta-path=DIR1[:DIR2[:...]]
 #
 # URL to Rose documentation.
 rose-doc=http://metomi.github.io/rose/
+#


This will create a leading blank line in the docs.

Should now be fixed.

oliver-sanders · 2018-10-10T09:56:35Z

Code looks good, a good bit of waffle removed. Functionality testing to follow.

matthewrmshin · 2018-10-10T12:00:30Z

Just added another changeset to remove more unused logic.

sadielbartholomew

I'm mid-review, but will record some initial comments, all minor. I have been unable to break anything thus far. The new settings are documented well; the docs build & new parts read & link fine.

The three test modules 26-... to 28- ... under t/rose-suite-run are very repetitive with respect to the example set-up & expected processed suite.rc files (not just due to this PR, but it has increased the replication). Is it worth trying to consolidate these?
While grepping for --host, I noticed that t/rose-cli-bash-completion/01-suite-running.t uses rose suite-run ... --host=localhost on line 32, with the host option having no effect (setting it to another valid host or a random string does not change the test pass, both here & on the current master.) Unless you know of a purpose for it, we might as well remove it in this PR.

sadielbartholomew · 2018-10-10T16:37:58Z

lib/python/rose/suite_engine_procs/cylc.py

+        items = []
+        for dirpath, dnames, fnames in os.walk(rootd, followlinks=True):
+            if dirpath != rootd and any(
+                    name in dnames or name in fnames for name in sub_names):


Is it perhaps more Pythonic to do any(name in dnames + fnames for ...)?

It is nicer syntax certainly. Not sure whether the extra + operation will have a penalty against the or short circuit logic. Not likely to have any impact here. I'll change the syntax just for the readability.

I don't think or and + will do the same thing here?

>>> a = [1,2,3] >>> b = [4,5,6] >>> [x for x in a or x in b] [1, 2, 3] >>> [x for x in a + b] [1, 2, 3, 4, 5, 6]

The original logic is more like:

any(x in a or x in b for x in x_list) # which should give the same results as: any(x in a + b for x in x_list)

lib/python/rose/suite_engine_procs/cylc.py

matthewrmshin · 2018-10-11T08:12:00Z

I'll leave t/rose-suite-run/{26,27,28}-*.t as-is. Refactors of this nature should be done in separate PRs. (And hopefully, these stuffs will be removed from Rose in the not-so-distant future.)

I'll also leave --host=localhost for safety. It should, in theory, forces cylc run --host=localhost, which is most desirable for testing purpose. (We don't want the suite to run on configured suite hosts.)

sadielbartholomew

Functional & sensible overall. Tests pass locally. Previous feedback addressed well.

oliver-sanders · 2018-10-24T13:20:43Z

This has been broken by cylc/cylc-flow#2759 (rose suite-run doesn't work if suite already registered).

Re-starting Travis-CI build.

matthewrmshin · 2018-10-24T14:47:29Z

OK. I'll take a look at the failed tests.

matthewrmshin · 2018-10-24T17:51:00Z

I am out of time. I have fixed 2 of 3 failed tests, but am unable to get the tutorial forecasting suite run to pass in my environment at the moment.

oliver-sanders · 2018-10-25T10:16:10Z

~~Taking a quick look it would appear the test is failing as it was taking advantage of the `cylc run DIR" behaviour (which was actually quite useful in this case):~~

~~cylc run "${TEST_DIR}/cylc-forecasting-suite" ...~~

I'll see if I can put up a fix to ~~your branch~~ Cylc => cylc/cylc-flow#2818

The failure occurs (with your changes to the .validate file) because the tutorial suite contains assets in a lib directory. Cylc doesn't automatically copy lib/template/map.html into the run directory. With the previous approach the suite is executed in the run directory so this isn't an issue.

A further change will be required after cylc/cylc-flow#2818 is merged, I'll take a look in tomorrow.

oliver-sanders · 2018-10-30T11:19:19Z

Looks like the ./t/rose-suite-restart/00-no-run.t failure requires attention.

The `--host=HOST` option for Cylc commands should now be redundant. Load suite contact file to determine whether a suite is running or not. Find suite contact files to list running suites. Modify `rose suite-gcontrol --all` to launch `cylc gscan`. Modify `rose suite-scan` to launch `cylc scan` directly. Rose suite automatic/custom environment variables now go to suite.rc. New ROSE_SITE environment variable + site=SITE setting

matthewrmshin · 2018-10-30T14:42:12Z

./t/rose-suite-restart/00-no-run.t failure should now be fixed.

fix tutorial suite test

matthewrmshin added this to the next-release milestone Oct 4, 2018

matthewrmshin self-assigned this Oct 4, 2018

matthewrmshin force-pushed the reduce-rose-cylc-interaction-1 branch 4 times, most recently from 929473d to 84a79d5 Compare October 8, 2018 09:34

matthewrmshin requested review from oliver-sanders and sadielbartholomew October 9, 2018 08:26

matthewrmshin force-pushed the reduce-rose-cylc-interaction-1 branch from da6e33e to 08b8d06 Compare October 9, 2018 08:43

oliver-sanders reviewed Oct 10, 2018

View reviewed changes

lib/python/rose/config_processors/jinja2.py Show resolved Hide resolved

oliver-sanders reviewed Oct 10, 2018

View reviewed changes

sadielbartholomew reviewed Oct 10, 2018

View reviewed changes

sadielbartholomew approved these changes Oct 12, 2018

View reviewed changes

matthewrmshin force-pushed the reduce-rose-cylc-interaction-1 branch from 36bdff5 to 34d4943 Compare October 24, 2018 17:48

matthewrmshin mentioned this pull request Oct 26, 2018

--all flag to rose suite-gcontrol not working #2244

Closed

matthewrmshin force-pushed the reduce-rose-cylc-interaction-1 branch from 34d4943 to 1086d67 Compare October 30, 2018 08:49

matthewrmshin force-pushed the reduce-rose-cylc-interaction-1 branch from bc38892 to bc659d3 Compare October 30, 2018 14:41

oliver-sanders and others added 2 commits October 30, 2018 16:36

fix tutorial suite test

e077682

Merge pull request #4 from oliver-sanders/fix-tutorial-suite-test

ba1438a

fix tutorial suite test

oliver-sanders approved these changes Oct 30, 2018

View reviewed changes

oliver-sanders merged commit aa1fc9a into metomi:master Oct 30, 2018

matthewrmshin deleted the reduce-rose-cylc-interaction-1 branch October 30, 2018 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce Rose-Cylc interaction part 1 #2239

Reduce Rose-Cylc interaction part 1 #2239

matthewrmshin commented Oct 4, 2018

oliver-sanders Oct 10, 2018

matthewrmshin Oct 10, 2018

oliver-sanders commented Oct 10, 2018

matthewrmshin commented Oct 10, 2018

sadielbartholomew left a comment

sadielbartholomew Oct 10, 2018

matthewrmshin Oct 11, 2018

oliver-sanders Oct 11, 2018

matthewrmshin Oct 11, 2018

matthewrmshin commented Oct 11, 2018

sadielbartholomew left a comment

oliver-sanders commented Oct 24, 2018 •

edited

Loading

matthewrmshin commented Oct 24, 2018

matthewrmshin commented Oct 24, 2018

oliver-sanders commented Oct 25, 2018 •

edited

Loading

oliver-sanders commented Oct 30, 2018

matthewrmshin commented Oct 30, 2018

Reduce Rose-Cylc interaction part 1 #2239

Reduce Rose-Cylc interaction part 1 #2239

Conversation

matthewrmshin commented Oct 4, 2018

oliver-sanders Oct 10, 2018

Choose a reason for hiding this comment

matthewrmshin Oct 10, 2018

Choose a reason for hiding this comment

oliver-sanders commented Oct 10, 2018

matthewrmshin commented Oct 10, 2018

sadielbartholomew left a comment

Choose a reason for hiding this comment

sadielbartholomew Oct 10, 2018

Choose a reason for hiding this comment

matthewrmshin Oct 11, 2018

Choose a reason for hiding this comment

oliver-sanders Oct 11, 2018

Choose a reason for hiding this comment

matthewrmshin Oct 11, 2018

Choose a reason for hiding this comment

matthewrmshin commented Oct 11, 2018

sadielbartholomew left a comment

Choose a reason for hiding this comment

oliver-sanders commented Oct 24, 2018 • edited Loading

matthewrmshin commented Oct 24, 2018

matthewrmshin commented Oct 24, 2018

oliver-sanders commented Oct 25, 2018 • edited Loading

oliver-sanders commented Oct 30, 2018

matthewrmshin commented Oct 30, 2018

oliver-sanders commented Oct 24, 2018 •

edited

Loading

oliver-sanders commented Oct 25, 2018 •

edited

Loading