aiida-core icon indicating copy to clipboard operation
aiida-core copied to clipboard

CLI: Add the subcommand `verdi computer export`

Open agoscinski opened this issue 1 year ago • 1 comments
trafficstars

Enables the export of the configuration and setup of a computer to yaml file similar to verdi code export. Since the setup and configuration are two seperate steps, one has to specify with config or setup which of the two should be exported.

There was the option to implement the commands verdi computer configure export and verdi computer setup export but since verdi computer setup does not take any additional required arguments it would be less intuitive for the user to have an optional additional argument that changes the command type. There was also the option to export both files in one command, but that seems to be less consistent with the rest of the CLI. The word config is used in verdi computer export config since in this case we are specifying the configuration, but it might be a bit confusing with verdi config (which I think should be called verdi configure for consistency). If anyone has strong opinions I can also rename it to verdi computer export configuration or verdi computer export configure as the latter seems to be used in other projects like aiida-core-registry

~~I am adding tests now, but already open a draft for comments~~

Relevant issues:

  • https://github.com/aiidateam/aiida-core/issues/3521
  • https://github.com/aiidateam/aiida-project/issues/3

agoscinski avatar May 13 '24 15:05 agoscinski

Very nice work, thanks @agoscinski! For me, the naming is fine. I just dogfooded it a bit by running verdi computer export --help, and exporting and re-importing my daint-gpu and localhost (from verdi presto) computers. Everything worked well, I just noted a few minor things (see code comments). In addition, when exporting localhost, the config YAML only contains safe_interval: 0, so using the interactive verdi computer config still asks for Use login shell when executing command. Is it possible to also include that in the exported YAML, so that all possible options are contained there? This was the case for daint-gpu, where without running -n/--non-interactive during import I didn't get prompted for anything as everything was contained in the YAML file. If not, it's also fine as it's just one option and using -n sets the default.

This sound like verdi presto should set the use_login_shell configuration since it is a required argument in the regular verdi computer configure. Is it okay if I change it on that side in the code? What do you think @GeigerJ2 ?

agoscinski avatar May 24 '24 12:05 agoscinski

This sound like verdi presto should set the use_login_shell configuration since it is a required argument in the regular verdi computer configure. Is it okay if I change it on that side in the code? What do you think @GeigerJ2 ?

Forwarding that to @sphuber as verdi presto is his baby :D I'm not sure if I remember correctly, but I think Gio once mentioned to me that use_login_shell could make things slower (I might be wrong, though, and it could have been another option). If so, we should then set a reasonable default there.

GeigerJ2 avatar May 24 '24 15:05 GeigerJ2

I squashed all changes from last review into the first commit, so all new commits are from the second round. The incomplete config @GeigerJ2 mentioned when using verdi presto is still an issue, but I don't know where I should solve it. The cmdline submodule is definitely the wrong place to impose defaults.

agoscinski avatar May 27 '24 16:05 agoscinski

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.69%. Comparing base (ef60b66) to head (47cf99c). Report is 112 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6389      +/-   ##
==========================================
+ Coverage   77.51%   77.69%   +0.18%     
==========================================
  Files         560      562       +2     
  Lines       41444    41699     +255     
==========================================
+ Hits        32120    32392     +272     
+ Misses       9324     9307      -17     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 27 '24 17:05 codecov[bot]

Thanks a lot @agoscinski

sphuber avatar May 27 '24 22:05 sphuber