Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 5, 2024

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.

@sphuber sphuber requested a review from mbercx June 5, 2024 15:47
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

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

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.
@sphuber sphuber force-pushed the feature/verdi-profile-configure-rabbitmq branch from d8468f2 to 93c0d01 Compare June 7, 2024 11:32
Copy link
Contributor

@GeigerJ2 GeigerJ2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, as well. Just left some minor comments. 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:

AssertionError: runner does not have a controller

while after configuring RMQ via verdi profile configure-rabbitmq it could be submitted and ran through successfully.

Just wondering if we should also add an option to disable RMQ again? Now, once RMQ is configured there's no going back, I suppose? This would just be for completeness, as I don't think anybody would ever actually do that, so also fine to not provide it.

@sphuber
Copy link
Contributor Author

sphuber commented Jun 7, 2024

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
Copy link
Contributor

GeigerJ2 commented Jun 7, 2024

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!

@sphuber sphuber merged commit 202a3ec into aiidateam:main Jun 7, 2024
12 checks passed
@sphuber sphuber deleted the feature/verdi-profile-configure-rabbitmq branch June 7, 2024 12:49
@mbercx
Copy link
Member

mbercx commented Jun 8, 2024

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
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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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 <module>
    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/[email protected]/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/[email protected]/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/[email protected]/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
Copy link
Member

mbercx commented Jun 11, 2024

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

This scope creep should be dealt with by #6473

mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants