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

Allow src dir in run dir #4861

Merged
merged 5 commits into from
May 13, 2022
Merged

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented May 6, 2022

This is a (very) small change with no associated issue - it just removes a check on source dir location.

Currently cylc install refuses to install a source directory that is located under ~/cylc-run, but I don't think there's a good technical reason for that, is there?? We probably just thought it indicated user CLI error or messy housekeeping practices.

Note there's an additional check that a source dir does not contain _cylc_install and log dirs etc., which should be sufficient for catching the primary CLI error we're presumably trying to catch (i.e. referring to run directory as a source directory).

Unfortunately the location check prevents a sub-worfklow source dir installed as part of a main workflow, from being used as the cylc install source for sub-workflow instances at run time.

Sub-workflow instances can of course be manually installed by simply copying the source from the main workflow run directory each time, but then we don't get the benefits of cylc install and cylc reinstall.

With this check disabled, I have everything I need to get (non-nested) sub-workflows working rather nicely, e.g. with the ability to fix and continue a broken sub-workflow or run a new instance of it by re-triggering the main-workflow task - but I'll demo that next week.

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).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Does not need tests
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver self-assigned this May 6, 2022
@hjoliver hjoliver requested a review from oliver-sanders May 6, 2022 11:56
@oliver-sanders oliver-sanders added this to the cylc-8.0rc3 milestone May 6, 2022
@MetRonnie
Copy link
Member

MetRonnie commented May 6, 2022

I think a test that you can install from a source dir that is inside ~/cylc-run would be useful.

And perhaps a test that you can't do that if the source dir was itself cylc installed

@hjoliver hjoliver force-pushed the allow-src-dir-in-run-dir branch from 9e624e6 to 55b0f54 Compare May 13, 2022 05:01
@hjoliver
Copy link
Member Author

I think a test that you can install from a source dir that is inside ~/cylc-run would be useful.

And perhaps a test that you can't do that if the source dir was itself cylc installed

👍 done

@hjoliver
Copy link
Member Author

@MetRonnie - I'll merge this, as the last item on the rc3 milestone. Perhaps you can do a quick post-merge sanity check on the two unit tests added in response to your comment though.

@hjoliver hjoliver merged commit 592b483 into cylc:master May 13, 2022
@hjoliver hjoliver deleted the allow-src-dir-in-run-dir branch May 13, 2022 05:08
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.

✔️

(src_dir / WorkflowFiles.FLOW_FILE).touch()
validate_source_dir(src_dir, 'dieter')
# Test that src dir is not allowed to be an installed dir.
src_dir = cylc_run_dir / 'ajay'
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe, I hope so 🤣

@hjoliver
Copy link
Member Author

[UPDATE] For the record, this superseded the aborted "support for sub-workflows" PRs #4811 #4821. Allowing source dirs under cylc-run allows proper installation of sub-workflows that belong to a parent workflow; beyond that we're leaving it to external scripts to manage sub-workflow names and run directories.

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.

3 participants