`verdi code setup`: validate the uniqueness of the full label #5205

sphuber · 2021-10-28T14:00:43Z

The full label of a Code instance is defined as label@computer.label.
Recently, the uniqueness of the full label was not even checked and it
was possible to create codes with duplicate full labels. Since this was
fixed though, verdi code setup would only fail when all parameters are
parsed and it would try to store the code. In interactive mode, this is
pretty annoying as the command will not fail until all parameters have
been specified.

Here, a validator is added to the --label option to check the
uniqueness of the full label. In order to check this, it needs the
selected computer. To ensure the computer is parsed before the label, the
option is defined before the label when the command is defined. At least
for the interactive mode, this guarantees that the computer is prompted
for before the label, so the callback of the label can check for
uniqueness. If it fails, the user is shown the reason why and prompted
again.

Note that the parsing order of parameters in the non-interactive mode is
not guaranteed by the definition order but by the order in which they
are passed by the caller. Therefore, the callback of the label cannot be
relied upon, because if the label is specified first, the callback won't
know the computer and will have to exit. Therefore, the callback has to
be called manually once again in the body of the command when it is
guaranteed that both the label and computer will have been defined.
There is a slight overhead in the sense that the callback is called
twice, but the first call will exit almost instantly and so the overhead
will be minimal.

codecov · 2021-10-28T15:21:31Z

Codecov Report

Merging #5205 (10782f5) into develop (bb92dd5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5205      +/-   ##
===========================================
+ Coverage    81.20%   81.21%   +0.02%     
===========================================
  Files          532      532              
  Lines        37307    37322      +15     
===========================================
+ Hits         30290    30307      +17     
+ Misses        7017     7015       -2

Flag	Coverage Δ
django	`76.06% <100.00%> (+0.02%)`	⬆️
sqlalchemy	`75.16% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_code.py	`89.69% <100.00%> (+0.15%)`	⬆️
aiida/cmdline/params/options/commands/code.py	`100.00% <100.00%> (ø)`
aiida/engine/daemon/client.py	`76.27% <0.00%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb92dd5...10782f5. Read the comment docs.

sphuber · 2021-10-28T17:13:40Z

@ltalirz you might be interested in this one

ltalirz · 2021-10-29T12:23:43Z

Thanks @sphuber !

Recently, the uniqueness of the full label was not even checked and it
was possible to create codes with duplicate full labels. Since this was
fixed though, verdi code setup would only fail when all parameters are
parsed and it would try to store the code.

Just to check: this only happens when using verdi code setup, right?
What happens when importing a code with an existing computer+code label?

To ensure the computer is parsed before the label, the
option is defined before the label when the command is defined.

