aiidateam / aiida-core

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

CLI: Add the `verdi profile configure-rabbitmq` command #6454

Closed sphuber closed 3 weeks ago

sphuber commented 3 weeks ago

Now that profiles can be created without defining a broker, a command is needed that can add a RabbitMQ connection configuration. The new command verdi profile configure-rabbitmq enables a broker for a profile if it wasn't already, and allows configuring the connection parameters.

codecov[bot] commented 3 weeks ago

Codecov Report

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

Project coverage is 77.75%. Comparing base (ef60b66) to head (93c0d01). Report is 46 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6454 +/- ## ========================================== + Coverage 77.51% 77.75% +0.25% ========================================== Files 560 561 +1 Lines 41444 41768 +324 ========================================== + Hits 32120 32473 +353 + Misses 9324 9295 -29 ```

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

sphuber commented 3 weeks ago

Thanks for the review @GeigerJ2

I also basically ran the steps of test_configure_rabbitmq via the command line: Trying to submit the ArithmeticAddCalculation with the non-RMQ profile led to the expected:

Indeed, and in #6465 I improve that error message to make it clear that is because there probably is no broker.

This would just be for completeness, as I don't think anybody would ever actually do that, so also fine to not provide it.

Like you said, I don't see a use case for now. So I prefer not to add it. If we ever come across it, it should be easy to add later on.

GeigerJ2 commented 3 weeks ago

Indeed, and in #6465 I improve that error message to make it clear that is because there probably is no broker.

That's good to know, then I'll also give that my review now :D

Like you said, I don't see a use case for now. So I prefer not to add it. If we ever come across it, it should be easy to add later on.

Fine for me!

mbercx commented 3 weeks ago

Hi guys, sorry for being late to the party. I'm dogfooding right now and have a few questions/comments.

Should non-interactive be the default?

One of the motivations for creating this command is that we want to give new users who started with verdi presto the possibility of upgrading their profile with RabbitMQ. However, if a user now tries to use this command to do so, the interactive mode is enabled, and they are asked for a lot of input they will not understand.

Sure, they can just press enter for the defaults, but my experience is that even just asking for these settings will give users pause and make them uncertain what to do.

Why not make non-interactive the default execution mode, and add an "interactive" option instead? In the case of RabbitMQ configuration, I think we have a set of sensible defaults that cover +90% of use cases.

Check if the configuration was successful

If I try to configure RabbitMQ with a set of incorrect options, the command will execute fine (without a success message, something we might want to add as well). However, attempting to run verdi status afterwards will raise a ConnectionError:

❯ verdi status
 ✔ version:     AiiDA v2.5.1.post0
 ✔ config:      /Users/mbercx/project/core/.aiida
 ✔ profile:     presto
 ✔ storage:     SqliteDosStorage[/Users/mbercx/project/core/.aiida/repository/sqlite_dos_0026c04e2c1e4117a926b82be5935866]: open
Traceback (most recent call last):
[...]
[Trimmed for conciseness, see full Traceback below]
[...]
ConnectionError: [Errno 61] Connect call failed ('127.0.0.1', 5672)
Full Traceback ```python Traceback (most recent call last): File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aiormq/connection.py", line 228, in connect self.reader, self.writer = await asyncio.open_connection( File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/streams.py", line 48, in open_connection transport, _ = await loop.create_connection( File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_events.py", line 1076, in create_connection raise exceptions[0] File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_events.py", line 1060, in create_connection sock = await self._connect_sock( File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_events.py", line 969, in _connect_sock await self.sock_connect(sock, address) File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/selector_events.py", line 501, in sock_connect return await fut File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/selector_events.py", line 541, in _sock_connect_cb raise OSError(err, f'Connect call failed {address}') ConnectionRefusedError: [Errno 61] Connect call failed ('127.0.0.1', 5672) The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/Users/mbercx/.aiida_venvs/core/bin/verdi", line 8, in sys.exit(verdi()) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/click/core.py", line 1157, in __call__ return self.main(*args, **kwargs) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/click/core.py", line 1078, in main rv = self.invoke(ctx) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/click/core.py", line 1688, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/Users/mbercx/project/core/git/aiida-core/src/aiida/cmdline/groups/verdi.py", line 117, in invoke return ctx.invoke(self.callback, **ctx.params) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/click/core.py", line 783, in invoke return __callback(*args, **kwargs) File "/Users/mbercx/project/core/git/aiida-core/src/aiida/cmdline/commands/cmd_status.py", line 135, in verdi_status message = f'Unable to connect to broker: {broker}' File "/Users/mbercx/project/core/git/aiida-core/src/aiida/brokers/rabbitmq/broker.py", line 37, in __str__ return f'RabbitMQ v{self.get_rabbitmq_version()} @ {self.get_url()}' File "/Users/mbercx/project/core/git/aiida-core/src/aiida/brokers/rabbitmq/broker.py", line 122, in get_rabbitmq_version return parse(self.get_communicator().server_properties['version'].decode('utf-8')) File "/Users/mbercx/project/core/git/aiida-core/src/aiida/brokers/rabbitmq/broker.py", line 52, in get_communicator self._communicator = self._create_communicator() File "/Users/mbercx/project/core/git/aiida-core/src/aiida/brokers/rabbitmq/broker.py", line 64, in _create_communicator self._communicator = RmqThreadCommunicator.connect( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/kiwipy/rmq/threadcomms.py", line 52, in connect comm = cls( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/kiwipy/rmq/threadcomms.py", line 109, in __init__ self._communicator: communicator.RmqCommunicator = self._loop_scheduler.await_( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/pytray/aiothreads.py", line 164, in await_ return self.await_submit(awaitable).result(timeout=self.task_timeout) File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 458, in result return self.__get_result() File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result raise self._exception File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/pytray/aiothreads.py", line 178, in coro res = await awaitable File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/kiwipy/rmq/communicator.py", line 599, in async_connect connection = await connection_factory(**connection_params) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aio_pika/robust_connection.py", line 271, in connect_robust return await connect( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aio_pika/connection.py", line 333, in connect await connection.connect( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aio_pika/robust_connection.py", line 127, in connect result = await super().connect( File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aio_pika/connection.py", line 120, in connect self.connection = await asyncio.wait_for( File "/opt/homebrew/Cellar/python@3.10/3.10.14/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/tasks.py", line 408, in wait_for return await fut File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aio_pika/connection.py", line 105, in _make_connection connection = await aiormq.connect(self.url, **kwargs) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aiormq/connection.py", line 542, in connect await connection.connect(client_properties or {}) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aiormq/base.py", line 168, in wrap return await self.create_task(func(self, *args, **kwargs)) File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aiormq/base.py", line 25, in __inner return await self.task File "/Users/mbercx/.aiida_venvs/core/lib/python3.10/site-packages/aiormq/connection.py", line 232, in connect raise ConnectionError(*e.args) from e ConnectionError: [Errno 61] Connect call failed ('127.0.0.1', 5672) ```

Would it be possible to check if the configuration was successful? It's true that the user might reconfigure RabbitMQ while e.g. the service is not running, but raising a warning might be a good idea.

On a different note, we should probably also capture that ConnectionError when running verdi status.

mbercx commented 2 weeks ago

On a different note, we should probably also capture that ConnectionError when running verdi status.

This scope creep should be dealt with by https://github.com/aiidateam/aiida-core/pull/6473