-
Notifications
You must be signed in to change notification settings - Fork 93
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
Cylc install #4000
Cylc install #4000
Conversation
Looks impressive @datamel 🎉 |
Dammit I totally forgot we were approaching PR 4000! |
IMO this is why Mel waited for a few days to put up the draft PR! |
The bash docker test failures are nothing to worry about, those tests pass locally, likely that the test configuration requires a bit of a poke. Looking at the GH actions --- a/tests/functional/lib/bash/test_header
+++ b/tests/functional/lib/bash/test_header
@@ -430,7 +430,7 @@ install_suite() {
SUITE_RUN_DIR="${RUN_DIR}/${SUITE_NAME}"
mkdir -p "${TEST_DIR}/${SUITE_NAME}/" # make source dir
cp -r "${TEST_SOURCE_DIR}/${TEST_SOURCE_BASE}/"* "${TEST_DIR}/${SUITE_NAME}/"
- cylc install --no-run-name --flow-name="${SUITE_NAME}" --directory="${TEST_DIR}/${SUITE_NAME}" 2>'/dev/null'
+ cylc install --no-run-name --flow-name="${SUITE_NAME}" --directory="${TEST_DIR}/${SUITE_NAME}"
cd "${TEST_DIR}/${SUITE_NAME}/"
} It is possible that is might be installing the files in the wrong place? We run as |
The cylc-rose stuff has been merged now, should be straight forward to merge the two
|
Took a crack, this seems to do the job: diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index 5011996d2..f773ffdc4 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -58,9 +58,13 @@ multiple workflow run directories that link to the same suite definition.
"""
+import os
+import pkg_resources
+
from cylc.flow.option_parsers import CylcOptionParser as COP
-from cylc.flow.suite_files import install_workflow
from cylc.flow.terminal import cli_function
+from cylc.flow.pathutil import get_workflow_run_dir
+from cylc.flow.suite_files import install_workflow
def get_option_parser():
@@ -116,14 +120,30 @@ def get_option_parser():
def main(parser, opts, flow_name=None, src=None):
if opts.no_run_name and opts.run_name:
parser.error(
- """options --no-run-name and --run-name are mutually exclusive.
+ """opts --no-run-name and --run-name are mutually exclusive.
Use one or the other""")
- install_workflow(
+
+ for entry_point in pkg_resources.iter_entry_points(
+ 'cylc.pre_configure'
+ ):
+ entry_point.resolve()(opts.source)
+
+ flow_name = install_workflow(
flow_name=opts.flow_name,
source=opts.source,
run_name=opts.run_name,
no_run_name=opts.no_run_name,
- no_symlinks=opts.no_symlinks)
+ no_symlinks=opts.no_symlinks
+ )
+
+ for entry_point in pkg_resources.iter_entry_points(
+ 'cylc.post_install'
+ ):
+ entry_point.resolve()(
+ dir_=opts.source or os.getcwd(), # move the source dir logic into this script
+ opts=opts,
+ dest_root=get_workflow_run_dir(flow_name)
+ )
if __name__ == "__main__": |
b3be897
to
b66a765
Compare
This might be the answer to the bash test failures: diff --git a/dockerfiles/bash/Dockerfile b/dockerfiles/bash/Dockerfile
index 8b9f6a2e1..2396924a1 100644
--- a/dockerfiles/bash/Dockerfile
+++ b/dockerfiles/bash/Dockerfile
@@ -13,6 +13,7 @@ RUN apt-get update && apt-get install -y \
python3.7-dev \
sqlite3 \
wget \
+ rsync \
&& python3.7 -m pip install -U pip setuptools \
&& rm -rf /var/lib/apt/lists/* |
return rsync_cmd | ||
|
||
|
||
def install_workflow(flow_name=None, source=None, run_name=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can break this function down a bit, would make it possible to unit test bits of of (e.g. the runN bit).
Something seems to have angered the tests there. Lots of this diff in the logs:
|
Just looking into it now - angering the tests is my speciality! |
06deb04
to
ea288a5
Compare
Looking at the timestamps in the logs none of the four cancelled jobs from the last run produced any output in the minutes before being cancelled. This suggests that some tests are hanging. Unfortunately it can be quite hard to work out which of the tests are hanging (I have an idea on this I developed for the Rose tests, will try some time) but you can always try locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given a test locally, works well.
I found one quirk where you can end up with a combination of custom and automatically named runs:
$ cylc install --run-name a
$ cylc install
$ tree ~/cylc-run -L 2
~/cylc-run/
└── testy
├── _cylc-install
├── b
├── run1
└── runN -> ~/cylc-run/testy/run1
I think the correct behaviour should be that the second install would fail, we can tell that b
is an installed flow not a run because ~/cylc-run/testy/b
contains a flow.cylc
file.
Are we happy for someone to do a named run after a named run? Is it only mix of named and run numbered installs we want to avoid? e.g.
|
Yes, because the meaning of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests!
GH actions is angry again, looks like all tests are failing for the same reason at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cylc install, prohibit source in cylc-run
Add cylc-install to cylc-clean
Add functional tests for cylc-install
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Fix functional test for cylc-install
Fix get_flow_file Fix unit test
* abort if no flow is to be purged * only wait on lsof for a maximum of five tries
Update yml timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a lot of nuances and Cylc history has been blasted aside here, nice.
There are some minor quirks I think we should skip over for this review and come back to later:
- The argument of
cylc install
can be reg/path.- Come back to this later (universal id stuff).
- The
flow.cylc
is being created in the "source" dir andrsync
'ed to the "run" dir.- It may be better not to create the symlink in the "source" dir.
- This will silence future warnings.
- Users should just
mv suite.rc flow.cylc
.
- The runN symlinks are absolute rather than relative.
- This blocks registration changes, but perhaps that's not the worst thing?
yeah brilliant, thanks @datamel 🎉 I'll have a close look as soon as possible... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, no problems found 🎉 🎉
Cylc Install command added. These changes close #3889
Cylc can now install workflows from from arbitrary locations into the cylc-run directory.
This PR includes:
log/install/<timestamp>-install.log
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.