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

Make suite context available before config parsing #2963

Merged
merged 7 commits into from
Mar 13, 2019

Conversation

dwsutherland
Copy link
Member

Problem
In order to have information available in the environment for template filters (Jinja2 ...etc), it needs to be exported before parsing/processing the suite.rc. For example I may want to reference the installed or source directory:

#!/usr/bin/env python

# Get items from suite run-dir file to create tasks...

import os

def get_file_items(ITEMS):
    suiterun = os.environ['CYLC_SUITE_RUN_DIR']
    infile = open(os.path.join(suiterun,'dat','items.dat'))
    ITEMS = [line.rstrip('\n') for line in infile]
    return ITEMS.sort()

With the run path not always being the same as the def/source path, and PWD not being consistent anyway, the alternative is to hard code this somewhere before running (or create some awkward workaround?).

The suite context is available locally after startup (on reload).

A test suite with the above (and below) filter might be:

#!/usr/bin/env python

# Get the current environment (maybe os module available in jinja?)

import os

def test_func(ENV_DIC):
    return ENV_DIC.update(os.environ)
#!Jinja2

[meta]
    title = Suite Env Jinja

{% set FROM_FILE = true %}

{% if FROM_FILE %}
    {% set ITEMS=[] | get_file_items %}
{% else %}
    {% set ITEMS = ['baz'] %}
{% endif %}

{% set ENV_DIC={} | test_func %}
{% for ITEM, VALUE in ENV_DIC.items() %}
# {{ITEM}} = {{VALUE}}
{% endfor %}

[cylc]
    cycle point format = CCYY
[scheduling]
    initial cycle point = 1
    final cycle point = 1
    [[dependencies]]
        graph = """
{% for ITEM in ITEMS %}
{{ ITEM }}
{% endfor %}
"""
[runtime]
    [[root]]
        [[[environment]]]
{% if FROM_FILE %}
            KEYS = small_key
            VALS = chest-padlock
{% else %}
            KEYS = big_key,mediam_key
            VALS = house,garage
{% endif %}

{% for ITEM in ITEMS %}
    [[{{ ITEM }}]]
        script = """

IFS=', ' read -r -a k_ray <<< "$KEYS"
IFS=', ' read -r -a v_ray <<< "$VALS"

for index in "${!k_ray[@]}"; do
    echo "unlocking {{ ITEM }} ${v_ray[index]} with ${k_ray[index]}"
    if [[ "${v_ray[index]}" == *"chest"* ]]; then
        echo 'You Struck Gold!!! $$$'
    fi
done
echo
"""
{% endfor %}

Solution
There doesn't appear to be a reason why the suite context can't be set in the local environment before parsing the suite config. So this is what I've done.
However, to make the same functionality work for getting config and graph information via the CLI, I had to shift the exports out of the schedular.py and into the config.py (rather than just exporting before the SuiteConfig instantiation).

