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

Feat: Improve error message for unconfigured computers #4670

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

ramirezfranciscof
Copy link
Member

@ramirezfranciscof ramirezfranciscof commented Jan 22, 2021

Fixes #4645

I added a check to the get_authinfo method of the computer class that improves on the error raised by the call to authinfos.AuthInfo.objects(...).get(...) when a (computer, user) couple is not found for the requested case. Also added a test that checks for the correct error raised, and also that the critical pieces of information are present in the error message.

@ramirezfranciscof
Copy link
Member Author

@sphuber I still didn't add the check you wanted because I have a problem. In principle I would just need to call computer.get_authinfo( <SOME_USER> ) right after this line.

However, I'm not sure what user to provide or how to access it. In the case of the call inside the engine (the error shown in the issue #4645), the task was provided with the process as an input argument so it can get its underlying calcjob node and the user it belongs to. In the validator, however, I have no acces to the node (I only get the inputs and the context, but I couldn't find the class definition of the context to check if it has a reference to the calcjob node). Is there a way to either get that node or get the user from the profile that is currently loaded?

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #4670 (87aff8e) into develop (75310e0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4670      +/-   ##
===========================================
+ Coverage    79.52%   79.52%   +0.01%     
===========================================
  Files          519      519              
  Lines        37083    37087       +4     
===========================================
+ Hits         29486    29491       +5     
+ Misses        7597     7596       -1     
Flag Coverage Δ
django 74.24% <100.00%> (-0.01%) ⬇️
sqlalchemy 73.17% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
aiida/orm/computers.py 68.37% <100.00%> (+0.37%) ⬆️
aiida/orm/users.py 82.50% <0.00%> (+1.25%) ⬆️

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 75310e0...87aff8e. Read the comment docs.

@ltalirz
Copy link
Member

ltalirz commented Jan 27, 2021

Also these test failures seem to have been caused by the Open SSH upgrade - please continue with review/approval

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I have just only minor typo a dn one doubt where @sphuber might help, otherwise looks good to me

aiida/orm/computers.py Outdated Show resolved Hide resolved
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Good to go!

@ramirezfranciscof ramirezfranciscof merged commit a8ee41d into aiidateam:develop Mar 25, 2021
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.

Improve exception message when running on unconfigured computer
4 participants