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

Get Cylc template variables from database for validation. #5189

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Oct 11, 2022

Allowed "revalidation" of cylc workflows where template variables are collected from

Probably closes #4740

database of already played workflows.

  • Added --revaladidate as an option to the following scripts (Made async to allow testing of changes):
    • cylc validate
    • cylc view
    • cylc graph
    • cylc config

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).
  • Tests are included.
  • 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.

@wxtim wxtim added this to the cylc-8.1.0 milestone Oct 11, 2022
@wxtim wxtim self-assigned this Oct 11, 2022
@wxtim wxtim force-pushed the 20221010T0945--HEAD--get_template_vars_from_running_wf branch from 08db7c2 to a438418 Compare October 11, 2022 10:32
@wxtim wxtim changed the title Allowed "revalidation" of cylc workflows where template variables are… Get Cylc template variables from database for validation. Oct 11, 2022
@wxtim wxtim force-pushed the 20221010T0945--HEAD--get_template_vars_from_running_wf branch 3 times, most recently from 77823b7 to f7ba0b9 Compare October 11, 2022 14:35
@wxtim wxtim marked this pull request as draft October 11, 2022 14:40
@wxtim
Copy link
Member Author

wxtim commented Oct 11, 2022

Converted to draft because this change doesn't play nicely with Cylc Rose as it is. :(

Now working :)

@wxtim wxtim force-pushed the 20221010T0945--HEAD--get_template_vars_from_running_wf branch 2 times, most recently from 290d794 to 5935610 Compare October 12, 2022 11:50
… collected from

database of already played workflows.
- Added `--revaladidate` as an option to the following scripts
  (Made async to allow testing of changes):
  - cylc validate
  - cylc view
  - cylc graph
  - cylc config
@wxtim wxtim force-pushed the 20221010T0945--HEAD--get_template_vars_from_running_wf branch from 5935610 to b2e51e2 Compare October 12, 2022 11:56
@wxtim wxtim marked this pull request as ready for review October 12, 2022 15:07
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.

LGTM

Comment on lines 448 to 453
if self.revalidate:
self.add_std_option(
'--revalidate',
help="Get template variables from prevous workflow run.",
action='store_true', default=False
)
Copy link
Member

Choose a reason for hiding this comment

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

Were we going to infer this option if the provided workflow is a run directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@@ -251,7 +251,7 @@ def process_plugins(fpath, opts):
# If you want it to work on sourcedirs you need to get the options
# to here.
plugin_result = entry_point.resolve()(
srcdir=fpath, opts=opts
fpath, opts=opts
Copy link
Member

Choose a reason for hiding this comment

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

Note: Double check that cylc-rose is ok with this.

"""Extract key and value and run eval_var on them assigning
them to self.template_vars.
"""
self.template_vars[row[0]] = eval_var(row[1])
Copy link
Member

Choose a reason for hiding this comment

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

Note: check that eval_var doesn't require fancy error catching for its other uses in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you're asking me here.

tests/integration/plugins/test_get_old_tvars.py Outdated Show resolved Hide resolved
tests/integration/plugins/test_get_old_tvars.py Outdated Show resolved Hide resolved
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.

Quick skim, looks good.

cylc/flow/pre_configure/get_old_tvars.py Outdated Show resolved Hide resolved
cylc/flow/pre_configure/get_old_tvars.py Outdated Show resolved Hide resolved
cylc/flow/pre_configure/get_old_tvars.py Outdated Show resolved Hide resolved
cylc/flow/pre_configure/get_old_tvars.py Outdated Show resolved Hide resolved
cylc/flow/scripts/validate.py Outdated Show resolved Hide resolved
@wxtim wxtim marked this pull request as draft October 26, 2022 07:07
@wxtim
Copy link
Member Author

wxtim commented Oct 26, 2022

This will want rebasing on Cylc VIP branch after merge.

wxtim and others added 5 commits October 31, 2022 10:17
Co-authored-by: Hilary James Oliver <[email protected]>
… of github.com:wxtim/cylc into 20221010T0945--HEAD--get_template_vars_from_running_wf

* '20221010T0945--HEAD--get_template_vars_from_running_wf' of github.com:wxtim/cylc: (51 commits)
  make revalidate automatic if workflow has changed at source
  play: --upgrade and --downgrade options
  Fix consolidation tutorial (cylc#5199)
  8.0.x -> master (cylc#5206)
  GH Actions Codecov upload: try using token arg (cylc#5179)
  Update CHANGES.md [skip ci]
  Add type hints.
  Switch to better monkeypatching approach. [skip ci]
  Add explanatory comments. [skip ci]
  Update CHANGES.md [skip ci]
  play: check Cylc version on restart
  Use a context manager in new test.
  Update change log.
  Extend new test coverage.
  install: scan only target workflow
  Apply suggestions from code review
  Add integration test.
  Scan workflow name during install.
  fix an error handling bug in Cylc Scan
  update PR template (cylc#5202)
  ...
@wxtim
Copy link
Member Author

wxtim commented Nov 1, 2022

I'm leaving this in draft format - I prefer the approach I took in #5214 - and that will probably form the basis for the work on validation for reinstall.

@wxtim wxtim closed this Nov 16, 2022
@oliver-sanders oliver-sanders removed this from the cylc-8.1.0 milestone Nov 16, 2022
@wxtim wxtim deleted the 20221010T0945--HEAD--get_template_vars_from_running_wf branch December 1, 2022 13:16
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.

config: template variables specified to play should be loaded
3 participants