-
Notifications
You must be signed in to change notification settings - Fork 92
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
Infer run number & centralised reg parse #4285
Conversation
3818764
to
53988d1
Compare
One consequence of this change is that the "offline" commands no longer accept paths to a file relative to PWD. Any relative path is interpreted as relative to |
53988d1
to
bc72520
Compare
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.
One consequence of this change is that the "offline" commands no longer accept paths to a file relative to PWD.
...
Does this need to be changed back to the original behaviour?
Hmm, it is kinda nasty accepting a path that is either relative to $PWD
or ~/cylc-run
, however, this has worked for us before. This interface accepts .
which is interpreted relative to $PWD
so not accepting ./foo/
or foo/
would be counter intuitive.
Options:
- Don't allow paths relative to
$PWD
but make an exception for.
(implemented. - Allow paths relative to
$PWD
, check$PWD
before~/cylc-run
. - Allow paths relative to
$PWD
, check~/cylc-run
before$PWD
. - Allow paths relative to
$PWD
but enforce them to begin with.
(e.g../foo/
).
Cons:
1: Counter intuitive.
$ cd cylc-src/foo
$ cylc validate .
ok
$ cd cylc-src
$ cylc validate ./foo
error
2&3: Has the potential for unexpected results:
$ cd ~/cylc-src/
$ cylc install foo
$ cylc validate foo # is this validating the src or the run copy?
4: Counter intuitive.
$ cd ~/cylc-src
$ ls foo
flow.cylc
$ cylc validate foo
error
@@ -548,5 +564,5 @@ def get_random_platform_for_install_target( | |||
|
|||
def get_localhost_install_target() -> str: | |||
"""Returns the install target of localhost platform""" | |||
localhost = platform_from_name() | |||
localhost = get_platform() |
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.
@wxtim can you check this.
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.
This is just changing it back to how it was before; both identical as can be seen from def get_platform(task_conf=None, task_id=UNKNOWN_TASK):
cylc-flow/cylc/flow/platforms.py
Lines 85 to 87 in bc72520
if task_conf is None or isinstance(task_conf, str): | |
# task_conf is a platform name, or get localhost if None | |
return platform_from_name(task_conf) |
The reason I had previously changed it to platform_from_name()
was because mypy was complaining that localhost
might be None
due to the Optional
return of get_platform()
. However this now is fixed by the use of @overload
to confirm that if task_conf
is None
, the return value will definitely be a dictionary
cylc-flow/cylc/flow/platforms.py
Lines 43 to 47 in bc72520
@overload | |
def get_platform( | |
task_conf: Union[str, None] = None, task_id: str = UNKNOWN_TASK | |
) -> Dict[str, Any]: | |
... |
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.
I've just found out about a nasty omission in mypy's checking of @overload
ed function implementations: python/mypy#9503
cylc/flow/scripts/edit.py
Outdated
def main(parser, options, *args): | ||
flow_file = parse_workflow_arg(options, args[0])[1] | ||
def main(parser: COP, options: 'Values', reg: str) -> None: | ||
flow_file = str(parse_reg(reg, src=True)[1]) |
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.
Is this str
needed?
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.
Not 100% sure but the variable was previously a string, so I think I did this to be safe in case something further down doesn't like using pathlib Paths instead of strings
cylc/flow/scripts/search.py
Outdated
curr_file = os.path.join(workflowdir, inc_file) | ||
line_no = inc_line_count | ||
else: | ||
curr_file = flow_file | ||
curr_file = str(flow_file) |
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.
Is this str
necessary?
5a3d081
to
eb9e3ce
Compare
eb9e3ce
to
1a9e119
Compare
@hjoliver I've tagged you in case you want to respond to any of the discussion on this, you don't need to review the code necessarily |
IMO it has to be 2. because non-absolute paths are expected be relative to |
Agreed, however, we could do with handling the ambiguity. It's easy enough to detect the cases where the user could be referring either to a flow relative to cylc-run or Slightly nasty as I don't think we have a way of forcing Cylc to pick the run copy in this eventuality. With universal-id they could achieve this by prefixing the user name on: $ cylc validate ~user/foo |
742f70a
to
c1205dc
Compare
fd1d6cc
to
c95baf5
Compare
c95baf5
to
fc44f5d
Compare
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.
Code looks good, thanks for the docstrings and for getting your head around what turned out to be a much more complex logic than expected!
Codacy has highlighted a few error cases which aren't covered by the tests, would be good to cover a few of them if possible.
Functional testing has mostly gone well, a few small issues:
- I think this is tricking
cylc clean
into thinking that the workflow is stopped.
To replicate try:
cylc install one; cylc play one; tree -a ~/cylc-run; cylc clean one; tree -a ~/cylc-run
This should confirm that the contact file is present when the clean command is run.
On a network filesystem I get the expected error when you remove a dir where a running process has an open file handle:
FileRemovalError: [Errno 39] Directory not empty: 'workflow'. This is probably a temporary issue with the filesystem, not a problem with Cylc.
- Traceback for
cylc clean
.
This might be unrelated, further attempts to clean the workflow in (1) resulted in traceback:
$ cylc cleal one
INFO - No workflow database - will only clean locally
INFO - Cleaning on local filesystem
Traceback (most recent call last):
...
OSError: [Errno 16] Device or resource busy: '.nfsc0000207034ffdad00000025'
I couldn't replicate this every time.
- Duplicate warnings for suite.rc deprecation
$ cylc validate ./whatever
WARNING - The filename 'suite.rc' is deprecated in favour of 'flow.cylc'
WARNING - The filename 'suite.rc' is deprecated in favour of 'flow.cylc'
(where whatever also exists in the cylc-run directory)
- Ambiguity where it isn't needed
(small issue, not a biggie, can punt to future work if it isn't a quick fix)
If I have the following setup:
~/
cylc-src/
whatever
cylc-run/
whatever
And try to validate the source workflow like so:
$ cd ~/cylc-src
$ cylc validate whatever
I get the expected warning due to ambiguity:
WARNING - Workflow files found in both ./whatever/suite.rc and /home/me/cylc-run/whatever/run1/flow.cylc. This command will use ./whatever/suite.rc.
However, if I prepend ./
to the flow name there shouldn't be any ambiguity any more, however, I still get the warning.
$ cd ~/cylc-src
$ cylc validate ./whatever
if not src: | ||
validate_workflow_name(reg) | ||
reg: Path = Path(expand_path(reg)) |
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.
Should the expand path come before the validate?
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.
I think validate_workflow_name()
is designed to validate what the user has typed verbatim, so we don't want to normalise/expand/etc it until after
cylc/flow/workflow_files.py
Outdated
REG_CLASH_MSG = ( | ||
"Workflow files found in both {0} and {1}. This command will use {0}." | ||
) |
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.
This message could be a little clearer.
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.
What do you have in mind?
I suppose I can change {1}
from being /home/blah/blah/user/cylc-run/<run_dir>
to just ~/cylc-run/<run_dir>
, but I'm guessing that's not what you mean
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.
Not sure I had anything particular in mind.
I think "Workflow files" surprised me a little, might not be so obvious to a user that this implies a reference ambiguity between a src and run dir.
{input} could mean {src_dir} or ~/cylc-run/{run_dir.relative_to(cylc_run_dir)}, using {src_dir} # ???
I suppose I can change {1} from being /home/blah/blah/user/cylc-run/<run_dir> to just ~/cylc-run/<run_dir>
Would be good.
This should be fixed by #4289
This seems to happen (on NFS) the second time |
Addressed in latest push
Will open a new issue |
d2dc35b
to
32f8f64
Compare
Confirmed as fixed.
That's fine, lets bump it to #3659 Although is this fix not related - MetRonnie@b02d74f?
Confirmed as fixed.
👍 |
I've found one more small issue, this time with the $ cylc install one
...
$ cylc install one
...
$ cylc reinstall one
WorkflowFilesError: Nested run directories not allowed - cannot install workflow name "one" as "/home/me/cylc-run/one/run1" is already a valid run directory. Although I'm not convinced that is necessarily to do with the reg parsing? Can bump if not related. |
Yes - I've been having a bit of a go at improving that
I can reproduce on master. ( |
Ok, I'll open a separate issue.
Ah, it should be because it's working with workflow identifiers (run dir only). |
In that case, could you add "make it use the centralised reg parse" to that issue please? |
But then we wouldn't be able to close the issues and would have mixed behaviour? Seems straightforward to convert, this did the trick for me: (note this also solves the traceback reported above due to inferring the run number) diff --git a/cylc/flow/scripts/reinstall.py b/cylc/flow/scripts/reinstall.py
index b25e2af23..1653b5861 100644
--- a/cylc/flow/scripts/reinstall.py
+++ b/cylc/flow/scripts/reinstall.py
@@ -45,9 +45,11 @@ from cylc.flow.exceptions import PluginError, WorkflowFilesError
from cylc.flow.option_parsers import CylcOptionParser as COP
from cylc.flow.pathutil import get_workflow_run_dir
from cylc.flow.workflow_files import (
+ WorkflowFiles,
get_workflow_source_dir,
+ parse_reg,
reinstall_workflow,
- WorkflowFiles)
+)
from cylc.flow.terminal import cli_function
if TYPE_CHECKING:
@@ -112,26 +114,20 @@ def get_option_parser():
@cli_function(get_option_parser)
def main(
- parser: COP, opts: 'Values', named_run: Optional[str] = None
+ parser: COP, opts: 'Values', workflow: str
) -> None:
- if not named_run:
- source, _ = get_workflow_source_dir(Path.cwd())
- if source is None:
- raise WorkflowFilesError(
- f'"{Path.cwd()}" is not a workflow run directory.')
- base_run_dir = Path(get_workflow_run_dir(''))
- named_run = str(Path.cwd().relative_to(base_run_dir.resolve()))
- run_dir = Path(get_workflow_run_dir(named_run))
+ workflow = parse_reg(workflow)
+ run_dir = Path(get_workflow_run_dir(workflow))
if not run_dir.exists():
raise WorkflowFilesError(
- f'"{named_run}" is not an installed workflow.')
+ f'"{workflow}" is not an installed workflow.')
if run_dir.name in [WorkflowFiles.FLOW_FILE, WorkflowFiles.SUITE_RC]:
run_dir = run_dir.parent
- named_run = named_run.rsplit('/', 1)[0]
+ workflow = workflow.rsplit('/', 1)[0]
source, source_path = get_workflow_source_dir(run_dir)
if not source:
raise WorkflowFilesError(
- f'"{named_run}" was not installed with cylc install.')
+ f'"{workflow}" was not installed with cylc install.')
if not Path(source).exists():
raise WorkflowFilesError(
f'Workflow source dir is not accessible: "{source}".\n'
@@ -153,7 +149,7 @@ def main(
) from None
reinstall_workflow(
- named_run=named_run,
+ named_run=workflow,
rundir=run_dir,
source=source,
dry_run=False # TODO: ready for dry run implementation Functional test alterations:
diff --git a/tests/functional/cylc-reinstall/00-simple.t b/tests/functional/cylc-reinstall/00-simple.t
index 83287a4bd..a5bb6c2c1 100644
--- a/tests/functional/cylc-reinstall/00-simple.t
+++ b/tests/functional/cylc-reinstall/00-simple.t
@@ -18,7 +18,7 @@
#------------------------------------------------------------------------------
# Test workflow re-installation
. "$(dirname "$0")/test_header"
-set_test_number 36
+set_test_number 18
# Test basic cylc reinstall, named run given
TEST_NAME="${TEST_NAME_BASE}-basic-named-run"
@@ -35,36 +35,6 @@ grep_ok "REINSTALLED ${RND_WORKFLOW_NAME}/run1 from ${RND_WORKFLOW_SOURCE}" "${R
popd || exit 1
purge_rnd_workflow
-# Test basic cylc reinstall, named run (including ``flow.cylc``) given
-TEST_NAME="${TEST_NAME_BASE}-flow-as-arg"
-make_rnd_workflow
-pushd "${RND_WORKFLOW_SOURCE}" || exit 1
-run_ok "${TEST_NAME}" cylc install
-run_ok "${TEST_NAME}-reinstall" cylc reinstall "${RND_WORKFLOW_NAME}/run1/flow.cylc"
-contains_ok "${TEST_NAME}.stdout" <<__OUT__
-INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
-__OUT__
-REINSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*reinstall.log')"
-grep_ok "REINSTALLED ${RND_WORKFLOW_NAME}/run1 from ${RND_WORKFLOW_SOURCE}" "${REINSTALL_LOG}"
-popd || exit 1
-purge_rnd_workflow
-
-# Test basic cylc reinstall, named run (including suite.rc) given
-TEST_NAME="${TEST_NAME_BASE}-suite.rc-as-arg"
-make_rnd_workflow
-pushd "${RND_WORKFLOW_SOURCE}" || exit 1
-rm -rf flow.cylc
-touch suite.rc
-run_ok "${TEST_NAME}" cylc install
-run_ok "${TEST_NAME}-reinstall-suite.rc" cylc reinstall "${RND_WORKFLOW_NAME}/run1/suite.rc"
-contains_ok "${TEST_NAME}.stdout" <<__OUT__
-INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
-__OUT__
-REINSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*reinstall.log')"
-grep_ok "REINSTALLED ${RND_WORKFLOW_NAME}/run1 from ${RND_WORKFLOW_SOURCE}" "${REINSTALL_LOG}"
-popd || exit 1
-purge_rnd_workflow
-
# Test install/reinstall executed from elsewhere in filesystem
TEST_NAME="${TEST_NAME_BASE}-named-flow"
make_rnd_workflow
@@ -116,38 +86,4 @@ grep_ok "The filename 'suite.rc' is deprecated in favour of 'flow.cylc'. Symlink
popd || exit 1
purge_rnd_workflow
-# Test cylc reinstall from within rundir, no args given
-TEST_NAME="${TEST_NAME_BASE}-no-args"
-make_rnd_workflow
-run_ok "${TEST_NAME}-install" cylc install --flow-name="${RND_WORKFLOW_NAME}" -C "${RND_WORKFLOW_SOURCE}"
-contains_ok "${TEST_NAME}-install.stdout" <<__OUT__
-INSTALLED ${RND_WORKFLOW_NAME}/run1 from ${RND_WORKFLOW_SOURCE}
-__OUT__
-pushd "${RND_WORKFLOW_RUNDIR}/run1" || exit 1
-touch "${RND_WORKFLOW_SOURCE}/new_file"
-run_ok "${TEST_NAME}-reinstall" cylc reinstall
-REINSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*reinstall.log')"
-grep_ok "REINSTALLED ${RND_WORKFLOW_NAME}/run1 from ${RND_WORKFLOW_SOURCE}" "${REINSTALL_LOG}"
-exists_ok new_file
-popd || exit 1
-purge_rnd_workflow
-
-# Test cylc reinstall from within rundir, no args given
-TEST_NAME="${TEST_NAME_BASE}-no-args-no-run-name"
-make_rnd_workflow
-pushd "${RND_WORKFLOW_SOURCE}" || exit 1
-run_ok "${TEST_NAME}-install" cylc install --no-run-name -C "${RND_WORKFLOW_SOURCE}"
-contains_ok "${TEST_NAME}-install.stdout" <<__OUT__
-INSTALLED ${RND_WORKFLOW_NAME} from ${RND_WORKFLOW_SOURCE}
-__OUT__
-pushd "${RND_WORKFLOW_RUNDIR}" || exit 1
-touch "${RND_WORKFLOW_SOURCE}/new_file"
-run_ok "${TEST_NAME}-reinstall" cylc reinstall
-REINSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/log/install" -type f -name '*reinstall.log')"
-grep_ok "REINSTALLED ${RND_WORKFLOW_NAME} from ${RND_WORKFLOW_SOURCE}" "${REINSTALL_LOG}"
-exists_ok new_file
-popd || exit 1
-popd || exit 1
-purge_rnd_workflow
-
exit
diff --git a/tests/functional/cylc-reinstall/02-failures.t b/tests/functional/cylc-reinstall/02-failures.t
index e81ae00a2..da327270a 100644
--- a/tests/functional/cylc-reinstall/02-failures.t
+++ b/tests/functional/cylc-reinstall/02-failures.t
@@ -18,20 +18,7 @@
#------------------------------------------------------------------------------
# Test workflow reinstallation expected failures
. "$(dirname "$0")/test_header"
-set_test_number 26
-
-# Test fails is there is a nested run directory structure in the run directory to be reinstalled
-
-TEST_NAME="${TEST_NAME_BASE}-nested-rundir-forbidden-reinstall"
-make_rnd_workflow
-run_ok "${TEST_NAME}-install" cylc install -C "${RND_WORKFLOW_SOURCE}" --flow-name="${RND_WORKFLOW_NAME}"
-mkdir "${RND_WORKFLOW_RUNDIR}/run1/nested_run_dir"
-touch "${RND_WORKFLOW_RUNDIR}/run1/nested_run_dir/flow.cylc"
-run_fail "${TEST_NAME}-reinstall-nested-run-dir" cylc reinstall "${RND_WORKFLOW_NAME}"
-contains_ok "${TEST_NAME}-reinstall-nested-run-dir.stderr" <<__ERR__
-WorkflowFilesError: Nested run directories not allowed - cannot install workflow name "${RND_WORKFLOW_NAME}" as "${RND_WORKFLOW_RUNDIR}/run1" is already a valid run directory.
-__ERR__
-purge_rnd_workflow
+set_test_number 20
# Test fail no workflow source dir
@@ -86,22 +73,6 @@ __ERR__
popd || exit 1
done
-# Test cylc reinstall (no args given) raises error when no source dir.
-TEST_NAME="${TEST_NAME_BASE}-reinstall-no-source-rasies-error"
-make_rnd_workflow
-pushd "${RND_WORKFLOW_SOURCE}" || exit 1
-run_ok "${TEST_NAME}-install" cylc install --no-run-name --flow-name="${RND_WORKFLOW_NAME}"
-pushd "${RND_WORKFLOW_RUNDIR}" || exit 1
-rm -rf "_cylc-install"
-run_fail "${TEST_NAME}-reinstall" cylc reinstall
-CWD=$(pwd -P)
-contains_ok "${TEST_NAME}-reinstall.stderr" <<__ERR__
-WorkflowFilesError: "${CWD}" is not a workflow run directory.
-__ERR__
-popd || exit 1
-popd || exit 1
-purge_rnd_workflow
-
# Test cylc reinstall (args given) raises error when no source dir.
TEST_NAME="${TEST_NAME_BASE}-reinstall-no-source-rasies-error2"
make_rnd_workflow |
5e0d240
to
a932ce6
Compare
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.
Have tested the reinstall changes, works great.
No more issues from testing, great improvement, good to go 🎉.
Excellent, I'm back reviewing this and will merge once tested. |
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.
I've read through the code, run the tests locally and had a manual test with the change. No problems found. Thanks @MetRonnie.
These changes close #4230 and close #3895
cylc.flow.workflow_files.parse_reg()
function provides a centralised way to process theREG
orWORKFLOW
arg in cylc commands.cylc pause foo/run3
you can docylc pause foo
assumingrun3
is the latest (also if you dofoo/runN
it processes this asfoo/run3
)parse_reg(reg, src=True)
. Withsrc=True
thereg
can be:flow.cylc
file (either absolute, or relative to PWD, or relative to~/cylc-run
(PWD takes precedence1))src=False
(the default). Withsrc=False
thereg
can be:~/cylc-run
only (the dir need not exist but it can't be a path to a file)Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.setup.py
andconda-environment.yml
.Footnotes
This is a change in behaviour. Previously it was the case that the path relative to
~/cylc-run
took precedence over PWD, apart from.
for PWD itself, strangely. See #4285 (comment) and #4285 (comment) ↩