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

Engine: Improve error message when submitting without broker #6465

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 6, 2024

Fixes #6374

The aiida.engine.launch.submit method was just raising a vague AssertionError in case the runner did not have a communicator, which is the case if it was constructed without a communicator which in turn happens for profiles that do not configure a broker.

Since profiles without brokers are now supported and users are bound to try to submit anyway, the error message should be clearer.

@sphuber
Copy link
Contributor Author

sphuber commented Jun 6, 2024

Perhaps the error message would be even better with a link to the docs which gives even more background information and tells users how they can configure the broker. That would require #6455 to be merged before that, because currently those docs don't exist yet.

Still, I am always a bit hesitant to add docs links in error messages since they can easily go stale. I know that SQLalchemy solves this somewhat by using shortened urls the referent of which can then be updated with redirects in case the documentation links change.

@sphuber sphuber requested a review from mbercx June 6, 2024 21:51
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.76%. Comparing base (ef60b66) to head (0d6397f).
Report is 114 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6465      +/-   ##
==========================================
+ Coverage   77.51%   77.76%   +0.26%     
==========================================
  Files         560      561       +1     
  Lines       41444    41796     +352     
==========================================
+ Hits        32120    32500     +380     
+ Misses       9324     9296      -28     

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

@mbercx mbercx added the priority/critical-blocking must be resolved before next release label Jun 19, 2024
The `aiida.engine.launch.submit` method was just raising a vague
`AssertionError` in case the runner did not have a communicator, which
is the case if it was constructed without a communicator which in turn
happens for profiles that do not configure a broker.

Since profiles without brokers are now supported and users are bound to
try to submit anyway, the error message should be clearer.
@sphuber sphuber force-pushed the fix/6374/improve-error-submit-no-broker branch from 351c671 to 0d6397f Compare June 28, 2024 08:35
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.

Just had a look (with the new error message). Good to go for me!

@sphuber sphuber merged commit 56995e1 into aiidateam:main Jun 28, 2024
9 checks passed
@sphuber sphuber deleted the fix/6374/improve-error-submit-no-broker branch June 28, 2024 09:04
mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
…am#6465)

The `aiida.engine.launch.submit` method was just raising a vague
`AssertionError` in case the runner did not have a communicator, which
is the case if it was constructed without a communicator which in turn
happens for profiles that do not configure a broker.

Since profiles without brokers are now supported and users are bound to
try to submit anyway, the error message should be clearer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/critical-blocking must be resolved before next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error when user tries to submit workflow without rabbitmq broker
4 participants