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

[[localhost]] platform selects from top of platforms list #4845

Closed
wxtim opened this issue May 3, 2022 · 7 comments · Fixed by #4854
Closed

[[localhost]] platform selects from top of platforms list #4845

wxtim opened this issue May 3, 2022 · 7 comments · Fixed by #4854
Assignees
Labels
doc Documentation
Milestone

Comments

@wxtim
Copy link
Member

wxtim commented May 3, 2022

Describe the bug
Localhost doesn't seem to follow the same platform selection rules as other platforms - it appears to select the top regex in the list, and not the bottom one!

Steps to reproduce the bug
Given the global.cylc:

[platforms]
    [[localhost|foo|bar]]
        global init-script = "echo 'defined  first'"
    [[localhost]]
        global init-script = "echo 'defined last'"
    [[foo|bar|baz]]
        global init-script = "echo 'defined first'"
    [[baz]]
        global init-script = "echo 'defined last'"

and the flow.cylc:

[scheduling]
    initial cycle point = 19831213T0600Z
    [[dependencies]]
        R1 = Quokka => Numbat
[runtime]
    [[Quokka]]
    [[Numbat]]
        platform = baz

then testing for the global init-script in the job script

grep "defined [first|last]" ~/cylc-run/localhost-investigation/runN/log/job/19831213T0600Z/*/NN/job

One would expect both to return "defined last" but in fact this is not the case for localhost.

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

@wxtim wxtim added the bug Something is wrong :( label May 3, 2022
@wxtim wxtim self-assigned this May 3, 2022
@wxtim
Copy link
Member Author

wxtim commented May 3, 2022

On further investigation, I'm not at all sure it's platforms that's at fault:

python -c "from cylc.flow.cfgspec.glbl_cfg import glbl_cfg; print(glbl_cfg().get(['platforms']).keys())"

Commenting out the default platform localhost fixes the problem.

Conclusion

What appears to be going on here is that the global config spec always adds localhost as a special case so it's always at the top of the list, even if you've defined it further down the list elsewhere.

Further examination suggests that even then you are getting the hard-set default localhost, and not overridding any of its settings - consider

python -c "from cylc.flow.cfgspec.glbl_cfg import glbl_cfg; print(glbl_cfg().get(['platforms', 'localhost', 'global init script']))"

Possible solutions

  1. Document that users should not use [localhost], add a warning if it's defined in the user config and suggest that users use something like [[localhost|some_very_unlikely_platform_name]] as a workaround.
  2. Add logic to parsec - allow the spec to specify cases where user setting can over-ride a hard set section like `[platforms][localhost].

@dpmatthews & @oliver-sanders - I'd appreciate your thoughts.

@oliver-sanders
Copy link
Member

oliver-sanders commented May 3, 2022

localhost is more of an alias than a host "name" per-se, it evaluates differently on every machine. I think it is ok if it doesn't work with platform pattern matches. Put everything into a [platform][localhost] section and I think you're fine?

So I'm suggesting we document not to use localhost in pattern matches.

What appears to be going on here is that the global config spec always adds localhost as a special case

We need localhost to be defined by default so we specify it with sensible defaults as a "meta" section of the config:

https://cylc.github.io/cylc-doc/latest/html/reference/config/global.html#global.cylc[platforms][localhost]

Further examination suggests that even then you are getting the hard-set default localhost, and not overridding any of its settings

Overriding [localhost] settings in the global.cylc works fine for me:

# global.cylc
[platforms]
    [[localhost]]
        hosts = foo
$ cylc config -i '[platforms][localhost]'
hosts = foo

Document that users should not use [localhost]

Isn't this the wrong way around?

@wxtim
Copy link
Member Author

wxtim commented May 3, 2022

Do I interpret this as you saying that

if "localhost" in platform_regex and platform_regex is not "localhost":
    raise CylcException("Don't use 'localhost' in a broader regex.")

@oliver-sanders
Copy link
Member

Yep, perhaps a warning rather than an exception, if we run the regex we can catch things like local.... and .* so this might be worthwhile:

if (
    # if the platform contains special regular expression characters
    re.escape(platform_regex) != platform_regex
    # and 
    and re.match(platform_regex, 'localhost')
):
    LOG.warn(f'The platform regex {platform_regex} matches "localhost", however, ...')

@oliver-sanders oliver-sanders added doc Documentation and removed bug Something is wrong :( labels May 3, 2022
@oliver-sanders oliver-sanders added this to the cylc-8.0rc4 milestone May 3, 2022
@oliver-sanders
Copy link
Member

Swapping "bug" for "doc", this behaviour is fine and easy to work around, we just need to document it somewhere. Either a warning in the code, or a line in cylc-doc or both.

@wxtim
Copy link
Member Author

wxtim commented May 5, 2022

In a conversation with @dpmatthews this morning, it looks like he favours making this an error, not a warning.

Also convinced me that it's probably worth bringing the comma separated list and expansion behaviour of the global config more into line with Cylc config.

@oliver-sanders oliver-sanders self-assigned this May 5, 2022
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue May 5, 2022
* Partially addresses cylc#4845
* This is intended to make it easier to configure the localhost platform
  to match other hosts e.g
  `[platform][localhost, desktop..., server...]`
* Note this **cannot** be done using a regex e.g.
  `[platform][localhost|desktop...|server...]` because the localhost
  platform is special (it gets defined by default as [localhost]) so the
  regex is not matched and consequently gets ignored - cylc#4845
@oliver-sanders
Copy link
Member

Items in the [graph] and [runtime] sections can be comma separated e.g. [runtime][foo, bar], however, this is not a generic parsec feature, it is enabled on a case-by-case basis so was not available to the global.cylc[platforms] section.

The [graph] and [runtime] section use different approaches, so I added another one 🙈 in parsec.utils which implements this splitting in a more generic fashion as we might want to turn it on for all __MANY__ sections.

https://github.com/oliver-sanders/cylc-flow/pull/new/parsec-section-expand-many

@MetRonnie MetRonnie changed the title [[localhost]] platform selects from top of platfroms list. [[localhost]] platform selects from top of platforms list May 12, 2022
datamel pushed a commit that referenced this issue May 13, 2022
* Add warning if a platform name regex matches localhost

* tests/u: parsec/test_util unittest->pytest

* glblcfg: split comma separated platform definitions

* Partially addresses #4845
* This is intended to make it easier to configure the localhost platform
  to match other hosts e.g
  `[platform][localhost, desktop..., server...]`
* Note this **cannot** be done using a regex e.g.
  `[platform][localhost|desktop...|server...]` because the localhost
  platform is special (it gets defined by default as [localhost]) so the
  regex is not matched and consequently gets ignored - #4845

* update changelog

* Better handling of [platforms][localhost]

convert warning to outright error

ensure that failure occurs wherever in hte platforms list the inappropriate regex is.

Update tests/functional/platforms/08-warn-if-regex-matches-localhost.t

Co-authored-by: Oliver Sanders <[email protected]>

removed two tests which will fail (correctly) after changes

* response to review

Co-authored-by: Oliver Sanders <[email protected]>
@dpmatthews dpmatthews modified the milestones: cylc-8.0rc4, cylc-8.0rc3 May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants