CLI: Add the subcommand `verdi computer export` #6389

agoscinski · 2024-05-13T15:22:09Z

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:

GeigerJ2 · 2024-05-15T05:05:10Z

docs/source/reference/command_line.rst

@@ -105,6 +105,7 @@ Below is a list with all available subcommands.
      disable    Disable the computer for the given user.
      duplicate  Duplicate a computer allowing to change some parameters.
      enable     Enable the computer for the given user.
+      export     Exports the Authinfo details for a computer (and user).


Why add the user here? Only computer should be enough, no? I suppose this is because Computer.get_configuration allows specifying a user? Though, as verdi computer export in its current state doesn't provide that option, we could remove its mentioning here, or, alternatively, allow specifying an optional user via @options.USER.

I wonder if we should phrase even slighly more differently still. What actually happens is that verdi computer setup creates a Computer instance and verdi computer configure then creates a AuthInfo, which joins a Computer and User together. This is because a computer can be configured for multiple users.

So really what we are exporting is a file describing the Computer instance and a file describing the AuthInfo. Now I think that most users are not really familiar with the AuthInfo, so I wonder if we should include it here in the docstring. How about

export Export a computer and its transport configuration

Note that exactly what data goes into the AuthInfo depends on the chosen transport type for the computer. That is also why verdi computer configure has the transport plugin as a subcommand, because that determines which options should be exposed.

I think users are more familiar with transport than AuthInfo.

Ah right, I did not fully understand this. Thanks for the catch. I now added the user option for export.

I am not sure if "transport" is more understandable for the user. I have the feeling "authentication info" (writing it out) is quite understandable while "transport" seems more abstract to me. @GeigerJ2 what do you think?

In any case I copied this from the verdi computer configure description, so would be in this case consistent and change it in both places

See commit for changes 521228b

For me, "authentication info" seems nicer than AuthInfo especially for users who are not yet familiar with the AiiDA classes. If that captures the essence of what is being configured, then that's fine for me.

src/aiida/cmdline/commands/cmd_computer.py

GeigerJ2 · 2024-05-15T05:10:03Z

src/aiida/cmdline/commands/cmd_computer.py

+@arguments.COMPUTER()
+@arguments.OUTPUT_FILE(type=click.Path(exists=False))
+@with_dbenv()
+def computer_export_setup(computer, output_file):


