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

ProcessNode.is_valid_cache: catch AttributeError for unloadable identifier #5222

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 5, 2021

Fixes #5220

The is_valid_cache property is supposed to only raise a ValueError
when the function or class represented by the process_type identifier
cannot be loaded. However, for a normal Python identifier of an existing
module but with a function or class that does not exist in that module
an AttributeError will be thrown which was not caught and converted.

…dentifier

The `is_valid_cache` property is supposed to only raise a `ValueError`
when the function or class represented by the `process_type` identifier
cannot be loaded. However, for a normal Python identifier of an existing
module but with a function or class that does not exist in that module
an `AttributeError` will be thrown which was not caught and converted.
@sphuber
Copy link
Contributor Author

sphuber commented Nov 5, 2021

@ltalirz I think this should fix your issue

@ltalirz
Copy link
Member

ltalirz commented Nov 5, 2021

Thanks a lot, I'll check it later today

@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #5222 (555a641) into develop (8acdc24) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5222      +/-   ##
===========================================
+ Coverage    81.21%   81.21%   +0.01%     
===========================================
  Files          532      532              
  Lines        37357    37357              
===========================================
+ Hits         30336    30337       +1     
+ Misses        7021     7020       -1     
Flag Coverage Δ
django 76.07% <100.00%> (+0.01%) ⬆️
sqlalchemy 75.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/nodes/process/process.py 86.74% <100.00%> (+1.03%) ⬆️
aiida/transports/util.py 65.63% <0.00%> (-6.25%) ⬇️
aiida/transports/plugins/local.py 81.66% <0.00%> (+0.26%) ⬆️

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 8acdc24...555a641. Read the comment docs.

ltalirz
ltalirz previously approved these changes Nov 5, 2021
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks a lot @sphuber - there's a typo but up to you whether you want to fix it.

just so I understand: the fix of the issue is implicit here, since ValueErrors will be dealt with and so the failure here is no longer catastrophic, is that right?

Shall we open a separate issue for getting a more specific process type for calcfunctions that are not defined at the module level?

tests/orm/node/test_node.py Outdated Show resolved Hide resolved
@sphuber
Copy link
Contributor Author

sphuber commented Nov 6, 2021

thanks a lot @sphuber - there's a typo but up to you whether you want to fix it.

Thanks @ltalirz away from my computer now, will take of this at the end of the weekend.

just so I understand: the fix of the issue is implicit here, since ValueErrors will be dealt with and so the failure here is no longer catastrophic, is that right?

Exactly, downstream is correctly catching ValueError but nothing else.

Shall we open a separate issue for getting a more specific process type for calcfunctions that are not defined at the module level?

Yes, thatis still an unsolved issue so good to open one.

Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>
@sphuber sphuber merged commit 0772f33 into aiidateam:develop Nov 8, 2021
@sphuber sphuber deleted the fix/5220/is-valid-cache-attribute-error branch November 8, 2021 10:07
unkcpz pushed a commit to unkcpz/aiida-core that referenced this pull request Jan 10, 2022
…dentifier (aiidateam#5222)

The `is_valid_cache` property is supposed to only raise a `ValueError`
when the function or class represented by the `process_type` identifier
cannot be loaded. However, for a normal Python identifier of an existing
module but with a function or class that does not exist in that module
an `AttributeError` will be thrown which was not caught and converted.
edan-bainglass pushed a commit to edan-bainglass/aiida-core that referenced this pull request Dec 23, 2023
…dentifier (aiidateam#5222)

The `is_valid_cache` property is supposed to only raise a `ValueError`
when the function or class represented by the `process_type` identifier
cannot be loaded. However, for a normal Python identifier of an existing
module but with a function or class that does not exist in that module
an `AttributeError` will be thrown which was not caught and converted.
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.

Process.process_class raises on calcfunction closure
2 participants