👌 Improve usage of `configure-rabbitmq` #6474

mbercx · 2024-06-12T09:03:17Z

Implements the suggestions in #6454 (comment).

Example UX:

❯ brew services stop rabbitmq
Stopping `rabbitmq`... (might take a while)
==> Successfully stopped `rabbitmq` (label: homebrew.mxcl.rabbitmq)
❯ verdi presto
Report: Option `--use-postgres` not enabled: configuring the profile to use SQLite.
Report: RabbitMQ server not found: configuring the profile without a broker.
Report: Initialising the storage backend.
Report: Storage initialisation completed.
Success: Created new profile `presto-7`.
Success: Configured the localhost as a computer.
❯ verdi status
 ✔ version:     AiiDA v2.5.1.post0
 ✔ config:      /Users/mbercx/project/core/.aiida
 ✔ profile:     presto-7
 ✔ storage:     SqliteDosStorage[/Users/mbercx/project/core/.aiida/repository/sqlite_dos_9334f1ac2f1f42d2b51724491fdb9030]: open,
 ⏺ broker:      No broker defined for this profile: certain functionality not available.
 ⏺ daemon:      No broker defined for this profile: daemon is not available.
❯ brew services start rabbitmq
==> Successfully started `rabbitmq` (label: homebrew.mxcl.rabbitmq)
❯ verdi profile configure-rabbitmq
Success: Successfully connected to RabbitMQ server.
❯ verdi status
 ✔ version:     AiiDA v2.5.1.post0
 ✔ config:      /Users/mbercx/project/core/.aiida
 ✔ profile:     presto-7
 ✔ storage:     SqliteDosStorage[/Users/mbercx/project/core/.aiida/repository/sqlite_dos_9334f1ac2f1f42d2b51724491fdb9030]: open,
Warning: RabbitMQ v3.13.1 is not supported and will cause unexpected problems!
Warning: It can cause long-running workflows to crash and jobs to be submitted multiple times.
Warning: See https://github.com/aiidateam/aiida-core/wiki/RabbitMQ-version-to-use for details.
 ✔ broker:      RabbitMQ v3.13.1 @ amqp://guest:guest@127.0.0.1:5672?heartbeat=600
 ⏺ daemon:      The daemon is not running.

codecov · 2024-06-12T09:33:22Z

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.76%. Comparing base (ef60b66) to head (a49e79a).
Report is 112 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/cmdline/commands/cmd_profile.py	91.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6474      +/-   ##
==========================================
+ Coverage   77.51%   77.76%   +0.26%     
==========================================
  Files         560      561       +1     
  Lines       41444    41781     +337     
==========================================
+ Hits        32120    32487     +367     
+ Misses       9324     9294      -30

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

sphuber

Thanks @mbercx . I'm onboard with the suggestion to change the default to being non-interactive behavior. Just some suggestions on the implementation

src/aiida/brokers/rabbitmq/defaults.py

src/aiida/cmdline/commands/cmd_profile.py

src/aiida/cmdline/params/options/main.py

src/aiida/cmdline/commands/cmd_profile.py

mbercx · 2024-06-18T07:34:35Z

@sphuber I made the requested changes:

The NON_INTERACTIVE option has been replaced with INTERACTIVE. This is the switch we discussed, but to me it was more natural to have "interactive" to avoid double negatives in the logic.
The configure-rabbitmq command now checks if it can successfully connect to the broker, if not it will ask for confirmation to configure, unless -f/--force is specified. It's a bit conflicting with the --non-interactive option, I suppose, but ok.

Here is the usage:

