Fix global config default-to-localhost.

[hjoliver]

These changes close #3507

In global config host settings, default-to-localhost was broken for list-valued items. In the spec, these need to default to None (It was empty list looks like "item-set-to-nothing" rather than "item-not-set").

Apparently broken in 7.8.0 ?

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).

• Appropriate tests are included (unit and/or functional).

• Appropriate change log entry included.

• No documentation update required. (Fix conforms to documented behaviour).

Once finished, this needs to go to 7.9.x as well. But not to master as the platforms changes will render this irrelevant there? - ping @wxtim

[hjoliver]

Global config, set all list items:

[hosts]
   [[localhost]]
      copyable environment variables = FOO, BAR
      task event handler retry delays = PT99S, PT1H
      retrieve job logs retry delays = PT10S, PT30S, PT1M, PT3M
      submission polling intervals = PT10S, PT30S
      execution polling intervals = PT10S, PT30S
      [[[batch systems]]]
         [[[[pbs]]]]
            execution time limit polling intervals = PT10S, PT30S
   [[foo]]
      # (default to localhost!)

On 7.8.x:

$ cylc get-global -i '[hosts][foo]' | egrep '(delay|variable|interval)s'
copyable environment variables = 
task event handler retry delays = 
retrieve job logs retry delays = 
submission polling intervals = 
execution polling intervals = 

On this branch:

$ cylc get-global -i '[hosts][foo]' | egrep '(delay|variable|interval)s'
copyable environment variables = FOO, BAR
task event handler retry delays = PT1M39S, PT1H
retrieve job logs retry delays = PT10S, PT30S, PT1M, PT3M
submission polling intervals = PT10S, PT30S
execution polling intervals = PT10S, PT30S

TODO - batch system settings are still not defaulting to localhost. Reason:
globalcfg:transform() needs to distinguish empty dict from None and recurse into sub-sections to do the localhost defaulting.

[hjoliver]

Ready to go for 7.8.5 (Note this is a real bug, reported by a user at another site).

@kinow - can you review at this end - I based my unit test on your cylc-7 suite config unit tests.

[hjoliver]

NOTE: this bit is still broken (from description above):

TODO - batch system settings are still not defaulting to localhost. Reason:
globalcfg:transform() needs to distinguish empty dict from None and recurse into sub-sections to do the localhost defaulting.

It's probably not hard to fix, but I need to move on to other things and users are unlikely to hit it (would anyone really set batch system settings for multiple hosts under localhost?).

So I'll flag it as a new low-priority issue.

[hjoliver]

I won't replicate this to 7.9.x - we can do the 7.9.0 release by copying 7.8.x at the last minute and applying the Jinja2 fix as discussed in our recent meeting.

[hjoliver]

@wxtim - this is irrelevant for cylc 8, right?

[kinow]

Code looks good to me, and thanks for the extra comment with the example configuration and what works and what doesn't. Easier to review.

Copied your example to my ~/.cylc/global.rc, then with 7.8.4

kinow@ranma:/opt/cylc$ ./usr/bin/cylc version
7.8.4-49-gcab35
kinow@ranma:/opt/cylc$ ./usr/bin/cylc get-global -i '[hosts][foo]' | egrep '(delay|variable|interval)s'
copyable environment variables = 
task event handler retry delays = 
retrieve job logs retry delays = 
submission polling intervals = 
execution polling intervals =

And with this branch:

kinow@ranma:/opt/cylc$ ./usr/bin/cylc version
7.8.4-51-g28c34
kinow@ranma:/opt/cylc$ git status
On branch pr-3508
nothing to commit, working tree clean
kinow@ranma:/opt/cylc$ ./usr/bin/cylc get-global -i '[hosts][foo]' | egrep '(delay|variable|interval)s'
copyable environment variables = FOO, BAR
task event handler retry delays = PT1M39S, PT1H
retrieve job logs retry delays = PT10S, PT30S, PT1M, PT3M
submission polling intervals = PT10S, PT30S
execution polling intervals = PT10S, PT30S

With Cylc 8 I get the same as Cylc 7.8.x but looks like it's not important right now (from reading @hjoliver 's comment).

[kinow]

@hjoliver I think the unit test will need some more work

lib/cylc/tests/test_global_config.py::TestGlobalConfig::test_localhost_default_list_items FAILED [ 10%]

(which works locally for me 👀 )

[hjoliver]

With Cylc 8 I get the same as Cylc 7.8.x but looks like it's not important right now (from reading @hjoliver 's comment)

Yeah, what I meant there was the defaulting to localhost is going to go away with the upcoming "Platforms" changes.

[hjoliver]

@hjoliver I think some unit tests will need some more work

lib/cylc/tests/test_global_config.py::TestGlobalConfig::test_localhost_default_list_items FAILED [ 10%]

Damn it, passes locally:

[oliverh@niwa-1007885 cylc-flow-7.8.x]$ git status                                                       │
On branch fix-localhost-inherit                                                                          │commit cab3598a2dad80e31872938ae7c29d2669f384bd (7.8.x)
nothing to commit, working tree clean                                                                    │Author: Hilary James Oliver <h.oliver@niwa.co.nz>
[oliverh@niwa-1007885 cylc-flow-7.8.x]$ python lib/cylc/tests/test_global_config.py                      │Date:   Thu Apr 9 10:20:12 2020 +1200
.                                                                                                        │
----------------------------------------------------------------------                                   │    Updated cylc-7 release process.
Ran 1 test in 0.004s                                                                                     │
                                                                                                         │commit 3c2bda66fbd1bd5adbabb5aca77f54bbed456d55
OK               

[kinow]

Ditto for me, just synced repo and imported in PyCharm, it runs fine.

[kinow]

@hjoliver did a few hacks on a branch, and pushed to my fork.

Running locally, the very first key that I get is 'task event handler retry delays' in that for loop. It appears to be the same in Travis. It fails the assertion (I added a print for host_item and key)

self.assertTrue(host_item == coercer(items[key], []),
                            "host_item=%s, key=%s" % (host_item, key))

and prints

>                               "host_item=%s, key=%s" % (host_item, key))
E                               AssertionError: host_item=[], key=task event handler retry delays

Locally, my host_item is different.

Not sure if you are able to guess anything from this @hjoliver ? I'm still not sure why Travis is unhappy :/

[kinow]

Tested on bionic (18.04 Ubuntu) but issue still happens there too. 🤔

[hjoliver]

Yeah, weird, I don't get it 😬

[hjoliver]

The unit test now passes on Travis 😌

[hjoliver]

Travis passed 🎉

[wxtim]

@wxtim - this is irrelevant for cylc 8, right?

YES!