Skip to content
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

verdi code setup should warn when setting up a code with existing label #3303

Closed
ltalirz opened this issue Sep 9, 2019 · 9 comments · Fixed by #5205
Closed

verdi code setup should warn when setting up a code with existing label #3303

ltalirz opened this issue Sep 9, 2019 · 9 comments · Fixed by #5205
Labels
good first issue Issues that should be relatively easy to fix also for beginning contributors topic/verdi

Comments

@ltalirz
Copy link
Member

ltalirz commented Sep 9, 2019

Currently, verdi code setup will happily set up codes with the same label multiple times.
It is fine that this is allowed in principle, but in interactive mode, a user should be warned when doing this.

The simplest might be something like:

$ verdi code setup
Info: enter "?" for help
Info: enter "!" to ignore the default and set no value
Label: cp
Description []: asdf
Default calculation input pluginquantumespresso.pw  
Installed on target computer? [True]: 
Computer: localhost
Warning: Code 'cp@localhost' already exists.
Remote absolute path:

If we want to be more fancy, we could add a prompt

Choose a different label [Y/n]: 
@ltalirz ltalirz added the good first issue Issues that should be relatively easy to fix also for beginning contributors label Feb 3, 2020
@ltalirz
Copy link
Member Author

ltalirz commented Oct 29, 2020

Along the same lines: when importing an archive file, it would make sense to relabel codes in the archive if a code of the same label already exists in the database (analogous to what is done for computers).
Otherwise, imports are likely to break scripts that rely on loading codes by label (which is a common practise).

@mbercx
Copy link
Member

mbercx commented Oct 30, 2020

Just out of curiosity: Would it not be better to disallow two codes with the same label altogether?

@ltalirz
Copy link
Member Author

ltalirz commented Oct 30, 2020

Would it not be better to disallow two codes with the same label altogether?

Once we implement the warning + renaming for verdi import, we're almost there in practice.
One problem might be that codes are represented as nodes, so I guess one cannot easily enforce uniqueness at the database schema level (while computers are not).

@ramirezfranciscof
Copy link
Member

One problem might be that codes are represented as nodes, so I guess one cannot easily enforce uniqueness at the database schema level (while computers are not).

But we could enforce uniqueness at the level of the node:code subclass no? Forcing it to check the DB for existing labels before creating (init?) and/or when changing the label property.

@greschd
Copy link
Member

greschd commented Nov 4, 2020

Just out of curiosity: Would it not be better to disallow two codes with the same label altogether?

In my opinion the code label doesn't need to be unique. For context, when testing a code across variations of our infrastructure I would distinguish them only by the computer. Of course it would be possible to add a -<computer_name> to the code label, but that seems silly given there's already the @<computer_name> syntax.

@ltalirz
Copy link
Member Author

ltalirz commented Nov 4, 2020

Yes, my comment seemed to imply that I was thinking of uniqueness for the code label only, but I agree that this discussion should focus on whether code-label@computer-label should be unique (not the code label alone, as @greschd rightfully points out)

@janssenhenning
Copy link
Contributor

Hello Aiida team,
I'm one of the developers of the aiida-fleur plugin and I want to also start making some contributions to aiida-core.
If this issue is still relevant I would be happy to work on it

@mbercx
Copy link
Member

mbercx commented Jul 16, 2021

Hi @janssenhenning! 👋 Welcome, happy to hear you want to work on this!

I definitely think it is still relevant! I guess we just have to agree on how to proceed. To continue the discussion, let me reply to @ltalirz's comment.

Yes, my comment seemed to imply that I was thinking of uniqueness for the code label only, but I agree that this discussion should focus on whether code-label@computer-label should be unique (not the code label alone, as @greschd rightfully points out)

Indeed, I agree that the code label alone should not need to be unique. However, I think we should enforce only having one Code with a specific label on each computer, so we'll never have 3 add@localhost code installed (as definitely happened during the tutorial).

@janssenhenning
Copy link
Contributor

I started trying to implement the mentioned warning mechanism for verdi code setup and verdi code duplicate for duplicate code_label@computer_label on my fork. I encountered a couple of things I'm not sure about:

  • What should happen when the command is called in non-interactive mode? Should the command exit with an error or should it be allowed?
  • I did this by adding an additional callback to the computer option, which then triggers a new label option if the label combination already exists. Doing this leads to the additional callback being called twice (I already opened an issue for this Callback always called twice when providing prompt_fn and callback to InteractiveOption #5023)
  • Is there a way to create a local code in the test system with a fixture? With the aiida_local_code_factory I can only create remote codes on localhost

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that should be relatively easy to fix also for beginning contributors topic/verdi
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants