aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
435 stars 188 forks source link

identifier resolution - usability improvements? #3346

Open ltalirz opened 5 years ago

ltalirz commented 5 years ago

We just had a case at the SINTEF tutorial, where someone called his first computer c1 in verdi setup. He proceeded to verdi computer configure local c1 and it failed with a message like computer with uuid 'c1' not found.

My feeling is that for commands like verdi computer and verdi code, where the default mode of operation is to pass labels, the chance of creating a label that can also be interpreted as a hex value is quite significant, and we should provide more assistance to users in this case.

There two options I can think of:

  1. Adding a line to the error message saying In order to force interpretation as a label, use '!{argument}'
  2. Switch to a "greedy" interpretation that will continue trying to find an identifier, even if it is a valid UUID that cannot be found

Solution 1. could be implemented globally, adding perhaps a similar error message when a PK is not found (with instructions on how to force interpretation as a uuid & as a label).

For solution 2. , the modification could be implemented by inheriting from the IDENTIFIER class that is used only in commands where the default pattern of people is to use labels (verdi computer, verdi code, ...).

Solution 1 is a bit more general and is something we might want to do anyhow (?). Solution 2 potentially adds a little bit of user friendliness on top (since removes hassle for dealing with these types of labels) but potentially can lead to misinterpretation of identifiers (not sure how likely this is).

Mentioning @sphuber and @giovannipizzi for comment

giovannipizzi commented 5 years ago

While I see the advantage of (2), it's a bit dangerous because if one new node starting with c1 is added to the DB then the behavior changes, and then it's even more surprising for users. So I would favour consistent behavior.

For (1) I fully agree - making the message clearer is of course good, and indeed even for me who I know how this is supposed to work, it's not obvious where I should look for an explanation. I added something to the Slovenia tutorial and I know there are some docs somewhere, but not obvious where to look. So ok. Only question is where to write the message. I think we should keep the same message in the python API, and change the message only in verdi. However

ltalirz commented 5 years ago

I think doing 1. is already a good enough step forward.

I think we should keep the same message in the python API, and change the message only in verdi.

Agreed.

we should catch both the errors for "missing node with that starting UUID" and "more than one node"

Yes - I guess you mean providing different messages depending on this, correct? I.e. explaining how to disambiguate when multiple objects are found.

we should do the same when looking for a starting part of an UUID only composed by digits (add a dash in that case)

Yes - if no PK is found, we'll add 2 messages, explaining how to let AiiDA resolve it as a uuid and how to let AiiDA resolve it as a label.

the issue remains if that string actually matches a valid node (e.g. you specify 12334, this is a valid PK, but you meant a node with UUID starting with 12334, or a computer whose label is 12334) - not sure on what to do in this case, though... maybe extend the -h for all times we use the click parameter, to have a short explanation or a link to the docs on how to specify a label, pk or UUID?

This case will remain but I don't see how to avoid this. One could think of always printing how an identifier was resolved, if the verbosity level is increased (not sure whether this is possible using the current machinery).

Providing a link to the docs (or some more info) in the help string sounds like a good idea.

ltalirz commented 4 years ago

One related issue I just noticed: verdi node graph generate currently uses short uuids, and it is not unlikely to hit a short uuid that is only integers, which then won't be resolved. I guess one can simply append the missing dash, but people may not be aware of this...

sphuber commented 4 years ago

verdi node graph generate currently uses short uuids

With short uuids do you mean for the node argument, or for the labels of the nodes in the printed graph?

ltalirz commented 4 years ago

Sorry, I meant the identifiers printed in the PDF output