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

Fix param expansion in inherited environment. #4248

Merged
merged 6 commits into from
Jun 14, 2021

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jun 8, 2021

These changes close #4242

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).
  • No dependency changes.
  • Appropriate tests are included (extended an existing functional test).
  • Appropriate change log entry included.
  • No documentation update required (this branch conforms to advertised behaviour).

@hjoliver hjoliver self-assigned this Jun 8, 2021
@hjoliver hjoliver added this to the cylc-8.0b2 milestone Jun 8, 2021
@hjoliver hjoliver added the bug Something is wrong :( label Jun 8, 2021
@hjoliver hjoliver requested a review from kinow June 8, 2021 23:41
@hjoliver
Copy link
Member Author

hjoliver commented Jun 8, 2021

This is ready to go now.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Code and tests look good! Pending only CI to pass 👍

@kinow
Copy link
Member

kinow commented Jun 9, 2021

mypy error fixed in the merged #4251

@hjoliver
Copy link
Member Author

hjoliver commented Jun 9, 2021

Rebased.

@kinow
Copy link
Member

kinow commented Jun 9, 2021

@hjoliver can we ignore the macos failure?

2021-06-09T01:59:36.3004720Z TOTAL                                                20623   7211   8643      0    75.36%
2021-06-09T01:59:36.3005000Z 
2021-06-09T01:59:36.3005400Z =========================== short test summary info ============================
2021-06-09T01:59:36.3006860Z FAILED tests/integration/test_scan.py::test_scan_sigstop - AssertionError: as...
2021-06-09T01:59:36.3007580Z ================== 1 failed, 86 passed, 24 warnings in 33.58s ==================
2021-06-09T01:59:36.2794800Z E           AssertionError: assert [(20, '[one.1...05056910e93')] == [(30, 'Workfl...05056910e93')]
2021-06-09T01:59:36.2796570Z E             At index 0 diff: (20, '[one.1] -released from runahead') != (30, 'Workflow not running: 46dcaa6a-c8c6-11eb-8e68-005056910e93')
2021-06-09T01:59:36.2798370Z E             Left contains one more item: (30, 'Workflow not running: 46dcaa6a-c8c6-11eb-8e68-005056910e93')
2021-06-09T01:59:36.2799580Z E             Full diff:
2021-06-09T01:59:36.2800190Z E               [
2021-06-09T01:59:36.2800750Z E             +  (20,
2021-06-09T01:59:36.2801840Z E             +   '[one.1] -released from runahead'),
2021-06-09T01:59:36.2802600Z E                (30,
2021-06-09T01:59:36.2803850Z E                 'Workflow not running: 46dcaa6a-c8c6-11eb-8e68-005056910e93'),
2021-06-09T01:59:36.2804740Z E               ]

@hjoliver
Copy link
Member Author

hjoliver commented Jun 9, 2021

Yeah that MacOS scan test is failing on most PRs now, nothing to do with this change.

Issue for it: #4243

And ... the fix just got merged minutes ago: #4244

I'll rebase this again

cylc/flow/config.py Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Sadly this didn't work for my example workflow, took a bit of staring before I found the edge case that was causing the bug.

On this branch the following example fails:

[task parameters]
    a = 1..3
    b = 1

[scheduling]
    [[graph]]
        R1 = x<b>

[runtime]
    [[FAM<a>]]
        [[[environment]]]
            param = %(a)s

    [[x<b=1>]]
        inherit = FAM<a=1>
        script = test $param -eq 1

    [[x<b>]]
        pre-script = echo pre

However, if you comment out the last task:

    # [[x<b>]]
    #    pre-script = echo pre

It will pass on this branch (but not on master).

@hjoliver
Copy link
Member Author

God damn it. Sadly indeed!

@hjoliver
Copy link
Member Author

It certainly works for your original example, on #4242

@hjoliver
Copy link
Member Author

Interestingly, it works if I swap the order:

    [[x<b>]]
        pre-script = echo pre

    [[x<b=1>]]
        inherit = FAM<a=1>
        script = test $param -eq 1

@hjoliver
Copy link
Member Author

Fixed now, and test extended to cover this case.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

My example now passes, thanks.

@oliver-sanders oliver-sanders merged commit 8612c02 into cylc:master Jun 14, 2021
@hjoliver hjoliver deleted the fix-param-inherit branch June 15, 2021 21:25
wxtim added a commit to wxtim/cylc that referenced this pull request Jun 21, 2021
* 'master' of https://github.com/cylc/cylc:
  config: make --sparse the defalt
  flake8: simplify
  remove global.cylc example files
  tidy up old scripts
  Fix param expansion in inherited environment. (cylc#4248)
  Fix play help text.
  Trivial warning message change.
  Remove debug line.
  Apply suggestions from code review
  actions: check contributor list in the release process
  actions: check shortlog
  entry points: centralise entry point iteration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parameters: inheriting from unrelated families
3 participants