❯ verdi presto 
Report: Option `--use-postgres` not enabled: configuring the profile to use SQLite.
Report: RabbitMQ server not found: configuring the profile without a broker.
Report: Initialising the storage backend.
Report: Storage initialisation completed.
Success: Created new profile `presto-9`.
Success: Configured the localhost as a computer.
❯ verdi profile configure-rabbitmq 
Warning: Unable to connect to RabbitMQ server with configuration: {'protocol': 'amqp', 'username': 'guest', 'password': 'guest', 'host': '127.0.0.1', 'port': 5672, 'virtual_host': ''}
Do you want to continue with the provided configuration? [y/N]: N
Aborted!
❯ brew services start rabbitmq
==> Successfully started `rabbitmq` (label: homebrew.mxcl.rabbitmq)
❯ verdi profile configure-rabbitmq
Success: Connected to RabbitMQ with the provided connection parameters
Success: RabbitMQ configuration for `presto-9` updated to: {'protocol': 'amqp', 'username': 'guest', 'password': 'guest', 'host': '127.0.0.1', 'port': 5672, 'virtual_host': ''}
❯ brew services stop rabbitmq
Stopping `rabbitmq`... (might take a while)
==> Successfully stopped `rabbitmq` (label: homebrew.mxcl.rabbitmq)
❯ verdi profile configure-rabbitmq
Warning: Unable to connect to RabbitMQ server with configuration: {'protocol': 'amqp', 'username': 'guest', 'password': 'guest', 'host': '127.0.0.1', 'port': 5672, 'virtual_host': ''}
Do you want to continue with the provided configuration? [y/N]: N
Aborted!
❯ verdi profile configure-rabbitmq -f
Warning: Unable to connect to RabbitMQ server with configuration: {'protocol': 'amqp', 'username': 'guest', 'password': 'guest', 'host': '127.0.0.1', 'port': 5672, 'virtual_host': ''}
Success: RabbitMQ configuration for `presto-9` updated to: {'protocol': 'amqp', 'username': 'guest', 'password': 'guest', 'host': '127.0.0.1', 'port': 5672, 'virtual_host': ''}

I was still considering making the final Success: a Report: instead.

mbercx · 2024-06-18T08:08:46Z

@sphuber will update the commits properly once we've agreed on the changes.

src/aiida/brokers/rabbitmq/defaults.py

sphuber · 2024-06-18T11:43:13Z

src/aiida/cmdline/params/options/main.py

-NON_INTERACTIVE = OverridableOption(
-    '-n',
-    '--non-interactive',
-    is_flag=True,
-    is_eager=True,
-    help='In non-interactive mode, the CLI never prompts but simply uses default values for options that define one.',
-)
-


Hmm, that wasn't quite the change I was aiming for 😅 I see the point of wanting to avoid the double negative though. Ok, fine to keep the change, but I think we cannot just remove the NON_INTERACTIVE option as it is probably used elsewhere. So please reinstate it and just define if as a INTERACTIVE.clone(). Ideally we would deprecate it, but that will take some doing. We probably would have to add a deprecated argument to OverridableOption that is then checked upon option call, or the value parsing. We cannot just do it on import because then it will always show, even if the user is not actually using it

Oh right, it can also be used outside of aiida-core. 🤦 I'll reinstate it for now, we can think of the deprecation path later.

Van uitstel komt afstel.....

Can we just redefine it using INTERACTIVE.clone() though? As I understand it, we can't override the option name, and this would be used in the function definition of the click command. So I think we need to fully preserve the NON_INTERACTIVE option if we want to maintain backwards compatibility.

I've added the deprecation warning as requested to avoid afstel. ^^

So now we are exactly in the situation that I was trying to avoid. We now have two options to do the same thing that need to be maintained and the old one is deprecated. Is all of that faf really necessary just to avoid a double negative in one place? It was working fine before wasn't it?

Yeah, I hadn't considered that the options were part of the public API. I still think having an INTERACTIVE switch option makes more sense, but it isn't worth the deprecation. I'll see about turning the NON_INTERACTIVE option into a proper switch instead.

For many setup/configuration CLI commands, the `NON_INTERACTIVE` option is added to allow the user to run the command without being prompted for input and use the defaults instead. However, new users are often not aware of this option, and will not understand some options as they are prompted. Even when a sensible default is offered by the prompt, users will still want to understand the option and be unsure if the default works for them. Hence, it might be preferable to run it non-interactively by default for some commands. Here we adapt the `NON_INTERACTIVE` option into a switch (`-n/-I` or `--non-interactive`/`--interactive`) that is `--interactive` by default.