As we recently added the option to make the sorting optional for the code export (see PR #6345), I propose we also add it here as an additional option.

Don't you think, this should be just be on always? No user option here? I feel like it is not a meaningful option for a user to have a random order in the yaml file the developer decided or sorted. Sorting is also not a computational expensive postprocessing task so I would always sort the yaml.

Personally, I find append_text appearing in the beginning of the YAML file confusing, as that is one of (if not the) last option one configures when running the command interactively. @sphuber and I discussed this also in the linked PR #6345. While I don't think it's super important, the aiida-code-registry and the aiida-resource-registry currently contain YAML files which are not alphabetically sorted, but instead in the order in which the options are defined in the src which should also be the order in which the user is prompted when running interactively. So if we consider these three use cases: 1) interactive CLI command, 2) import from the YAML files in the registry, and 3) export from a profile, it should be consistent throughout, rather than sorting alphabetically for 3) if that is not done for 1) and 2).

I see the point here. The default would sort it more logically, similar to order the prompt asks the user. I then agree with you

GeigerJ2 · 2024-05-15T05:14:02Z

src/aiida/cmdline/commands/cmd_computer.py

+        with open(output_file, 'w', encoding='utf-8') as yfhandle:
+            yaml.dump(computer_setup_data, yfhandle)
+        echo.echo_success(f"Computer<{computer.pk}> {computer.label} setup exported to file '{output_file}'.")
+    except Exception:


Can we make this more explicit? Do we know which exceptions to expect here? FileExistsError should be captured already in the @arguments.OUTPUT_FILE. Anything else? Possibly also communicate the failure via echo.echo_error/echo.echo_critical rather than just raising the Exception.

+1 for this. Ideally we narrow the scope of the exceptions being caught unless we really don't know what could be thrown. But definitely we should not reraise but simply do echo.echo_critical(f'Export failed: {exception}')

I put the error message into an echo.

I put a logging of the whole traceback into the debug logger, please let me know if this makes sense, ~~I moved the logger to the cmdline level, since it seems to make more sense to me than in utils.~~ I just import the logger from echo module instead of creating a new one. I am not sure if should not use a custom logger for the file CMDLINE_LOGGER = logging.getLogger('verdi computer'). I can also put this change to another PR.

FileExistsError should be captured already in the @arguments.OUTPUT_FILE

Seems not really to take take. One can overwrite files currently but verdi code export does overwrite it too, so it is consistent. We could change behavior in both commands to not overwrite and add an --overwrite flag? But I would do it in a different PR. What do you think @GeigerJ2 ? The only thing it does if a path with the same name exists it does not overwrite

Ah, seems like I misunderstood the behavior of exists=False in

@arguments.OUTPUT_FILE(type=click.Path(exists=False))

Thought that would raise if the file exists already, but it just means it doesn't necessarily need to exist ^^

I think exporting configuration files, it's fine to overwrite by default, as accidentally overwriting is not too critical. In my dumping PR, I added it as a click option with default=False, as there I'm really doing a recursive deletion of files and directories, so one might lose more relevant data.

You think it is worth to make an issue with priority nice-to-have for a --no-overwrite feature for the export commands?

GeigerJ2 · 2024-05-15T05:45:47Z

src/aiida/cmdline/commands/cmd_computer.py

+
+@computer_export.command('setup')
+@arguments.COMPUTER()
+@arguments.OUTPUT_FILE(type=click.Path(exists=False))


And, more general, some idea for the future: We could also make the OUTPUT_FILE optional, and use f{label}-setup/config.yaml as default (also for the code export), as this is probably how people would usually name the output file?

Makes sense, but are you suggesting that it is created in a folder? Or should it just create a file?

I am also in favor of this, I'll make an issue. I did not do it because I wanted it to be consistent with verdi code export which requires an output file name

GeigerJ2

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.

Maybe @khsrali and @sphuber can try it with their computers, or have other comments, then I think this should be ready to go soon 🚀

sphuber

Thanks a lot @agoscinski , looking good. I added a few more comments.

src/aiida/cmdline/commands/cmd_computer.py

tests/cmdline/commands/test_computer.py

agoscinski · 2024-05-24T12:36:48Z

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 ?

GeigerJ2 · 2024-05-24T15:43:23Z

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.

sphuber

Thanks @agoscinski . Just a few more remaining minor comments. I have no strong opinion on the sorting. As can be read from the discussion that @GeigerJ2 linked, I don't really see why this should matter. But now that it has been added, I am also fine with adding the option to optionally sort as was done for verdi code export.

sphuber · 2024-05-26T13:44:26Z

src/aiida/cmdline/commands/cmd_computer.py

+    }
+    try:
+        output_file.write_text(yaml.dump(computer_setup, sort_keys=True), 'utf-8')
+        echo.echo_success(f"Computer<{computer.pk}> {computer.label} setup exported to file '{output_file}'.")


I think this should rather go in the else-block of the try/except. Otherwise, if there would be a bug hiding in this line that raises, it would be caught by the exception that is designed to catch errors in the actual dumping

sphuber · 2024-05-26T13:45:27Z

src/aiida/cmdline/commands/cmd_computer.py

+        if user is None:
+            user = User.collection.get_default()
+        computer_configuration = computer.get_configuration(user)


This can be simplified as Computer.get_configuration already takes care of getting the default if user is None

Suggested change

if user is None:

user = User.collection.get_default()

computer_configuration = computer.get_configuration(user)

computer_configuration = computer.get_configuration(user)

sphuber · 2024-05-26T13:45:51Z

src/aiida/cmdline/commands/cmd_computer.py

+            user = User.collection.get_default()
+        computer_configuration = computer.get_configuration(user)
+        output_file.write_text(yaml.dump(computer_configuration, sort_keys=True), 'utf-8')
+        echo.echo_success(f"Computer<{computer.pk}> {computer.label} configuration exported to file '{output_file}'.")


Same, put this is else-clause

sphuber · 2024-05-26T13:48:17Z

src/aiida/cmdline/commands/cmd_computer.py

