Verdi: Warn for already existing code@computer label

[janssenhenning]

See #3303

Implement reprompting mechanism in verdi code setup and verdi code duplicate for code label/computer combinations that are already in the database. The Computer option gains callback checking the DB for the set code label/computer combination

If it is found the user is asked if he would like to change the label. If that is the case the label is set to None, which triggers a second label argument, which will perform the same check as the computer option but it will raise an error and reprompt instead. This second label argument is hidden=True, expose_value=False to avoid duplicate help messages and name clashes with the other label option

Works both for remote and local codes. For non-interactive commands an error is thrown

[codecov]

Codecov Report

Merging #5194 (ac19d17) into develop (a80812c) will increase coverage by 0.03%.
The diff coverage is 96.16%.

@@             Coverage Diff             @@
##           develop    #5194      +/-   ##
===========================================
+ Coverage    81.00%   81.02%   +0.03%     
===========================================
  Files          535      535              
  Lines        37401    37453      +52     
===========================================
+ Hits         30292    30342      +50     
- Misses        7109     7111       +2     

Flag	Coverage Δ
django	75.84% <96.16%> (+0.06%)	⬆️
sqlalchemy	74.95% <96.16%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
aiida/cmdline/params/options/commands/code.py	97.11% <96.00%> (-2.89%)	⬇️
aiida/cmdline/commands/cmd_code.py	89.69% <100.00%> (+0.10%)	⬆️

---

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 a80812c...ac19d17. Read the comment docs.

[ramirezfranciscof]

Hey @janssenhenning thanks for the contribution! I have started to look into the modifications and I have one general question before going more in depth: why did you need to create the LABEL_AFTER_DUPLICATE option? Is it just because the prompt for computer happens after the prompt for label and thus you can't do the check on the prompt you would need to re-trigger because you need the info of the other one?

If that is the only reason I would be inclined to suggest you just switch the order and have the computer be the first thing to be prompted. This would also require to change the "Installed on target computer?" question too, but I think it makes sense to ask this first, then the computer/path and then the label. I can check with the other devs if they see any problem with this change, but would this solve the issue of the duplicated option?

[janssenhenning]

Hi @ramirezfranciscof,

Yes, If I remember correctly this was the only reason for the LABEL_AFTER_DUPLICATE option, since the first label option cannot check for the existence since the computer is missing and I don't think it is possible to reprompt a option after it was already validated from a later option.

Should I update the PR to change the order or should I wait if there are any problems with this?

[ramirezfranciscof]

Should I update the PR to change the order or should I wait if there are any problems with this?

