GroupPath enhancements

[chrisjsewell]

As discussed, CLI improvements to open in a separate "enhancements" issue, when this PR is merged:

• Add --depth CLI option to work with --recursive
• Add node count option; parent paths will return a count of all descendant nodes, e.g.

$ verdi group path ls --recursive --count
Path       Nodes
-------  ------------
a         10
a/b       4
a/c       6

• Option to report paths relative to base path, e.g.

$ verdi group path ls a
a/b
a/c
$ verdi group path ls a --relative
b
c

• make delimiter publicly configurable
• dealing with multiple type_string

Originally posted by @chrisjsewell in #3613 (comment)

[ltalirz]

Sorry, one more question: I looked into #3613 and did not find any documentation being added.
Have I overlooked it or is there no documentation of this feature yet?

Just some basic info on the separator and how to use the group path cli would be helpful.

[chrisjsewell]

Have I overlooked it or is there no documentation of this feature yet?

No nothing yet 😬 this is a TODO in #3913

[ramirezfranciscof]

So, just to keep track of it in case we can come up with some solution, I would like to add here the issue of how groups that are incompatible with GroupPath are still partially shown by some methods and the way the code deals with them makes it, IMHO, a bit difficult for users to identify what the problem is (specially if we want to use this as the first and most intuitive way of handling groups, as @giovannipizzi suggested when discussing the tutorial).

[chrisjsewell]

Some additional notes:

• verdi group path ls: If no groups exist raises NoGroupsInPathError; this should be handled more elegantly with a click message
• make GroupPath available as from aiida.tools import GroupPath
• Allow to add description in GroupPath.get_or_create_group()

[chrisjsewell]

issue of how groups that are incompatible with GroupPath are still partially shown by some methods

It would be good if you could give some concrete examples here that we can discuss around

[giovannipizzi]

Indeed, @ramirezfranciscof could you report here the issues?
I think one is to maybe adapt the API for normal groups to already "complain" with a warning, at creation time, if a group name is not ok for a group path?
I don't remember the other things that @ramirezfranciscof pointed out right now

[ramirezfranciscof]

Ok, the behavior I believe could be confusing is this:

(aiida-core) $ verdi group create bad//group
Success: Group created with PK = 3 and name 'bad//group'
(aiida-core) $ verdi group path ls
bad
(aiida-core) $ verdi group path ls bad
/path/to/repo/aiida-core/aiida/tools/groups/paths.py:241: UserWarning: invalid path encountered: bad/
  warnings.warn('invalid path encountered: {}'.format(path_string))  # pylint: disable=no-member

[chrisjsewell]

Cheers, and what would you like to see the behaviour be here?

[chrisjsewell]

Note you can disable warnings with verdi group path ls --no-warn.
I definitely think that it would be better if, in the CLI, the warnings were captured and formatted nicer.
In terms of not actually displaying bad; the main issue is that you have to "lookahead" to check that it has no valid children, e.g. you still want it to show if you had groups bad//group and bad/ok. Not to say this can't be done, it just will require some changes to the current core logic.

[ramirezfranciscof]

In the context of using this as "the intuitive/introductory aproach to groups", I don't think it would be a good idea to ask the users to pass a --no-warn option to avoid possible problems. Moreover, ignoring the problem could lead to even more confusing scenarios ("I have this bad group, I list it and has no sub-groups, I count nodes and has 0, but I try to delete it and [ gives me an error / is still there / other ]").

I think warnings could be useful but for (1) notifying of possible inconvenients when creating a problematic group ("warning: this group will not be able to be accessed properly with the group paths tools, only with group list") and (2) giving clear information of the problem while warning of possible ommisions (when path lsing: "Warning: some groups could not be displayed with this tool, run with --show-unavail to see these or use group list to see them" / "Warning: the following groups cannot be listed in the group paths format").

Besides the warnings, I also believe the proper behavior for verdi group path ls (or, really, for the walk() and children methods) should be to not show names that don't correspond to any valid group-path or group (i.e. "bad" is nothing, it is not a group and it is not a valid path to a group). I am not exactly sure what you mean by "lookahead", but from what I could see of how children works, wouldn't adding a check that there is no double "delimiter" (or no empty elements in the path list) before adding something to the yielded list achieve this? (I am looking at line 229 and 235 of aiida/tools/groups/paths.py) The method walk() is a bit more difficult for me to understand (i.e. I'm not sure how for child in self: works...) but there should be a similar solution I would guess.

Anyways, this is just my opinion, perhaps nobody else thinks this behavior is confusing and it makes no sense worrying about this right now. I just thought this should be considered given the intended use for this feature.

[chrisjsewell]

I just thought this should be considered given the intended use for this feature.

Oh yes it should definitely be considered, and I'm sure there are some improvements we can make here 😄

[giovannipizzi]

One more thing - I think we should add a command to show the nodes in a group path (essentially an alias or a modified version of verdi group show) also under verdi group path, to avoid that one has to jump between verdi group and verdi group path - of course the verdi group path show could also enrich the output with group-path-specific output.

Anyway, I feel the ultimate benefit from GroupPaths will come when fixing giovannipizzi/aiida-node-shell#5 and then providing the functionality to users