(remote host might need tested, as I didn't set that up, even though the other tests passed)

Note: if this passes standard, I'll make a branch for pull request to 7.8.x...

@dwsutherland dwsutherland self-assigned this Feb 21, 2019
@hjoliver
Copy link
Member

(Style violations reported in T-CI)

@cylc cylc deleted a comment Feb 21, 2019
@dwsutherland
Copy link
Member Author

dwsutherland commented Feb 21, 2019

(Style violations reported in T-CI)

Apologies - Only used pylint and test-battery before submitting, will hit my next ones with Travis/Codacy next time.

lib/cylc/config.py Outdated Show resolved Hide resolved
@hjoliver hjoliver added this to the soon milestone Feb 21, 2019
@hjoliver
Copy link
Member

# Get the current environment (maybe os module available in jinja?)

https://cylc.github.io/cylc/doc/built-sphinx/suite-config.html#accessing-environment-variables-with-jinja2

@hjoliver
Copy link
Member

This is fine in principle, I think. We'd need to document that the values are as defined on the parsing host (could be confusing if users use these values in task job config instead of referencing the environment variable for evaluation at job run time?)

@hjoliver
Copy link
Member

(But lets see what others think, before code review).

@dwsutherland
Copy link
Member Author

# Get the current environment (maybe os module available in jinja?)

https://cylc.github.io/cylc/doc/built-sphinx/suite-config.html#accessing-environment-variables-with-jinja2

Ah yes, I've even used that myself 😆 ... Didn't bother looking it up again, as the issue still stands.

@dwsutherland
Copy link
Member Author

dwsutherland commented Feb 21, 2019

This is fine in principle, I think. We'd need to document that the values are as defined on the parsing host (could be confusing if users use these values in task job config instead of referencing the environment variable for evaluation at job run time?)

Even without this change the parsing host environment and job-writer-suite-environment use/access the same variables/values (defined in the scheduler and config);

        # Pass static cylc and suite variables to job script generation code
        self.task_job_mgr.job_file_writer.set_suite_env({
            'CYLC_UTC': str(get_utc_mode()),
            'CYLC_DEBUG': str(cylc.flags.debug).lower(),
            'CYLC_VERBOSE': str(cylc.flags.verbose).lower(),
            'CYLC_SUITE_NAME': self.suite,
            'CYLC_CYCLING_MODE': str( 
                self.config.cfg['scheduling']['cycling mode']),
            'CYLC_SUITE_INITIAL_CYCLE_POINT': str(self.initial_point),
            'CYLC_SUITE_FINAL_CYCLE_POINT': str(self.final_point),
        })

And apparently this is overwritten by the task/job management prior to submission.. The comment I should have rewritten somewhere else:

        # Set local values of variables to give suite context before parsing
        # config, i.e for template filters (Jinja2, python ...etc) and possibly
        # needed locally by event handlers. Potentially task-specific due to
        # different directory paths on different task hosts, however, they are
        # overridden by tasks prior to job submission:

Sorry, I see what you're saying now... Yes, might not be a good idea for them to use TASK_LOG_FILE = {{ environ['CYLC_SUITE_LOG_DIR'] }}/mylog.out with a job destined for remote host..

In the morning I'll add to the documentation what environmental suite context is available, in the same Jinja2 environ section you referenced:
https://cylc.github.io/cylc/doc/built-sphinx/suite-config.html#accessing-environment-variables-with-jinja2
Even though there already is a caution, perhaps I could make it a bold "Note:" footnote.

@dwsutherland
Copy link
Member Author

dwsutherland commented Feb 22, 2019

I thought of suffixing all of the variables with _ON_SUITE_HOST, i.e.:
CYLC_SUITE_DEF_PATH
CYLC_SUITE_DEF_PATH_ON_SUITE_HOST
But that's for the purpose of jobs on remote hosts, so it should be the other way round:
CYLC_TASK_SUITE_DEF_PATH

Because then CYLC_SUITE_DEF_PATH should only ever mean one thing! (I think)

Obviously sub suites are their own suites, so a different environmental suite context.

@dwsutherland
Copy link
Member Author

Updated module docstring, and sphinx:
sphinx_update

@cylc cylc deleted a comment Feb 22, 2019
@dwsutherland
Copy link
Member Author

dwsutherland commented Feb 22, 2019

BTW - CYLC_SUITE_HOST and CYLC_SUITE_OWNER should be always the same as the already available HOSTNAME & USER (right?).. So no point in adding those..

@matthewrmshin matthewrmshin modified the milestones: soon, cylc-8.0.0 Feb 25, 2019
@cylc cylc deleted a comment Mar 1, 2019
@dwsutherland
Copy link
Member Author

Why do I have to keep kicking travis-ci ... Changing the docs should not cause those failures, so inconsistent.

@kinow
Copy link
Member

kinow commented Mar 2, 2019

@dwsutherland: #2894

@cylc cylc deleted a comment Mar 12, 2019
@dwsutherland
Copy link
Member Author

Re-based post python 3 merge.

@cylc cylc deleted a comment Mar 13, 2019
@dwsutherland
Copy link
Member Author

Rebased onto reverted master.. @matthewrmshin ready for review

@dwsutherland
Copy link
Member Author

@matthewrmshin thanks! how do we normally proceed? (who does the merge?)

I'll create a mirror request for 7.8.x after (then I'll have this available for NIWA Ops after next release?!)

@hjoliver
Copy link
Member

Normally the 2nd reviewer does the merge (if the result is "approved"). I'll take another look now ...

@hjoliver
Copy link
Member

(Style violations reported in T-CI)

Apologies - Only used pylint and test-battery before submitting, will hit my next ones with Travis/Codacy next time.

You can use the pep8 command or its successor pycodestyle in your own environment, to check style.

@hjoliver
Copy link
Member

BTW - CYLC_SUITE_HOST and CYLC_SUITE_OWNER should be always the same as the already available HOSTNAME & USER (right?).. So no point in adding those..

Yes (these CYLC_ vars are provided to job environments ... because jobs can potentially run as another user on another host).

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending suggested minor doc change.

doc/src/suite-config.rst Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Mar 13, 2019
@hjoliver
Copy link
Member

Thanks @dwsutherland 👍

@hjoliver hjoliver merged commit e78ad06 into cylc:master Mar 13, 2019
@dwsutherland
Copy link
Member Author

Whoop-whoop! cherry popped 🍒
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants