giovannipizzi / aiida-node-shell

Proof of concept of a way to easily browse AiiDA nodes through an interactive, customisable shell with tab completion
MIT License
6 stars 1 forks source link

various enhancements #4

Closed zhubonan closed 4 years ago

zhubonan commented 4 years ago

I think it would be a good time to raise and PR, as discussed in the meeting I have added a few commands and I will add more/change the existing accordingly.

This PR is a work-in-progress and mainly act as a potential place of discussion and progress tracking.

Changes:

Konwn Issues:

This PR address:

zhubonan commented 4 years ago

@giovannipizzi @bosonie I have made some major improvement based on the meeting notes. Could you please have the new version in this PR a try and give me some feedback?

Obviously there is no automated testing at the moment... So feedback would be very welcome.

A major improvement that I think people will like is that the entire verdi command line suite can be used with much better responsiveness than that from the shell. In addition, we have command substitution using the currently loaded node or reference to history. So you can use the comand like this:

verdi calcjob outputcat {-1} | less

without the need to remember the pks.

There is no tab completion though, and it is unlikely to have an easy solution, but people are free to add aliases. For example, I use vpl for verdi process list.

We can close #2 and #3 once this is merged.

bosonie commented 4 years ago

Hi @zhubonan. This looks great! Seriously! Only one minor comments. The graphics of nodehist_show doesn't satisfy me fully. At the moment I see:

    (Dict<21668>@first) nodehist_show
    WorkChainNode<21662> Li 
    🢃  ---  [iteration_01] CALL_CALC
    CalcJobNode<21665>  
    🢃  ---  [output_parameters] CREATE
    Dict<21668>  <-- We are here
    (Dict<21668>@first)

it would be nice to have an indentation or a bar that helps to distinguish between the output of the nodehist_show command and the shell prompt. Something like:

    (Dict<21668>@first) nodehist_show
      |  WorkChainNode<21662> Li 
      |  🢃  ---  [iteration_01] CALL_CALC
      |  CalcJobNode<21665>  
      |  🢃  ---  [output_parameters] CREATE
      |  Dict<21668>  <-- We are here
    (Dict<21668>@first)

I'm not very familiar with groups therefore I didn't test this part extensively... the rest seems perfect to me. Thanks!

zhubonan commented 4 years ago

@bosonie Thanks, I agree it looks much better with the indentation! I will add it soon.

giovannipizzi commented 4 years ago

Indeed, this really looks great! I will now give some feedback on the PR.

There is no tab completion though, and it is unlikely to have an easy solution, but people are free to add aliases. For example, I use vpl for verdi process list. I just created a PR on your PR branch (I'm trying to create circular dependencies in GitHub ;-) ) that introduces fast bash completion for verdi! With this, the tool is perfect, I would say :-D

Actually, I would maybe thing to rename it to verdi powershell? In principle even if you don't care about the rest of the commands, now you can just call verdi powershell and then continue working with verdi commands, that now become super fast?

giovannipizzi commented 4 years ago

In general it looks great! I didn't try out the comments interface but we can improve later in case it's needed.

Instead, I have a final comment on the groups.

What I had in mind was an interface mirroring the new verdi group path (pinging @chrisjsewell for feedback). The key idea is that if you have the following groups a, a/b, a/c, a/c/d, e, they behave as folders, so if you do verdi group path ls you see only a and e, if you do verdi group path ls a you see b and c, and so on.

Maybe we could do the following (in the following I speak of groups but I actually refer to group paths):

One important thing to decide is how to deal with group types. Should we limit to only "user types"? Can we define a syntax (do we already have one?) to load a group of a different type, e.g. group_name:group_type?

Since this discussion might become long: maybe let's just have a simple group_belong in this PR, and we move possible discussions on the group path functionality in an issue (@zhubonan if you see that there starts to be discussion, feel free to move to a separate issue).

zhubonan commented 4 years ago

@giovannipizzi Thanks for the comments. I will change the codes accordingly.

I think perhaps it is good to leave the group related stuff to the next PR. I am not familiar with the group path concept yet, since I have not upgraded aiida-core to v1.2.0 (need to test the plugins with the new changes in v1.2.0 for safety).

zhubonan commented 4 years ago

@giovannipizzi All done. It is ready to be merged now.

chrisjsewell commented 4 years ago

pinging @chrisjsewell for feedback

I think what you said makes sense btw. But yes I'd have to have a play around with this feature to understand better, and happy to have a stab at it in a further PR 👍

zhubonan commented 4 years ago

@giovannipizzi Thanks, all done, please do check if the colors are OK on OSX!

giovannipizzi commented 4 years ago

Thanks a lot!!!!