aiidateam / aiida-core

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

Inconsistent usage of `setup`/`create`/`configure` in verdi commands `profile`/`user`/`code`/`computer` to make new instances, set and update parameters #6431

Open agoscinski opened 1 month ago

agoscinski commented 1 month ago

This arises from the discussion in issue https://github.com/aiidateam/aiida-core/issues/4981 since this covers more than just theverdi computer setup command I made a separate issue to it.

Motivation

To make new instances of objects, set parameters, update parameters the verdi commands verdi [profile|user|code|computer|group] use different command names which is very confusing for new users (at least for me). I can see some reasons for the different naming, but there so strong inconsistencies that result in a bad user experience.

verdi profile setup # make new instance from positional database plugin and set paramaters

verdi user configure # make new instance, set parameters, and update parameters

verdi code create # make new instance and sets parameters taking code plugin as positional argument
verdi code setup # make new instance and set parameters, or update parameters of existing instance

verdi computer setup # make new instance and sets some parameters
verdi computer configure # set remaining parameters taking transport plugin as positional argument

verdi group create # make new instance

Current work in progress

With the changes in PR https://github.com/aiidateam/aiida-core/pull/6198 we can merge verdi computer setup and configure to one command as

If we were to migrate the computer setup code to use the DynamicEntryPointCommandGroup we could turn it into a single command. It would work exactly like the verdi code create where you would have verdi computer create and the transport plugin would be the subcommand. So you would have verdi computer create core.local and verdi computer create core.ssh.

https://github.com/aiidateam/aiida-core/issues/4981#issuecomment-2136995240

So with these changes the state would be (correct me if I am wrong)

verdi computer create # make new instance and all parameters taking transport plugin as positional argument

Further suggestions

Since users still want to change computer parameters (see discussion https://github.com/aiidateam/team-compass/issues/12), there would be still some command that allows updating the parameters of the computer

verdi computer create # make new instance and all parameters taking transport plugin as positional argument
verdi computer configure # update parameters

This would clash with the verdi user configure that is used for the creation and an update of the parameters, so maybe we also split up verdi user configure into verdi user create and verdi user configure Furthermore, there is verdi code setup that seems redundant to me to verdi code create In addition, if we go with the naming of create then verdi profile setup would be renamed to verdi profile create.

So the final suggestion would be

verdi profile create # make new instance from positional database plugin and set paramaters

verdi user create # make new instance, set parameters
verdi user configure # and update parameters

verdi code create # make new instance and sets parameters taking code plugin as positional argument

verdi computer create # make new instance and all parameters taking transport plugin as positional argument
verdi computer configure # update parameters

verdi group create # make new instance
sphuber commented 1 month ago

The inconsistency is essentially due to us having to keep backwards-compatibility. Historically the commands were as follows:

verdi setup  # Setup a profile (Storage was not pluginnable, so was hard-coded to `core.psql_dos`)
verdi quicksetup  # Called through to `verdi setup` after auto-creating PSQL user+database
verdi computer setup
verdi code setup

Profiles

When we recently made storage backends pluginnable, we needed a new command to set up a profile with any of the storage plugins. This became verdi profile setup. So now verdi setup has been replaced by veri profile setup core.psql_dos and verdi setup should be deprecated.

The idea is to add the functionality of verdi quicksetup of automatically creating PSQL user+database to verdi presto that can be enabled with a flag. Once implemented, verdi quicksetup should be deprecated as well.

The user then has

verdi profile setup  # Setup profile with any storage plugin
verdi presto  # Full automatic setup of `core.sqlite_dos` or `core.psql_dos` profile with/without RMQ

Codes

Originally, there was 1 code class, Code, but this was essentially working as two different types, called a "remote" and "local" code. Here "remote" meant a code that was simply an executable available on the target Computer and "local" meant you have a folder with the code on the local file system that contained the binary (and optionally other supporting files). Besides the names remote/local being notoriously confusing, having a single class/command deal with two different types was difficult to maintain and extend.

The Code class was deprecated and refactored into InstalledCode and PortableCode to replace the "remote" and "local" versions, respectively. Later the ContainerizedCode was added. These needed a command to setup these codes. Since verdi code setup was taken and could not be changed in order to not break backwards compatibility, the new command was named verdi code create. The verdi code setup command is deprecated.

Users

There is not too much history here. I agree that verdi user configure to both create new and update existing users is very unintuitive. I would personally rename this to

verdi user create
verdi user update

These are the typical verbs used in CRUD (create,read,update,delete) applications. The word "configure" is a bit uncommon.

Ideal case

I mostly like your suggestion for the command names but would make a few changes. Overall I think using create as the main verb over setup makes sense thinking of CRUD apps:

verdi group create # create new group

verdi profile create # This is fine but would have to deprecate `verdi profile setup` that was just added

verdi user create # create new user
verdi user update # update existing users

verdi code create # create new code (dynamic based on code plugin)

verdi computer create # create new computer
verdi computer configure # create new auth info (dynamic based on transport plugin

So essentially I would just change the user commands.

If we were to accept this proposal, we would just have to deprecate verdi computer setup, verdi profile setup and verdi user configure. The latter is not a problem, but the former two are more impactful. Especially verdi computer setup is used a lot. Andverdi profile setupwas added only just the last release, so it feels a bit silly to deprecate already. But then again, when we argue that this will make the entire CLI more consistent and easy to use, it could be justified. Moreover, none of this would actually be breaking as we can deprecate the old commands. The change ofverdi user configure` would not be bad at all as it is used very little by users I suspect.