The `configure-rabbitmq` command was introduced mainly to allow new users that set up their profile with `verdi presto` before they set up RabbitMQ to upgrade their profile to use the message broker. However, since the command is interactive, they would be prompted for each of the options when running the command without `-n`/`--non-interactive`. Users that don't understand these inputs will typically want to set the defaults, so switching the modus operandi of the commmand to be non-interactive will make life easier for these users. Users that _do_ want to set different values than the defaults will understand the options of the command and should be able to be able to provide them directly or via the interactive mode.

Currently the `verdi profile configure-rabbitmq` command doesn't give any feedback to the user whether the provided options can successfully connect to the RabbitMQ server. Here we adapt the `detect_rabbitmq_config` function to accept the broker configuration as `**kwargs`, and use it to check if the provided options in the `configure-rabbitmq` can successfully connect to the RabbitMQ server. A "success" message is printed if we can connect to the server, else a warning is printed and the user is asked for confirmation before proceeding to configure the broker. A `--force` flag is also added to avoid asking for confirmation in case the command is unable to connect to the broker.

mbercx · 2024-06-19T11:29:40Z

@sphuber the NON_INTERACTIVE option has been reinstated, commit messages updated.

sphuber · 2024-06-19T15:57:43Z

@sphuber the NON_INTERACTIVE option has been reinstated, commit messages updated.

Fantastic, thanks a lot for going through that effort, much appreciated

mbercx requested a review from sphuber June 12, 2024 09:03

mbercx added priority/critical-blocking must be resolved before next release labels Jun 12, 2024

sphuber requested changes Jun 12, 2024

View reviewed changes

mbercx force-pushed the improve/cli-conf-rabbitmq branch 3 times, most recently from 7577bc8 to 2540606 Compare June 18, 2024 07:27

mbercx requested a review from sphuber June 18, 2024 08:06

sphuber requested changes Jun 18, 2024

View reviewed changes

mbercx force-pushed the improve/cli-conf-rabbitmq branch 4 times, most recently from e87eb5c to da8db25 Compare June 19, 2024 05:47

mbercx added 3 commits June 19, 2024 13:07

mbercx force-pushed the improve/cli-conf-rabbitmq branch from da8db25 to a49e79a Compare June 19, 2024 11:27

mbercx requested a review from sphuber June 19, 2024 11:29

sphuber approved these changes Jun 19, 2024

View reviewed changes

sphuber merged commit c2ca642 into aiidateam:main Jun 19, 2024
12 checks passed

mbercx deleted the improve/cli-conf-rabbitmq branch June 19, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👌 Improve usage of `configure-rabbitmq` #6474

👌 Improve usage of `configure-rabbitmq` #6474

mbercx commented Jun 12, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

sphuber left a comment

mbercx commented Jun 18, 2024

mbercx commented Jun 18, 2024

sphuber Jun 18, 2024

mbercx Jun 18, 2024

sphuber Jun 18, 2024

mbercx Jun 19, 2024

sphuber Jun 19, 2024

mbercx Jun 19, 2024

mbercx commented Jun 19, 2024

sphuber commented Jun 19, 2024

👌 Improve usage of configure-rabbitmq #6474

👌 Improve usage of configure-rabbitmq #6474

Conversation

mbercx commented Jun 12, 2024

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

sphuber left a comment

Choose a reason for hiding this comment

mbercx commented Jun 18, 2024

mbercx commented Jun 18, 2024

sphuber Jun 18, 2024

Choose a reason for hiding this comment

mbercx Jun 18, 2024

Choose a reason for hiding this comment

sphuber Jun 18, 2024

Choose a reason for hiding this comment

mbercx Jun 19, 2024

Choose a reason for hiding this comment

sphuber Jun 19, 2024

Choose a reason for hiding this comment

mbercx Jun 19, 2024

Choose a reason for hiding this comment

mbercx commented Jun 19, 2024

sphuber commented Jun 19, 2024

👌 Improve usage of `configure-rabbitmq` #6474

👌 Improve usage of `configure-rabbitmq` #6474

codecov bot commented Jun 12, 2024 •

edited

Loading