Skip to content
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 suite-run: plugin for rose-suite.conf functionality #3819

Closed
11 tasks done
oliver-sanders opened this issue Sep 16, 2020 · 25 comments
Closed
11 tasks done

rose suite-run: plugin for rose-suite.conf functionality #3819

oliver-sanders opened this issue Sep 16, 2020 · 25 comments
Assignees
Labels
question Flag this as a question for the next Cylc project meeting.
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 16, 2020

Superseedes: #2960
Related to: #1885

Write a Cylc plugin which detects the rose-suite.conf file and acts accordingly.

This plugin will call out to Rose2 libraries, this functionality already exists in the Rose it's just a matter of injecting it in the correct places in the Cylc logic.

The rose-suite.conf File

The rose-suite.conf file has the following sections:

  • opts - Optional configuration keys.
  • root-dir - Affects suite installation, symlinks Cylc directories.
  • [env] - Environment variables.
  • [jinja2:suiterc] - Jinja2 variables.
  • [empy:suiterc] - Empy variavles.
  • [file:*] - File installation.

opts

Rose should use this variable to determine which optional configuration files to load. This is already implemented in the existing Rose configuration logic so shouldn't require any extra work on the Cylc side.

root-dir

Obsolete, replaced by #3688, log a warning and do nothing.

Questions:

  • Should we make an attempt to map this onto the new Cylc [symlink dirs][<install target>]run configs? No.

[env]

Export these environment variables into the Cylc environment (i.e. os.environ[key] = var).

Note: We should not edit the flow.cylc file, if exporting the env vars is insufficient we could consider adding a command line option to cylc (run|restart).

For now the path of least resistance is probably to just go ahead and meddle with the flow.cylc file, we can keep this logic in the Cylc part (rather than the plugin part) and improve it later, perhaps with a new Cylc options file or an environment variable table in the database (to match the template variables table).