I would say wait for just a bit, both so I can check this (although I'm pretty sure it would be ok), but more importantly because they are about to merge some changes in click (see PR #5111 ) and you may have to adapt something for that as well, so no point in doing double work.

[sphuber]

Thanks for the PR @janssenhenning . PR #5111 has now been merged. However, I am not sure that we should go in the direction of adding a new parameter. It makes things a lot more complex and I am hoping there is a simpler solution. Ideally the validation is done in a callback on the label parameter. However, the tricky part is that for that validator the computer value should be known, which can be retrieved from the context that is passed in, but then it needs to be guaranteed that computer is parsed before label. This is the tricky part, because as I remember from last time I looked into this, the order of parsing is not guaranteed by the order in which we define them, but possibly the order in which the parameters are specified by the caller. However, I did some experimenting and it looks like the order of prompting at least is guaranteed.

Consider the following test script simplifying the situation:

#!/usr/bin/env python
import click

from aiida.cmdline.params.options.interactive import InteractiveOption
from aiida.cmdline.params.types import ComputerParamType
from aiida.orm import Code, Computer, QueryBuilder


def validate_label(ctx, param, value):
    computer = ctx.params.get('computer', None)

    if computer is not None:
        query = QueryBuilder()
        query.append(Computer, filters={'id': computer.pk}, tag='computer')
        query.append(Code, with_computer='computer', filters={'label': value})

        if query.first() is not None:
            raise click.BadParameter(f'the code `{value}@{computer.label}` already exists.')

    return value


@click.option('--label', type=str, cls=InteractiveOption, prompt='Label', callback=validate_label)
@click.option('--computer', type=ComputerParamType(), cls=InteractiveOption, prompt='Computer')
@click.command()
def main(label, computer):
    print(label, computer)


if __name__ == '__main__':
    main()

It seems that by specifying computer last, it will always get prompted first. (Note this is only from observation and I didn't analyze the code to make sure this is actually guaranteed.) When trying it out (assuming that bash@localhost already exists):

(aiida_dev) sph@citadel:~/code/aiida/env/dev/aiida-core$ ./prompt_test.py 
Computer: localhost
Label: bash
Error: the code `bash@localhost` already exists.
Label: bash2
None localhost (localhost), pk: 5

It fails the validation and then reprompts, so this works. It also work in non-interactive mode:

(aiida_dev) sph@citadel:~/code/aiida/env/dev/aiida-core$ ./prompt_test.py --computer localhost --label bash
Usage: prompt_test.py [OPTIONS]
Try 'prompt_test.py --help' for help.

Error: Invalid value for '--label': the code `bash@localhost` already exists.

However, this is only because the --label is specified last. If we inverse the order, the validator for label is called before the computer has been parsed and so it is skipped (because of the if computer is not None conditional in the validator):

(aiida_dev) sph@citadel:~/code/aiida/env/dev/aiida-core$ ./prompt_test.py --label bash --computer localhost
None localhost (localhost), pk: 5

So this is the only remaining problem here. One possible solution is to manually call the validator again in the command body:

@click.option('--label', type=str, cls=InteractiveOption, prompt='Label', callback=validate_label)
@click.option('--computer', type=ComputerParamType(), cls=InteractiveOption, prompt='Computer')
@click.command()
@click.pass_context
def main(ctx, label, computer):
    validate_label(ctx, None, label)
    print(label, computer)

Now the validation is called independent of the order in which the parameters are specified:

(aiida_dev) sph@citadel:~/code/aiida/env/dev/aiida-core$ ./prompt_test.py --label bash --computer localhost
Usage: prompt_test.py [OPTIONS]
Try 'prompt_test.py --help' for help.

Error: Invalid value: the code `bash@localhost` already exists.

The only downside I can see in this approach is that it calls the validator twice, but the first time will be very cheap, so the overhead should be minimal.

What do you guys think? Is there a problem that I missed?

[sphuber]

There may be even a more elegant solution to the parsing order of the non-interactive case. There is the option is_eager=True which we could give to the --computer option ensuring it is parsed first. However, I remember this having some other potential problems. Will try this out to see if it also works in the more complex case of the actual verdi code setup.

[sphuber]

Worked out my idea in an alternative implementation in #5205 @janssenhenning and @ramirezfranciscof

[ramirezfranciscof]

Hey @janssenhenning , sorry but it seems that apparently there were some other details that needed to be sorted out together with this issue 😅 (see for example #5204) so we ended up having to take more personal control of this feature, I hope that is ok. We anyways appreciate a lot the contribution and the initiative to participate in this project! Feel free to check the merged PR (#5205) in case there is any aspect you find still missing from that (and if not, then I'll let you close this PR).

[sphuber]

@janssenhenning I had written a message here before merging #5205 but apparently that never made it to the server, not sure what happened.

Anyway, I wrote to thank you for your contribution and that the reason I opened an alternative PR was not my original intention. When @ramirezfranciscof asked me whether the suggested approach was feasible, I remembered having looked into this and found issues due to how click works, which I wrote in the post above. Since I thought that solution was a dead end, I didn't want to give the go ahead for you to start changing your PR, because you would end up doing a lot of work for nothing. Therefore I decided to create a quick MWE to demonstrate the problem I thought there to have been, but in doing so, figured out that actually it could be made to work. With some additional kinks to sort out, I ended up with the implementation done. At this point, it seemed more logical to just open a PR as opposed to have you change this one entirely by copy pasting the full implementation. Again, this was not to discourage you or be disrespectful. Your contributions would be very much appreciated and valuable.

[janssenhenning]

@ramirezfranciscof @sphuber Absolutely no problem. I had this lying around for quite some time. I used it to get some practice with click to use it in aiida-fleur and related packages.

Looking at #5205 what happens if you store the code in the database so --store-in-db? Then the computer option is None and the label can be duplicate again, right?

[sphuber]

You are absolutely right @janssenhenning that is something we missed. This distinction between "remote" and "local" codes has always been confusing, especially when you have "remote" codes on localhost...

I have opened #5215 with a potential fix. It would be great if you could have a look and see if you can find any problems.