+        echo.echo_critical(
+            f'Unexpected error while exporting configuration for Computer<{computer.pk}> {computer.label}'
+            f' and User<{user.pk}> {user.email}: {e!s}.'


I think there is actually a bug here. If the user is not specified, it will be None and user.pk and user.email will raise.

I mean it depends, if I use before

if user is None: user = User.collection.get_default()

then this should work

Okay get_default can return None I see, then I will do here some check and get the user from computer

I think technically one gets an error of the form

Critical: no default profile defined: None

and because it immediately exists one never has the issue with user is None, but this requires knowledge of the layers below, so I added an if then else changing the error message depending on if the user is None

I think technically one gets an error of the form

Critical: no default profile defined: None

I don't understand where that exception would come from.

and because it immediately exists one never has the issue with user is None, but this requires knowledge of the layers below, so I added an if then else changing the error message depending on if the user is None

The situation I imagine is computer.get_configuration raising for some reason. The code then goes to the except clause and if there you use user but it is None you will get another exception. And user will be None if the caller does not explicitly specify one, which is usually the case.

I don't understand where that exception would come from.

Sorry, cannot recreate, must being doing something weird or my environment was broken.

The situation I imagine is computer.get_configuration raising for some reason. The code then goes to the except clause and if there you use user but it is None you will get another exception. And user will be None if the caller does not explicitly specify one, which is usually the case.

Yes I think it is fair point. I thought that somehow it will always exit beforehand because of some precheck.

agoscinski · 2024-05-27T16:54:00Z

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.

codecov · 2024-05-27T17:17:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sphuber · 2024-05-27T17:47:07Z

src/aiida/cmdline/commands/cmd_code.py

@@ -240,7 +240,7 @@ def show(code):
    '--sort/--no-sort',
    is_flag=True,
    default=True,
-    help='Sort the keys of the output YAML.',
+    help='Sort the keys of the output YAML. Default --no-sort.',


Suggested change

help='Sort the keys of the output YAML. Default --no-sort.',

help='Sort the keys of the output YAML.',

We shouldn't manually encode the default. click options take show_default=True if you want that. There might be a bug in our InteractiveOption class that causes it not to be rendered, but that should be fixed separately.

Besides, is --no-sort actually the default? It seems that you define default=True which I think would mean it would sort by default, right?

Thanks for the catch, changed it everywhere to show_default

sphuber · 2024-05-27T18:11:24Z

src/aiida/cmdline/commands/cmd_computer.py

@@ -673,7 +675,7 @@ def get_command(self, ctx, name):

 @verdi_computer.group('configure', cls=LazyConfigureGroup)
 def computer_configure():
-    """Configure the Authinfo details for a computer (and user)."""
+    """Configure the authentication info for a computer and user."""


I think this new version is an improvement, but I wonder if we should actually call it "authentication". As I mentioned in this comment in another issue, even though it does create an AuthInfo object, it really configures the transport. Some of these options don't necessarily have to do with authentication at all, e.g., the safe_interval and use_login_shell for example. So what would be more correct is

Configure the transport for a computer and user.

I can see how transport might be a bit vague to users (although it does come up in the verdi computer setup that precedes this command). So maybe the following could work?

Configure a computer for a given user.

I like the first one actually more. Having only this

configure Configure a computer for a given user. setup Create a new computer.

does not transport much information. And transport is something one can look up in the documentation and find actually materials.

sphuber · 2024-05-27T19:19:39Z

src/aiida/cmdline/commands/cmd_computer.py

+        echo.echo_critical(
+            f'Unexpected error while exporting configuration for Computer<{computer.pk}> {computer.label}'
+            f' and User<{user.pk}> {user.email}: {e!s}.'


I think technically one gets an error of the form

Critical: no default profile defined: None

I don't understand where that exception would come from.

and because it immediately exists one never has the issue with user is None, but this requires knowledge of the layers below, so I added an if then else changing the error message depending on if the user is None

The situation I imagine is computer.get_configuration raising for some reason. The code then goes to the except clause and if there you use user but it is None you will get another exception. And user will be None if the caller does not explicitly specify one, which is usually the case.

sphuber · 2024-05-27T19:22:21Z

src/aiida/cmdline/commands/cmd_computer.py

+)
+@with_dbenv()
+def computer_export_config(computer, output_file, user, sort):
+    """Export the configuration of the authentication info for a computer and user to a yaml file."""


Suggested change

"""Export the configuration of the authentication info for a computer and user to a yaml file."""

"""Export computer configuration to a yaml file."""

I did

"""Export computer transport configuration for a user to a YAML file."""

tests/cmdline/commands/test_computer.py

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.

move echo_success messages into else branch of the try-except in computer_export_config gerror message includes user information only if user is specified

Co-authored-by: Sebastiaan Huber <[email protected]>

tests/cmdline/commands/test_computer.py

sphuber · 2024-05-27T22:33:58Z

Thanks a lot @agoscinski

This command has two subcommands, `setup` and `config`, that dump the definition of a `Computer` and an associated `AuthInfo` to a YAML file that can be used to recreate it using `verdi computer setup` and `verdi computer configure` using the `--config` option, respectively.

agoscinski force-pushed the verdi-computer-export branch 2 times, most recently from 92fe8e3 to 1b7c3e0 Compare May 14, 2024 06:11

agoscinski marked this pull request as ready for review May 14, 2024 06:14

agoscinski requested a review from GeigerJ2 May 14, 2024 06:14

GeigerJ2 reviewed May 15, 2024

View reviewed changes

sphuber requested changes May 15, 2024

View reviewed changes

agoscinski mentioned this pull request May 24, 2024

CLI: Make output file for verdi <subcommend(s)> export optional #6414

Closed

agoscinski requested a review from GeigerJ2 May 24, 2024 12:37

sphuber requested changes May 26, 2024

View reviewed changes

agoscinski force-pushed the verdi-computer-export branch from 6a71ae0 to a742c9c Compare May 27, 2024 16:48

sphuber reviewed May 27, 2024

View reviewed changes

sphuber requested changes May 27, 2024

View reviewed changes

agoscinski force-pushed the verdi-computer-export branch from 1db2b6f to a8f77fc Compare May 27, 2024 19:51

agoscinski and others added 10 commits May 27, 2024 21:51

add optional sort argument to export

67cec29

make test more senseical by using exit code

7626b12

add comments from sphuber

3ae5e45

move echo_success messages into else branch of the try-except in computer_export_config gerror message includes user information only if user is specified

change order of arguments to be consistent with decorators

83dd98a

test except code

7748aa0

rm default option from command help message and use show_default option

b859bba

yaml -> YAML in help page

fcaea76

adapt computer configure related help pages

5102940

Apply suggestions from code review

51999ec

Co-authored-by: Sebastiaan Huber <[email protected]>

agoscinski force-pushed the verdi-computer-export branch from a8f77fc to 51999ec Compare May 27, 2024 19:51

Merge branch 'main' into verdi-computer-export

5045484

sphuber requested changes May 27, 2024

View reviewed changes

Apply suggestions from code review

47cf99c

sphuber approved these changes May 27, 2024

View reviewed changes

sphuber merged commit 9e3ebf6 into aiidateam:main May 27, 2024
17 checks passed

agoscinski deleted the verdi-computer-export branch May 28, 2024 06:54

sphuber mentioned this pull request Jun 1, 2024

dump computer/codes in YAML format #3521

Closed

	help='Sort the keys of the output YAML. Default --no-sort.',
	help='Sort the keys of the output YAML.',

	"""Export the configuration of the authentication info for a computer and user to a yaml file."""
	"""Export computer configuration to a yaml file."""

CLI: Add the subcommand verdi computer export #6389

CLI: Add the subcommand verdi computer export #6389

Conversation

agoscinski commented May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeigerJ2 May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeigerJ2 left a comment • edited Loading

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

agoscinski commented May 24, 2024 • edited Loading

GeigerJ2 commented May 24, 2024

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski commented May 27, 2024

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski May 27, 2024 • edited Loading

Choose a reason for hiding this comment

sphuber commented May 27, 2024

CLI: Add the subcommand `verdi computer export` #6389

CLI: Add the subcommand `verdi computer export` #6389

agoscinski commented May 13, 2024 •

edited

Loading

agoscinski May 24, 2024 •

edited

Loading

GeigerJ2 May 24, 2024 •

edited

Loading

agoscinski May 24, 2024 •

edited

Loading

GeigerJ2 left a comment •

edited

Loading

agoscinski commented May 24, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

agoscinski May 27, 2024 •

edited

Loading