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

parsec: catch section/option conflict in validation #5450

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

oliver-sanders
Copy link
Member

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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added bug Something is wrong :( small labels Apr 4, 2023
@oliver-sanders oliver-sanders added this to the cylc-8.1.3 milestone Apr 4, 2023
@oliver-sanders oliver-sanders self-assigned this Apr 4, 2023
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

I think it's worth considering whether the integration test is ok.

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

I'm happy with this -
I think it's big enough to be worth tagging @MetRonnie or @hjoliver (Who-ever gets there first) as second review.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Tested out a few examples.

I think some items can be tripped up earlier by the deprecation upgrader. E.g.

# flow.cylc
scheduling = 22
  File "~/cylc-flow-8.1/cylc/flow/scripts/validate.py", line 154, in wrapped_main
    cfg = WorkflowConfig(
  File "~/cylc-flow-8.1/cylc/flow/config.py", line 308, in __init__
    self.pcfg = RawWorkflowConfig(
  File "~/cylc-flow-8.1/cylc/flow/cfgspec/workflow.py", line 2042, in __init__
    self.loadcfg(fpath, "workflow definition")
  File "~/cylc-flow-8.1/cylc/flow/parsec/config.py", line 84, in loadcfg
    self.upgrader(sparse, title)
  File "~/cylc-flow-8.1/cylc/flow/cfgspec/workflow.py", line 1891, in upg
    u.upgrade()
  File "~/cylc-flow-8.1/cylc/flow/parsec/upgrade.py", line 200, in upgrade
    old = self.get_item(upg['old'])
  File "~/cylc-flow-8.1/cylc/flow/parsec/upgrade.py", line 107, in get_item
    item = item[key]
TypeError: string indices must be integers

Also I noticed the opposite (using a setting as a section) gives traceback too, seems like a low priority though

# flow.cylc
[scheduling]
    [[cycling mode]]
  File "~/cylc-flow-8.1/cylc/flow/scripts/validate.py", line 154, in wrapped_main
    cfg = WorkflowConfig(
  File "~/cylc-flow-8.1/cylc/flow/config.py", line 308, in __init__
    self.pcfg = RawWorkflowConfig(
  File "~/cylc-flow-8.1/cylc/flow/cfgspec/workflow.py", line 2042, in __init__
    self.loadcfg(fpath, "workflow definition")
  File "~/cylc-flow-8.1/cylc/flow/parsec/config.py", line 86, in loadcfg
    self.validate(sparse)
  File "~/cylc-flow-8.1/cylc/flow/parsec/config.py", line 96, in validate
    return self.validator(sparse, self.spec)
  File "~/cylc-flow-8.1/cylc/flow/parsec/validate.py", line 1130, in cylc_config_validate
    return CylcConfigValidator().validate(cfg_root, spec_root)
  File "~/cylc-flow-8.1/cylc/flow/parsec/validate.py", line 216, in validate
    cfg[key] = self.coercers[specval.vdr](value, keys + [key])
  File "~/cylc-flow-8.1/cylc/flow/parsec/validate.py", line 389, in coerce_str
    value = cls.strip_and_unquote(keys, value)
  File "~/cylc-flow-8.1/cylc/flow/parsec/validate.py", line 528, in strip_and_unquote
    if value.startswith(substr):
AttributeError: 'OrderedDictWithDefaults' object has no attribute 'startswith'

@oliver-sanders oliver-sanders marked this pull request as draft April 17, 2023 10:43
@oliver-sanders
Copy link
Member Author

Will haver a quick look to see what can be done for those cases.

@oliver-sanders oliver-sanders marked this pull request as ready for review April 19, 2023 13:00
@oliver-sanders
Copy link
Member Author

@MetRonnie, I've expanded this PR to cover the two additional cases you found.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Can't find a way to produce traceback anymore. Worst thing I could find was

# flow.cylc
[scheduling]
    [[max active cycle points]]
$ cylc validate Mist
WARNING - deprecated items were automatically upgraded in "workflow definition"
WARNING -  * (8.0.0) [scheduling]max active cycle points -> [scheduling]runahead limit - "n" -> "Pn"
WorkflowConfigError: bad runahead limit "POrderedDictWithDefaults()" for integer cycling type

But I think this is very niche and not worth addressing

cylc/flow/parsec/upgrade.py Outdated Show resolved Hide resolved
cylc/flow/parsec/validate.py Outdated Show resolved Hide resolved
cylc/flow/parsec/validate.py Outdated Show resolved Hide resolved
tests/functional/validate/76-section-section-transpose.t Outdated Show resolved Hide resolved
@wxtim
Copy link
Member

wxtim commented Apr 21, 2023

Just deconflicted CHANGES.md

Will merge once tests OK

One test failure - being dealt with elsewhere and unrelated to this pr

@wxtim wxtim merged commit 39159b8 into cylc:8.1.x Apr 21, 2023
@oliver-sanders oliver-sanders deleted the 4955 branch April 21, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsec: accidentally specifying sections as settings not handled properly
3 participants