cylc vip #5121

wxtim · 2022-09-07T10:12:15Z

Replaces #5094

#3896 - Implements the validate install play (VIP) working practice. I think that it works in principle for the other working practices.

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.
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 vip cylc-doc#533.

datamel

This is running smoothly for me with basic examples. I will test passing various options and re-review. A couple of comments so far.

cylc/flow/scripts/validate_install_play.py

cylcvip

oliver-sanders

There a quite a few of these kicking about, worth running through the lot to make sure.

FYI: PyPa / flake8 / black now agree that operators should proceed statements rather than follow e.g:

# do this
foo = (
    1
    + 2
    - 3
    + 4
)

# not this
foo = (
    1 +
    2 -
    3 +
    4
)

Reads better.
Better association of operators.
Safer to add/remove lines.
Super obvious when an operator is missing.

For the same reason I prefer preceding whitespace:

foo = (
    'Lorel'
    ' ipsum'
    ' dolor.'
    '\nSid'
    ' amet'
    ' apising'
    ' ellet!'
)

Which makes it really easy to spot missing whitespace in text like this.

cylc/flow/scheduler_cli.py

datamel

Next lot of small comments from me.

cylc/flow/option_parsers.py

wxtim · 2022-09-30T07:10:23Z

There a quite a few of these kicking about, worth running through the lot to make sure.

The text version of what your highlighting? Not the operator example? (I found plenty of the former, but none of the latter (I also found a string which had become accidentally truncated)).

datamel

This is working well for me. A couple of small comments...
When cylc vip is not given any arguments, we get traceback with a TypeError, rather than the standard cylc: error: Wrong number of arguments (too few).

cylc/flow/scripts/validate_install_play.py

cylc/flow/scheduler_cli.py

oliver-sanders

I seem to be hitting a missing positional argument issue?

$ cylc vip
Traceback (most recent call last):
  File "~/mambaforge/envs/cylc.dev/bin/cylc", line 33, in <module>
    sys.exit(load_entry_point('cylc-flow', 'console_scripts', 'cylc')())
  File "/net~/cylc-flow/cylc/flow/scripts/cylc.py", line 657, in main
    execute_cmd(command, *cmd_args)    
  File "/net~/cylc-flow/cylc/flow/scripts/cylc.py", line 284, in execute_cmd
    entry_point.resolve()(*args)
  File "/net~/cylc-flow/cylc/flow/terminal.py", line 229, in wrapper                 
    wrapped_function(*wrapped_args, **wrapped_kwargs)
TypeError: main() missing 1 required positional argument: 'workflow_id'

cylc/flow/option_parsers.py

cylc/flow/scripts/validate_install_play.py

tests/functional/cylc-combination-scripts/00-vip.t

oliver-sanders

The OptionSettings approach is looking much cleaner / more intuitive for people adding new args 👍.

I think the kwarg constants (e.g. APPEND, HELP & DEST) can be removed now?

oliver-sanders · 2022-11-14T17:34:51Z

cylc/flow/option_parsers.py

+icp_option = Option(
+    *ICP_OPTION.args, **ICP_OPTION.kwargs)  # type: ignore[arg-type]


FYI you can provide args as kwargs and vice versa:

>>> def foo(a, b=2): ... print(a, b) ... >>> foo(1, b=3) 1 3 >>> foo(1, 3) 1 3 >>> foo(a=1, b=3) 1 3 >>> foo(b=3, a=1) 1 3

So you could potentially absorb the positional args into the kwargs for simplicity.

oliver-sanders · 2022-11-15T11:28:22Z

Testing going well so far. I've spotted one oddity in the logged command:

$ cylc vip generic --workflow-name foo --no-run-name
$ cylc validate ~/cylc-src/generic
Valid for cylc-8.1.0.dev
$ cylc install .
INSTALLED foo from ~/cylc-src/generic
$ cylc play foo

The logged output says cylc install . which is incorrect as PWD=$HOME at the time.

oliver-sanders · 2022-11-15T11:32:30Z

The install part of vip seems to be skipping the previous-run check.

If I install the workflow directly...

$ cylc install generic --workflow-name foo
INSTALLED foo/run3 from ~/cylc-src/generic
NOTE: 2 runs of "foo" are already active:
  ▶ foo/run1 vld601.cmpd1.metoffice.gov.uk:43118
  ▶ foo/run2 vld601.cmpd1.metoffice.gov.uk:43002
You can stop them all with:         
  cylc stop 'foo/*'                 
See "cylc stop --help" for options.

... any previous runs are listed. Whereas cylc vip doesn't display this:

$ cylc vip generic --workflow-name foo                                               
$ cylc validate ~/cylc-src/generic
Valid for cylc-8.1.0.dev
$ cylc install .
INSTALLED foo/run2 from ~/cylc-src/generic
$ cylc play foo
 
 ▪ ■  Cylc Workflow Engine 8.1.0.dev
 ██   Copyright (C) 2008-2022 NIWA
▝▘    & British Crown (Met Office) & Contributors
 