Questions:

  • How is this supposed to work, Cylc may re-execute on another host?!
    • Rose adds items under [cylc][evironment]
  • Can we achieve this without meddling with the flow.cylc file (really don't want to go down that route again)?

[jinja2:suiterc]

Note: I think we will probably keep this as :suite.rc for now.

With cylc7/rose-2019 Rose prepends these variables to the top of the suite.rc file. It should not be necessary to edit any Cylc files to achieve this in Cylc8.

Note: Look at implementing this logic in the parsec layer so that it doesn't have to be manually implemented on a command by command basis.

Fallback: Otherwise we have the ability to set "suite variables" (i.e. Jinja2/empy varaibles) on the command line using -s KEY=value or -f file-containing-variables. Load these variables and prepend them to the list of variables passed in on the command line.

[empy:suiterc]

Same as for Jinja2.

  • Raise an error if both Jinja2 and Empy variables are provided.
  • For bonus marks check the shebang at the top of the flow.cylc file to make sure the rose-suite.conf file is configured for the correct template parser and raise an error if it is not or if no template parser is being used.

[file:*]

Call out to the Rose file installation system as the current rose suite-run logic does.

Questions:

  • Should we perform file installation logic on reloads? Yes
  • Do we need a command line option to control this?

Where To Inject This Logic In Cylc

At a glance I think the following commands will require loading the rose-suite.conf:

  • get-suite-config|validate|view|graph|diff
    • opts
    • jinja2:suite.rc
    • empy:suite.rc
  • run|restart
    • all sections are relevant to these commands
    • the plugins should be called after the cylc run|restart command has been re-executed on the suite server (if the suite server is not localhost)
  • reload
    • all sections are relevant to this command
    • this one is somewhat trickier as it runs inside the running scheduler
    • it may be better to leave this till last

Implementation

Create two new Python entry points to keep this functionality separate from Cylc:

  • One entry point for setting environment ([env]) and template ([*:suite.rc]) variables.
  • One for performing installation operations ([file:*]) for suite installation.

The plugin interfaces should probably look something like this:

set_environment(src_dir: Path) -> dict  # {'environment': {}, 'template-vars': {}}
install(src_dir: Path, run_dir: Path, run_mode: str) -> None

The plugins should output any required information using the Cylc log.

This work can be attempted in stages implementing one section at a time. The [env] section is probably the easiest to get started with.

Checklist

Plugins:

  • The one which modifies the suite environment.
  • The one which performs installation operations.

Commands:

  • cylc get-suite-config
  • cylc run
  • cylc restart
  • cylc validate
  • cylc view
  • cylc diff
  • cylc view
  • cylc graph - perhaps skip this one as the command only exists for test functionality
  • cylc reload - perhaps leave this till last as it may be a bit more involved
@oliver-sanders oliver-sanders added the question Flag this as a question for the next Cylc project meeting. label Sep 16, 2020
@oliver-sanders oliver-sanders added this to the cylc-8.0a4 milestone Sep 16, 2020
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 16, 2020

The big question: where do we put this?

  • Do we want to develop this functionality in the Cylc Flow repo (making Rose2 an optional dependency of Cylc).
  • Or should we put it into the Rose repository making Cylc8 an optional dependency of Rose2.
  • Or should we just create a new repository.

I'm happy with whatever, it might be easier for the moment to develop in Cylc Flow and transfer later. The entry point should keep the codebases sufficiently separate.

@hjoliver
Copy link
Member

(I think the questions in the Issue description are mainly aimed at the UK Rose experts).

The big question: where do we put this?

I'm not too bothered either, if you want to do in cylc-flow now then move it out later.

@wxtim
Copy link
Member

wxtim commented Sep 17, 2020

Do we want to develop this functionality in the Cylc Flow repo (making Rose2 an optional dependency of Cylc).

This seems the most logical to me, at least for the time being.

@TomekTrzeciak
Copy link
Contributor

[jinja2:suiterc]

Note: I think we will probably keep this as :suite.rc for now.

With cylc7/rose-2019 Rose prepends these variables to the top of the suite.rc file. It should not be necessary to edit any Cylc files to achieve this in Cylc8.

We have the ability to set "suite variables" (i.e. Jinja2/empy varaibles) on the command line using -s KEY=value or -f file-containing-variables.

Load these variables and prepend them to the list of variables passed in on the command line.

Due to the current implementation there's a significant difference between rose and cylc suite variables in that rose ones are treated as Jinja2 code, whereas cylc ones only as strings. Perhaps it would make sense to look first at implementing equivalent of #3169 to get these two on equal footing and treat them the same.

[file:*]

Call out to the Rose file installation system as the current rose suite-run logic does.

Questions:

  • Should we perform file installation logic on reloads?

Reload is often used to update files in the suite, so I'd say yes. Unless you want to use this as an "incentive" for people to migrate off rose 😉.

@oliver-sanders
Copy link
Member Author

rose ones are treated as Jinja2 code, whereas cylc ones only as strings

Under the hood Cylc passes these variables straight through to Jinja2, it's just the CLI that enforces string arguments so this approach should support typing from the off.

@TomekTrzeciak
Copy link
Contributor

TomekTrzeciak commented Sep 28, 2020

rose ones are treated as Jinja2 code, whereas cylc ones only as strings

Under the hood Cylc passes these variables straight through to Jinja2, it's just the CLI that enforces string arguments so this approach should support typing from the off.

Not enough. If you're not going to write the rose vars into the template any more, you'll need to eval them somewhere before passing into jinja to reproduce cylc7 behaviour.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 28, 2020

Well yes, we will have to eval them so that we are passing Python objets rather than strings (we won't be able to just strip away the outer quotes and let Jinja2 implicitly eval them). This shouldn't be an issue with the internal interface.

@wxtim
Copy link
Member

wxtim commented Sep 30, 2020

I had a proof-of-concept go at implementing the basic functionality of the [jinja2:suite.rc] section in this branch.

The logic to support optional configs is present in the function get_rose_vars and I tested it, but for the PoC I didn't bother plugging it into the cylc command line.

Also, I haven't tried the literal_eval yet - can anyone give me some examples of the sort of thing we might expect to need evaluating?

Any thoughts.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 30, 2020

The [jinja2:suite.rc] section is typically used to inject basic Python data types into the suite.rc file. Lists are often used and, although unsupported in the Rose GUI, dicts are pretty common as well.

At present anything you configure just gets dumped straight into the suite.rc file and left for Jinja2 to evaluate so any types that Jinja2 supports could be used.

Had a quick look at the jinja2 code not expecting any surprises from that.

>>> from ast import literal_eval
>>> literal_eval('42')
42
>>> literal_eval('True')
True
>>> literal_eval('"True"')
'True'
>>> literal_eval('[1, 2, 3]')
[1, 2, 3]
>>> literal_eval("{'a': 1, 'b': 2, 'c': 3}")
{'a': 1, 'b': 2, 'c': 3}

@wxtim
Copy link
Member

wxtim commented Sep 30, 2020

I think that I have now got the literal eval working in that branch.

@dpmatthews
Copy link
Contributor

Further details on how the [env] section is used.

  • Some users have accessed [env] variables in the suite.rc via jinja2, e.g.

    {{ environ['WORKING_COPY_PATH'] }}
    

    Note that this currently only works if you run the suite locally rather than cylc spawning on another server. Should we support this?

  • Some users reference [env] variables in other sections of the rose-suite.conf file, e.g.

    [env]
    APPS_BASE="fcm:foo_tr"
    APPS_RVN="@1234"
    [file:apps/bar]
    source = ${APPS_BASE}/apps/bar${APPS_RVN}
    

    This is only way of defining a single variable which can be referenced in multiple places within the rose-suite.conf file.
    It is a "supported" way of working (although I can't find it documented anywhere - we should fix that)

  • All defined [env] variables get added to the [cylc][[environment]] section.

  • Setting CYLC_VERSION and ROSE_VERSION variables in rose-suite.conf currently allows rose suite-run to:
    a) Change the version of Cylc and check the version of Rose used to run the suite;
    b) Prevent suites accidentally upgrading or downgrading versions on restart.
    Review whether we want to support this in some way later.

@oliver-sanders
Copy link
Member Author

For reference here is some example output (suite.rc) showing the Jinja2 and environment meddling that Rose currently does and the default variables it provides:

#!jinja2                                                                                                                                                                                                       
{# Rose Configuration Insertion: Init #}
{% set CYLC_VERSION="7.8.6" %}
{% set ROSE_ORIG_HOST="myhostname" %}
{% set ROSE_SITE="sitename" %}
{% set ROSE_VERSION="2019.01.3" %}
{% set ROSE_SUITE_VARIABLES={
    'CYLC_VERSION': CYLC_VERSION,
    'ROSE_ORIG_HOST': ROSE_ORIG_HOST,
    'ROSE_SITE': ROSE_SITE,
    'ROSE_VERSION': ROSE_VERSION,
} %}
[cylc]
    [[environment]]
        CYLC_VERSION=7.8.6
        ROSE_ORIG_HOST=myhostname
        ROSE_SITE=
        ROSE_VERSION=2019.01.3
{# Rose Configuration Insertion: Done #}

@hjoliver
Copy link
Member

hjoliver commented Oct 6, 2020

Some users have accessed [env] variables in the suite.rc via jinja2, e.g.
{{ environ['WORKING_COPY_PATH'] }}

Note that this currently only works if you run the suite locally rather than cylc spawning on another server. Should we support this?

I guess we can't prevent this, even though it is not advisable, except by not exporting rose-suite.conf variables in the scheduler environment at all - is that what you're suggesting?

@dpmatthews
Copy link
Contributor

I guess we can't prevent this, even though it is not advisable, except by not exporting rose-suite.conf variables in the scheduler environment at all - is that what you're suggesting?

It only works if we add these variables to the environment before we start to process the cylc.flow file (just adding them to the [cylc][[environment]] section isn't enough). So, I think we would have to choose to deliberately support this.

@TomekTrzeciak
Copy link
Contributor

Is this still in the plan #3169 (comment):

From current discussions, in Cylc8 we will likely have a new file for setting suite variables which will be a Python file (allowing variables to be directly imported) which would provide a much nicer solution for this.

I would really like to be able to run a generic, custom logic in a standard way at start up, not just literal_eval.

@oliver-sanders
Copy link
Member Author

Is this still in the plan #3169 (comment):

Not officially tagged against Cylc8, however, pretty trivial to implement using the interface provided by this plugin.

@TomekTrzeciak
Copy link
Contributor

With rose suite-run it is common to pass optional configs with -O opt_conf command line options. I guess not something that can be done with the plugin, or can it?

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 8, 2020

A lot of people use this so we will need to find a way to supporting -O. I expect the plugin can provide an optparse object or else just some basic options in a dictionary.

(Note it's not worth building too-advanced an interface for it now as the CLI library is likely to change)

@TomekTrzeciak
Copy link
Contributor

A lot of people use this so we will need to find a way to supporting -O. I expect the plugin can provide an optparse object or else just some basic options in a dictionary.

Interesting challenge for CLI design...

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 8, 2020

Yeah not the nicest, perhaps (one day) we should use a load method interface for plugins, pytest does this quite nicely.

@oliver-sanders
Copy link
Member Author

Assigning @wxtim as he has some some prelim investigation work with Jinja2 vars.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Nov 12, 2020

Optional Conf Keys

Installation

With Rose 2019.x optional config keys can be configured in three ways:

  1. The opts configuration in the rose-suite.conf file.
  2. The -O CLI option to rose suite-run.
  3. The ROSE_SUITE_OPT_CONF_KEYS env var visible to rose suite-run.

All three should remain supported in Cylc8/Rose2.

  • These should apply to cylc install.
  • (2) and (3) will need to be recorded somewhere so that subsequent commands (e.g. cylc play) pick up the optional config(s) that the flow was installed with.
  • In Rose 2019.x this information goes into log/rose-suite.conf.
  • In Cylc8 we could manipulate the opts config in the installed rose-suite.conf file if required.

Re-Installation

Subsequent re-installation should preserve optional configs specified by previous installations.

I.E:

cat > rose-suite.conf <<< opts=foo
cylc install -O bar -O baz
# optional configs foo, bar and baz applied
cylc reinstall
# optional configs foo, bar and baz still applied

However, additional CLI options will be provided to permit the removal of targeted optional configs or permit rebuilding the list from scratch (as done by the original install).

I.E:

cylc reinstall -N bar
# optional configs foo and baz still applied, bar removed
cylc reinstall --rebuild-opt-confs
# optional config foo applied (as it is specified in the rose-suite.conf)
# bar and baz removed (as they were specified by other means)

CLI arguments TBC but should be something along the lines of:

  • -O, --opt-conf-key - To match current Rose interface
  • -N, --remove-opt-conf-key
  • --reset-opt-conf-keys

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Jan 11, 2021

@TomekTrzeciak - In reply to #4023 (comment)

We are planning to keep Rose and Cylc templatevars separate but have unified their syntax as much as possible:

  • The Rose config as a whole will be installed into the run dir (unlike at present) along with all optional configs.
  • Rose options set on the CLI will get written to their own cylc-install optional config in the installed workflow.
  • This provides a high level of transparency and allows installed options to be edited on-the-fly.
  • This also means you will be able to read the config using the CLI (rose config) or the Python API (metomi.rose.config).
  • And the future Rose Edit will be able to view and edit these configurations too (with metadata).
  • Of course the biggie is that installed options will persist across restarts.
  • You can override Rose templatevars with the Cylc -s, --set-file.

We have opted not to unify Rose and Cylc template variables because they are not directly equivalent and don't quite have the same scope:

Unifying Rose and Cylc templatevars is not all that straight forward and I'm not entirely sure it makes sense:

  • It wouldn't make sense to pull Cylc templatevars into Rose config files.
  • If Rose templatevars went in a Cylc config file they would not be visible to Rose.
  • Duplicating them in two places, 🤮.

More context:

  • Cylc does not yet possess a mechanism for "installing" template vars (as they are stored in the suite DB).
    • We will likely write a Cylc (built-in) plugin which supports a standard Cylc templatevar file.
    • This would write CLI changes back to that templatevar file and work much like the Rose plugin.
    • E.G. you could read and edit templatevars in the run directory.
    • However, we would need to remove templatevars from the DB to avoid nasty dual-state.
    • This plugin would be on an equal footing to the Rose plugin, the two could co-exist, however, would not be able to communicate.
      • Good for simplicity and separation.
      • Will need to think through conflict resolution.
    • A nice-to-have future enhancement but not currently mission critical.
  • Rose templatevars are processed, Cylc templatevars are not.
    • Rose templatevars can include environment variables from the "suite run" environment or the Rose [env] section.
    • Cylc doesn't have a way of controlling env vars, they are also a pretty horrible way to define inputs so not keen to go down that route.
  • Rose has different parsing rules for templatevars.
    • Despite best efforts, I've had to concede on this one.
    • Sadly there are just too many workflows using Jinja2 semantics (like true rather than True) for us to drop support in Rose completely.
    • Other example include implicit tuples - a, b, c rather than (a, b, c) which are in very wide use and hard to replace in metadata.
    • However, we will drop support for Jinja2 expressions (e.g. range(5), [0, 1] + [2, 3]) as planned.
    • An unfortunate side-effect is that Rose will not support Python set syntax {a, b, c} which Cylc already does.
    • Might be able to hack around this last point, however, since Rose doesn't support sets anyway and it would be a nasty hack, not keen on trying.

For more information see:

@oliver-sanders
Copy link
Member Author

@wxtim closed by cylc-rose PRs?

@wxtim
Copy link
Member

wxtim commented Feb 23, 2021

cylc/cylc-rose#37 to replace outstanding section of this issue.

@wxtim wxtim closed this as completed Feb 23, 2021
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Flag this as a question for the next Cylc project meeting.
Projects
None yet
Development

No branches or pull requests

5 participants