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

CalcJobResultManager: fix bug that broke tab completion #4187

Merged
merged 3 commits into from Jun 23, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 19, 2020

Fixes #4184

For tab-completion to work properly, the __getattr__ method need to
properly return an AttributeError if the corresponding attribute does
not exist, however, it was accidentally raising a KeyError since the
implementation caught an AttributeError but the dereferencing was
using __getitem__ so would never raise an AttributeError.

Likewise, the __getitem__ implementation was incorrectly returning an
AttributeError instead of a KeyError making it behave different from
any other mapping-like data structure.

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #4187 into develop will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4187      +/-   ##
===========================================
+ Coverage    78.94%   78.97%   +0.03%     
===========================================
  Files          467      467              
  Lines        34508    34512       +4     
===========================================
+ Hits         27240    27253      +13     
+ Misses        7268     7259       -9     
Flag Coverage Δ
#django 70.88% <100.00%> (+0.02%) ⬆️
#sqlalchemy 71.78% <100.00%> (+0.04%) ⬆️
Impacted Files Coverage Δ
aiida/orm/utils/managers.py 91.81% <ø> (ø)
aiida/orm/utils/calcjob.py 95.84% <100.00%> (+11.75%) ⬆️
aiida/transports/plugins/local.py 80.21% <0.00%> (-0.25%) ⬇️
aiida/orm/utils/log.py 93.34% <0.00%> (+16.67%) ⬆️

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 3339fd5...b306494. Read the comment docs.

@@ -97,17 +97,17 @@ def __getattr__(self, name):
"""
try:
return self.get_results()[name]
except AttributeError:
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to guard also for any exception that can happen in get_results, both here and below.

I found at least two possibilities, both a ValueError:

ValueError: cannot load results because process class cannot be loaded: no process type for CalcJobNode<None>: cannot recreate process class

ValueError: cannot load results as the default node could not be retrieved: no neighbor with the label output_parameters found

I am not sure if there is still some case if an AttributeError can still be raised.

Copy link
Member

Choose a reason for hiding this comment

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

For tests, in the first case I just created an unstored CalcJobNode(); in the second one, a calculation that excepted and hand no output node.

"""
try:
return self.get_results()[name]
except AttributeError:
raise AttributeError("Default result node<{}> does not contain key '{}'".format(self._result_node.pk, name))
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

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

Also here

For tab-completion to work properly, the `__getattr__` method need to
properly return an `AttributeError` if the corresponding attribute does
not exist, however, it was accidentally raising a `KeyError` since the
implementation caught an `AttributeError` but the dereferencing was
using `__getitem__` so would never raise an `AttributeError`.

Likewise, the `__getitem__` implementation was incorrectly returning an
`AttributeError` instead of a `KeyError` making it behave different from
any other mapping-like data structure.
@sphuber sphuber force-pushed the fix/4184/calcjob-res-tabcomplete branch from 8ceb61f to c7e27e2 Compare June 19, 2020 14:25
@sphuber sphuber merged commit af153c9 into aiidateam:develop Jun 23, 2020
@sphuber sphuber deleted the fix/4184/calcjob-res-tabcomplete branch June 23, 2020 09:42
unkcpz pushed a commit to unkcpz/aiida-core that referenced this pull request Jul 7, 2020
…4187)

For tab-completion to work properly, the `__getattr__` method need to
properly return an `AttributeError` if the corresponding attribute does
not exist, however, it was accidentally raising a `KeyError` since the
implementation caught an `AttributeError` but the dereferencing was
using `__getitem__` so would never raise an `AttributeError`.

Likewise, the `__getitem__` implementation was incorrectly returning an
`AttributeError` instead of a `KeyError` making it behave different from
any other mapping-like data structure.
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.

Tab completion not working for CalcJob.res
2 participants