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

RabbitMQ: remove validation of broker_parameters from profile #4542

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 4, 2020

Fixes #4540

This validation was added as an attempt to help users with detecting
invalid parameters in the broker_parameters dictionary of a profile,
but aiida-core internally has no real limitations here. It is the
libraries underneath that decide what is acceptable and this can differ
from library to library plus it is not always clear. For example,
currently we use topika and pika which behave different from
aiormq which will be replacing them soon once tornado is replaced
with asyncio. It is best to not limit the options on aiida-core's
side and just let it fail downstream as to not artificially limit the
parameters that might be perfectly acceptable by the libraries
downstream.

This validation was added as an attempt to help users with detecting
invalid parameters in the `broker_parameters` dictionary of a profile,
but `aiida-core` internally has no real limitations here. It is the
libraries underneath that decide what is acceptable and this can differ
from library to library plus it is not always clear. For example,
currently we use `topika` and `pika` which behave different from
`aiormq` which will be replacing them soon once `tornado` is replaced
with `asyncio`. It is best to not limit the options on `aiida-core`'s
side and just let it fail downstream as to not artificially limit the
parameters that might be perfectly acceptable by the libraries
downstream.
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #4542 into release/1.4.3 will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/1.4.3    #4542      +/-   ##
=================================================
+ Coverage          79.23%   79.23%   +0.01%     
=================================================
  Files                475      475              
  Lines              34832    34828       -4     
=================================================
- Hits               27595    27592       -3     
+ Misses              7237     7236       -1     
Flag Coverage Δ
django 73.08% <0.00%> (-<0.01%) ⬇️
sqlalchemy 72.29% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/manage/external/rmq.py 68.54% <0.00%> (-1.35%) ⬇️
aiida/transports/plugins/local.py 81.29% <0.00%> (-0.25%) ⬇️
aiida/engine/daemon/client.py 73.57% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6c6cc4...ba0a3c2. Read the comment docs.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 5, 2020

@muhrin @ramirezfranciscof I had the contact person at LLNL test this branch and they confirmed it still works with their environment, so this should be good to merge. Then I will prepare the 1.4.3 release.

Copy link
Contributor

@muhrin muhrin 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

@sphuber sphuber merged commit 0a7039e into aiidateam:release/1.4.3 Nov 5, 2020
@sphuber sphuber deleted the fix/4540/broker-parameters-validation branch November 5, 2020 11:02
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.

None yet

2 participants