This could probably be clarified (I don't quite follow).

In general, this is a very useful change; I will go through the code now.

ltalirz

thanks for this @sphuber !

Just a suggestion for rephrasing the comments.

One thing to keep in mind is that this needs to be documented in the migration guide since some scripts may rely on the order of prompts in code setup (just as our tests do)

ltalirz · 2021-10-29T12:29:21Z

aiida/cmdline/commands/cmd_code.py

+# Note that the ``COMPUTER`` option needs to be specified before the ``LABEL`` option, since the latter has a callback
+# that relies on the former and so that needs to be prompted for first, which is the case if it is defined first. This
+# only works guaranteed for the interactive case, however, and so the callback needs to be called manually for the
+# non-interactive case in the case the label is specified before the computer and so when its callback is invoked, the
+# computer is not known yet and so cannot be validated.


Below my go at unwinding these sentences a bit

Suggested change

# Note that the ``COMPUTER`` option needs to be specified before the ``LABEL`` option, since the latter has a callback

# that relies on the former and so that needs to be prompted for first, which is the case if it is defined first. This

# only works guaranteed for the interactive case, however, and so the callback needs to be called manually for the

# non-interactive case in the case the label is specified before the computer and so when its callback is invoked, the

# computer is not known yet and so cannot be validated.

# Defining the ``COMPUTER`` option first guarantees that the user is prompted for the computer first.

# This is necessary because the ``LABEL`` option has a callback that relies on the computer being already set.

# Execution order is guaranteed only for the interactive case, however.

# For the non-interactive case, the callback is called explicitly once more to cover the case when the label is specified before the computer.

Yeah, that is better, thanks. I will adapt the comment.

ltalirz · 2021-10-29T12:30:09Z

aiida/cmdline/commands/cmd_code.py

@@ -97,13 +105,18 @@ def setup_code(non_interactive, **kwargs):
    echo.echo_success(f'Code<{code.pk}> {code.full_label} created')


+# Note that the ``COMPUTER`` option needs to be specified before the ``LABEL`` option, since the latter has a callback


same as above

sphuber · 2021-10-29T21:29:24Z

One thing to keep in mind is that this needs to be documented in the migration guide since some scripts may rely on the order of prompts in code setup (just as our tests do)

Sure, will add it to the guide, but then I will also add that people should use the --non-interactive mode with the normal flags if they are using a script and not rely on prompting, as that is not what it is intended for. Our tests only rely on it because they are actually testing the prompting behavior. Otherwise (such as the GHA setup scripts) they are using non-interactive mode.

The full label of a `Code` instance is defined as `label@computer.label`. Recently, the uniqueness of the full label was not even checked and it was possible to create codes with duplicate full labels. Since this was fixed though, `verdi code setup` would only fail when all parameters are parsed and it would try to store the code. In interactive mode, this is pretty annoying as the command will not fail until all parameters have been specified. Here, a validator is added to the `--label` option to check the uniqueness of the full label. In order to check this, it needs the selected computer. To ensure the computer is parsed before the label, the option is defined before the label when the command is defined. At least for the interactive mode, this guarantees that the computer is prompted for before the label, so the callback of the label can check for uniqueness. If it fails, the user is shown the reason why and prompted again. Note that the parsing order of parameters in the non-interactive mode is not guaranteed by the definition order but by the order in which they are passed by the caller. Therefore, the callback of the label cannot be relied upon, because if the label is specified first, the callback won't know the computer and will have to exit. Therefore, the callback has to be called manually once again in the body of the command when it is guaranteed that both the label and computer will have been defined. There is a slight overhead in the sense that the callback is called twice, but the first call will exit almost instantly and so the overhead will be minimal.

ltalirz

thanks @sphuber !

sphuber force-pushed the fix/3303/verdi-code-check-full-label-duplicate branch from fad194e to 856c3e7 Compare October 28, 2021 14:58

sphuber mentioned this pull request Oct 28, 2021

Verdi: Warn for already existing code@computer label #5194

Closed

ltalirz suggested changes Oct 29, 2021

View reviewed changes

sphuber force-pushed the fix/3303/verdi-code-check-full-label-duplicate branch from 856c3e7 to 10782f5 Compare November 1, 2021 08:23

sphuber requested a review from ltalirz November 1, 2021 08:31

ltalirz approved these changes Nov 1, 2021

View reviewed changes

sphuber merged commit d25339d into aiidateam:develop Nov 1, 2021

sphuber deleted the fix/3303/verdi-code-check-full-label-duplicate branch November 1, 2021 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`verdi code setup`: validate the uniqueness of the full label #5205

`verdi code setup`: validate the uniqueness of the full label #5205

sphuber commented Oct 28, 2021

codecov bot commented Oct 28, 2021 •

edited

Loading

sphuber commented Oct 28, 2021

ltalirz commented Oct 29, 2021

ltalirz left a comment

ltalirz Oct 29, 2021 •

edited

Loading

sphuber Oct 29, 2021

ltalirz Oct 29, 2021

sphuber commented Oct 29, 2021

ltalirz left a comment

		@@ -97,13 +105,18 @@ def setup_code(non_interactive, **kwargs):
		echo.echo_success(f'Code<{code.pk}> {code.full_label} created')


		# Note that the ``COMPUTER`` option needs to be specified before the ``LABEL`` option, since the latter has a callback

verdi code setup: validate the uniqueness of the full label #5205

verdi code setup: validate the uniqueness of the full label #5205

Conversation

sphuber commented Oct 28, 2021

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

sphuber commented Oct 28, 2021

ltalirz commented Oct 29, 2021

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

sphuber Oct 29, 2021

Choose a reason for hiding this comment

ltalirz Oct 29, 2021

Choose a reason for hiding this comment

sphuber commented Oct 29, 2021

ltalirz left a comment

Choose a reason for hiding this comment

`verdi code setup`: validate the uniqueness of the full label #5205

`verdi code setup`: validate the uniqueness of the full label #5205

codecov bot commented Oct 28, 2021 •

edited

Loading

ltalirz Oct 29, 2021 •

edited

Loading