INFO - Extracting job.sh to ~/cylc-run/foo/run2/.service/etc/job.sh
foo/run2: vld601.cmpd1.metoffice.gov.uk PID=128496

This also means the --no-ping option is currently functionless in cylc vip.

Should try to get the previous-run-scanning working in cylc vip as the compound command makes it much easier to end up with multiple runs running in parallel.

wxtim · 2022-11-15T11:35:32Z

Should try to get the previous-run-scanning working in cylc vip as the compound command makes it much easier to end up with multiple runs running in parallel.

I think this is straightforward - I was looking at that code yesterday.

wxtim · 2022-11-15T11:37:42Z

The OptionSettings approach is looking much cleaner / more intuitive for people adding new args +1.

I think the kwarg constants (e.g. APPEND, HELP & DEST) can be removed now?

DEST is indeed no-longer required. The other two are still used. I've been able to remove ACTION, DEFAULT, METAVAR, CHOICES, OPTS, USEIF

cylc/flow/option_parsers.py

cylc/flow/scripts/validate_install_play.py

oliver-sanders · 2022-11-16T16:57:21Z

All looking good to me, will finish off tomorrow.

datamel

Small flake8 fix needed. I have checked this branch out and have been working with it for a while.
I have read the code and run the tests locally, I have run a bunch of manual tests, no problems from me found. I'm happy to approve once CI passing.

cylc/flow/option_parsers.py

Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>

wxtim · 2022-11-17T11:00:02Z

@datamel I found a new bug this very morning - would you care to check a22d978 too?

Did your testing include

cd ~/cylc-src/myworkflow
cylc vip

datamel · 2022-11-17T11:44:34Z

@datamel I found a new bug this very morning - would you care to check a22d978 too?

Did your testing include
cd ~/cylc-src/myworkflow
cylc vip

For info. This was tested by me but my setup uses localhost as the run host so it worked without error for me!
Retesting this change now.

datamel

I've double checked the remote re-invocation is working correctly. Thanks @wxtim, great change, super handy one to have in!

oliver-sanders

Sorted 🚀!

I especially love how fast this is compared to rose suite-ruin!

oliver-sanders · 2022-11-17T18:03:58Z

tests/functional/cylc-combination-scripts/00-vip.t

+cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/flow.cylc" .
+cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/reference.log" .


Suggested change

cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/flow.cylc" .

cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/reference.log" .

cp "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/flow.cylc" .

cp "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/reference.log" .

oliver-sanders · 2022-11-17T18:04:31Z

tests/functional/database/04-lock-recover/bin/cylc-db-lock

@@ -10,7 +10,7 @@ def main():
        os.path.join(os.getenv("CYLC_WORKFLOW_RUN_DIR"), "log", "db"))
    lockf(handle, LOCK_SH)
    call([
-        "cylc", "task", "message", "I have locked the public database file"])
+        "cylc", "task-message", "I have locked the public database file"])


Suggested change

"cylc", "task-message", "I have locked the public database file"])

"cylc", "message", "I have locked the public database file"])

oliver-sanders · 2022-11-17T18:04:38Z

tests/functional/database/05-lock-recover-100/bin/cylc-db-lock

@@ -11,7 +11,7 @@ def main():
        os.path.join(os.getenv("CYLC_WORKFLOW_RUN_DIR"), "log", "db"))
    lockf(handle, LOCK_SH)
    call([
-        "cylc", "task", "message", "I have locked the public database file"])
+        "cylc", "task-message", "I have locked the public database file"])


Suggested change

"cylc", "task-message", "I have locked the public database file"])

"cylc", "message", "I have locked the public database file"])

wxtim requested review from datamel and oliver-sanders September 7, 2022 10:12

wxtim force-pushed the implement.VIP branch 2 times, most recently from fc088e7 to 9e821ec Compare September 7, 2022 10:22

wxtim self-assigned this Sep 7, 2022

wxtim added this to the cylc-8.1.0 milestone Sep 7, 2022

This was referenced Sep 7, 2022

cylc vip cylc/cylc-doc#533

Merged

2022 Cylc Meetings cylc/cylc-admin#143

Closed

datamel reviewed Sep 27, 2022

View reviewed changes

cylc/flow/scripts/validate_install_play.py Outdated Show resolved Hide resolved

cylcvip Outdated Show resolved Hide resolved

wxtim requested a review from datamel September 27, 2022 12:50

oliver-sanders reviewed Sep 29, 2022

View reviewed changes

cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved

cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved

datamel reviewed Sep 29, 2022

View reviewed changes

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

wxtim commented Sep 30, 2022

View reviewed changes

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

wxtim commented Sep 30, 2022

View reviewed changes

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

wxtim commented Sep 30, 2022

View reviewed changes

cylc/flow/option_parsers.py Show resolved Hide resolved

oliver-sanders mentioned this pull request Oct 5, 2022

config: template variables specified to play should be loaded #4740

Closed

datamel reviewed Oct 11, 2022

View reviewed changes

cylc/flow/scripts/validate_install_play.py Show resolved Hide resolved

cylc/flow/scheduler_cli.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Oct 20, 2022

View reviewed changes

