-
Notifications
You must be signed in to change notification settings - Fork 53
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
Rose user guide rose tutorial #2170
Rose user guide rose tutorial #2170
Conversation
This is now more or less finished (minus spit and polish) - the Rosie section could do with a critical eye and possibly some contributions. |
ab9526a
to
0667754
Compare
Conflicts resolved. Ignore codacy issues regarding the use of |
Remaining test battery failure is pycode style which will be fixed when rebased on-top of #2172. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick initial comments. I'll give the change another eyeball in the afternoon.
sphinx/hyperlinks.rst
Outdated
.. _FCM: https://metomi.github.io/fcm/doc/ | ||
.. _FCM User Guide: http://metomi.github.io/fcm/doc/user_guide/ | ||
.. _Github: https://github.com/metomi/rose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best label? Perhaps Rose at Github?
@@ -26,7 +30,7 @@ file that looks like this: | |||
graph = """compose_letter => send_letter | |||
bob => read_letter""" | |||
|
|||
This sets up a simple suite which consists of the following: | |||
This is simple suite which consists of the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a simple ...
sphinx/tutorial/rose/rosie.rst
Outdated
Switch back to the trunk then merge your change branch into the trunk:: | ||
|
||
fcm sw trunk | ||
fcm merge change trunk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fcm merge change
should be sufficient.
See the :ref:`date-time tutorial <rose-tutorial-datetime-manipulation>` | ||
for more information. | ||
|
||
:ref:`command-rose-host-select` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After cylc/cylc-flow#1885 and cylc/cylc-flow#2199?
f03d1ae
to
2b223ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About to open a PR with a number of changes picked up from reading through the built tutorial pages. For now, note:
- Everything builds fine on my environment.
- Functionally, tutorial practicals are all great, except the second ('Rose Bush') practical on the
Rose Suite Configurations
page, where something has gone amiss. Will add in a fix to my PR. - PR: otherwise small changes, some trivial but many requested because I found certain wording/explanations unclear. Ask if you are unsure why I have asked to change something. A few comments that can't go in a PR.
- Just need to look over small changes to other files now, will complete reviewing tomorrow.
In Rosie suites the :term:`suite directory` is added to `version control`_ | ||
using `FCM`_. | ||
|
||
FCM is a `subversion <SVN>`_ (SVN) wrapper which provides a standard working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SVN
link is broken - it doesn't seem to be defined either in this file or in hyperlinks.rst
. As discussed will leave to you to choose an appropriate link destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
sphinx/tutorial/rose/suites.rst
Outdated
#. The Cylc suite is run. | ||
#. The Cylc gui is launched. | ||
|
||
.. digraph:: Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great diagram - really clear & useful!
sphinx/tutorial/rose/summary.rst
Outdated
|
||
The relationship between them is as follows: | ||
|
||
.. graph:: Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another really useful diagram - eventually I think it could be handy to collect such diagrams (as well as where they currently are) either on the Cheat Sheet or on a similar 'quick reference' type page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, for the moment I'll add diagrams / link the relevant sections through from the glossary.
@matthewrmshin Note I've added a few new glossary items in 307639c that could probably do with a quick scan. |
Section: rose applications Section: metadata Section: rose suite configurations The bit where it says 'TODO - remove triple quotes in cylc tutorial' The changes to the forecast task - it doesn't say where to find this. With the setting of initial and final cycle points, it doesn't seem The 'post_process_exeter' task currently fails due to not being able to Cheat sheet: |
sphinx/glossary.rst
Outdated
* :term:`warm start` | ||
|
||
warm start | ||
In a :term:`datetime cycling` suite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily datetime.
sphinx/glossary.rst
Outdated
configuration. | ||
|
||
Reloading changes to the ``[runtime]`` section is safe, reloading changes | ||
to the ``[scheduling]`` section has caveats, see the `Cylc User Guide`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: [runtime]
changes that affect the family inheritance hierarchy may affect scheduling as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See new commit.
Review feedback amendments
Section: rose applications
This should now be fixed.
The tutorial now gets you to
Note sure why this is happening, I'll take a look at this one later. Section: metadata
Agreed, the API pages are much more technical. I'm not entirely sure what to do about this really. We could remove the link from the tutorial as it's not really necessary or update the text to read something like.
I was a bit worried about this. For the moment I'll put in a TODO for future work. Suggestions welcome. Section: rose suite configurations
Good point, done.
I've now moved this into a comment.
I've re-phrased this to note that the task is in the
Fixed.
I'll need to see this error in context to advise, possibly a typo or the likes or a misleading instruction. Cheat sheet:
One is correct and the other is a strange figment of my immigration. Well spotted, not quite sure where that came from, fixed. |
WIND_FILE_TEMPLATE=test-data/wind_{cycle}_{xy}.csv | ||
RAINFALL_FILE=test-data/rainfall.csv | ||
MAP_FILE=map.html | ||
CYLC_TASK_CYCLE_POINT=20171101T0000Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting for future improvements brief conversation in person regarding setting this variable to 20171101T0000Z
being a potential cause of confusion for readers, given it is a random date-time tied to the test data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've inserted a better explanation of the test data in the application tutorial (a42675a).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. Looks good otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments for commits made after my first review (a01e3b6
onwards, inclusive). Previous feedback addressed. Now I just need to go over all existing sphinx
files with small changes as opposed to being completely new (all files under etc/
& bin/
i.e. non-doc files look fine).
@@ -6,8 +6,8 @@ Installation | |||
|
|||
.. _archives: https://github.com/metomi/rose/tags | |||
|
|||
The source code for Rose is available via `GitHub`_, code releases | |||
are available in ``zip`` and ``tar.gz`` `archives`_. | |||
The source code for Rose is available via `GitHub <Rose GitHub>`_, code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link is broken. I seem to recall you cannot provide custom text for a hyperlink created from another file, so will need to rename directly in sphinx/hyperlinks.rst
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link to SVN
in sphinx/tutorial/rose/rosie.rst
is also broken for the same reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, I keep walking into that one.
@@ -181,9 +181,9 @@ can be used to provide the path to the Rose application: | |||
.. code-block:: rose | |||
|
|||
# The weighting to give to the wind file from each WIND_CYCLE | |||
# (should add up to 1). | |||
# (comma separated list, values should add up to 1). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer comments, but for consistency, this change should really be also made in the config files that are provided for the user to use in other practicals (i.e. etc/tutorial/metadata-tutorial/rose-app.conf
& etc/tutorial/rose-suite-tutorial/app/forecast/meta/rose-meta.conf
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
WEIGHTING=1 | ||
# List of cycle points to get wind data from. | ||
# Comma separated list of cycle points to get wind data from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comment above, & also in this case etc/tutorial/runtime-tutorial/runtime
needs adapting accordingly.
sphinx/glossary.rst
Outdated
|
||
.. _daemon: https://en.wikipedia.org/wiki/Daemon_(computing) | ||
|
||
By default a suite server program is a `daemon`_ meaning that is runs in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo 'is it'
sphinx/glossary.rst
Outdated
When we say that a :term:`suite` is "running" we mean that the cylc | ||
suite server program is running. | ||
|
||
The suite server program is responsible for running the suite, it submits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'... the suite. It submits ...' is better grammar & reads better.
sphinx/glossary.rst
Outdated
|
||
reload | ||
Any changes made to the ``suite.rc`` file whilst the suite is running | ||
will not have any effect untill the suite is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'untill'. Also, I suggest appending 'either' onto the end of the sentence here, to be fully clear that these things are not all necessary!
* :term:`Shutdown` and :term:`restarted <restart>` | ||
* "Reloaded" | ||
|
||
Reloading does not require the suite to be :term:`shutdown`. When a suite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:term:`shut down <shutdown>`
:)
sphinx/glossary.rst
Outdated
|
||
Reloading does not require the suite to be :term:`shutdown`. When a suite | ||
is reloaded any currently "active" :term:`tasks <task>` will continue with | ||
their "pre-reload" configuration, new tasks will use the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'configuration, whilst new tasks...' perhaps?
@@ -212,19 +222,13 @@ application: | |||
|
|||
.. code-block:: rose | |||
|
|||
# The cycle point for the test data. | |||
# The date when the test data was gathered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer! There should be no confusion about this now.
sphinx/glossary.rst
Outdated
to the ``[scheduling]`` section has caveats, see the `Cylc User Guide`_. | ||
Reloading changes is safe providing they don't affect the | ||
:term:`suite's <suite>` :term:`graph`. Changes to the graph have certain | ||
caveats attached, see the `Cylc User Guide`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attached. See the ... [for more information].' reads better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete. I have some final comments, all minor. My last set of feedback has been addressed suitably.
Overall the new Rose Tutorial section is extremely good & should be very instructive to anyone who reads it!
@@ -14,10 +14,18 @@ site configuration at ``$ROSE_HOME/etc/rose.conf`` where | |||
search for the user configuration at ``$HOME/.metomi/rose.conf`` where | |||
``$HOME`` is the home directory of the current user. | |||
|
|||
You can also override many internal constants of the ``rose config edit`` and | |||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note is no longer applicable, the rose.conf
settings section having been added in now.
foo -> bar -> baz -> qux | ||
pub -> bar -> wop | ||
|
||
#. **Use** ``cylc graph`` **to visualise the workflow.** | ||
#. **Use** ``Cylc graph`` **to visualise the workflow.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a command, so 'cylc' still needs to be lower-case here.
@@ -406,15 +401,15 @@ starting 5 cycles *after* the initial cycle point. | |||
|
|||
#. **Visualise the suite.** | |||
|
|||
Try visualising the suite using ``cylc graph``. | |||
Try visualising the suite using ``Cylc graph``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a command, so 'cylc' still needs to be lower-case here.
@@ -13,9 +13,9 @@ following: | |||
* (optionally) files containing data that cannot easily be represented by the | |||
INI format. | |||
|
|||
We have added the following conventions into the rose configuraiton format: | |||
We have added the following conventions into the Rose configuraiton format: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed a typo ('configuraiton') - it's outside of the PR changes but we should get it fixed now.
Built on #2166, hold review until merged (will resolve conflicts after this merge).Rose tutorial for the new documentation:
Main objectives for this content:
rose metadata-check
rose config
rose app-run
rose suite-run
rose task-run
To do in a future PR:
Ready for preliminary review (structure, approaches, etc).