wxtim force-pushed the implement.VIP branch from f30a524 to 01071f7 Compare October 24, 2022 07:27

wxtim requested review from oliver-sanders and datamel October 24, 2022 07:28

wxtim force-pushed the implement.VIP branch from 26363e4 to 00593a7 Compare October 24, 2022 14:25

wxtim marked this pull request as draft October 25, 2022 09:07

wxtim force-pushed the implement.VIP branch 5 times, most recently from a23a00c to ba5cd5e Compare October 25, 2022 13:25

wxtim marked this pull request as ready for review October 26, 2022 06:59

cylc vip

58228c6

wxtim force-pushed the implement.VIP branch from e99eb7d to 58228c6 Compare November 11, 2022 13:53

oliver-sanders reviewed Nov 14, 2022

View reviewed changes

oliver-sanders mentioned this pull request Nov 15, 2022

play: rename -n short option #5227

Closed

wxtim added 2 commits November 15, 2022 12:55

remove unwanted constants

db21b94

VIP uses correct install interface

cf8fd82

wxtim requested a review from oliver-sanders November 15, 2022 13:44

minor fix

6bcbf66

wxtim mentioned this pull request Nov 16, 2022

cylc vr #5229

Merged

8 tasks

oliver-sanders reviewed Nov 16, 2022

View reviewed changes

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Nov 16, 2022

View reviewed changes

cylc/flow/scripts/validate_install_play.py Show resolved Hide resolved

response to review;pruned unwanted code;made test check implicit install

fe4597b

wxtim requested a review from oliver-sanders November 17, 2022 08:32

datamel reviewed Nov 17, 2022

View reviewed changes

cylc/flow/option_parsers.py Outdated Show resolved Hide resolved

Update cylc/flow/option_parsers.py

5e81153

Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>

wxtim force-pushed the implement.VIP branch from 1e57247 to a22d978 Compare November 17, 2022 10:15

wxtim requested a review from datamel November 17, 2022 10:57

add workflow_id to sysargv for play step if it isn't there

48fc3ac

wxtim force-pushed the implement.VIP branch from a22d978 to 48fc3ac Compare November 17, 2022 10:58

datamel approved these changes Nov 17, 2022

View reviewed changes

oliver-sanders approved these changes Nov 17, 2022

View reviewed changes

oliver-sanders merged commit ecfdd88 into cylc:master Nov 17, 2022

wxtim deleted the implement.VIP branch December 1, 2022 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cylc vip #5121

cylc vip #5121

wxtim commented Sep 7, 2022 •

edited by hjoliver

Loading

datamel left a comment

oliver-sanders left a comment •

edited

Loading

datamel left a comment

wxtim commented Sep 30, 2022 •

edited

Loading

datamel left a comment

oliver-sanders left a comment

oliver-sanders left a comment •

edited

Loading

oliver-sanders Nov 14, 2022

oliver-sanders commented Nov 15, 2022

oliver-sanders commented Nov 15, 2022 •

edited

Loading

wxtim commented Nov 15, 2022

wxtim commented Nov 15, 2022

oliver-sanders commented Nov 16, 2022

datamel left a comment

wxtim commented Nov 17, 2022 •

edited

Loading

datamel commented Nov 17, 2022

datamel left a comment

oliver-sanders left a comment

oliver-sanders Nov 17, 2022

oliver-sanders Nov 17, 2022

oliver-sanders Nov 17, 2022

		icp_option = Option(
		ICP_OPTION.args, *ICP_OPTION.kwargs) # type: ignore[arg-type]

		cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/flow.cylc" .
		cp -r "${TEST_SOURCE_DIR}/${TEST_NAME_BASE}/reference.log" .

	"cylc", "task-message", "I have locked the public database file"])
	"cylc", "message", "I have locked the public database file"])

cylc vip #5121

cylc vip #5121

Conversation

wxtim commented Sep 7, 2022 • edited by hjoliver Loading

datamel left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

datamel left a comment

Choose a reason for hiding this comment

wxtim commented Sep 30, 2022 • edited Loading

datamel left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders Nov 14, 2022

Choose a reason for hiding this comment

oliver-sanders commented Nov 15, 2022

oliver-sanders commented Nov 15, 2022 • edited Loading

wxtim commented Nov 15, 2022

wxtim commented Nov 15, 2022

oliver-sanders commented Nov 16, 2022

datamel left a comment

Choose a reason for hiding this comment

wxtim commented Nov 17, 2022 • edited Loading

datamel commented Nov 17, 2022

datamel left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders Nov 17, 2022

Choose a reason for hiding this comment

oliver-sanders Nov 17, 2022

Choose a reason for hiding this comment

oliver-sanders Nov 17, 2022

Choose a reason for hiding this comment

wxtim commented Sep 7, 2022 •

edited by hjoliver

Loading

oliver-sanders left a comment •

edited

Loading

wxtim commented Sep 30, 2022 •

edited

Loading

oliver-sanders left a comment •

edited

Loading

oliver-sanders commented Nov 15, 2022 •

edited

Loading

wxtim commented Nov 17, 2022 •

edited

Loading