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

ORM: fix deprecation warning always being shown in link managers #5011

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jul 6, 2021

Fixes #5010

The link managers for the Node class which are used for the inputs
and outputs attributes and facilitate the tab-completion of incoming
and outgoing links, was recently changed to deprecate the direct use of
double underscores in link labels in favor of treating them as normal
nested dictionaries. The deprecation warning was thrown whenever the
label contained a double underscore, but this would therefore also
trigger on dunder methods, which is not desirable behaviour.

This inaccuracy manifested itself in the deprecation method being
printed even when just activating the tab-completion on node.outputs
or node.inputs without even specifying a label with a double
underscore. It is not fully understood how _get_node_by_link_label is
called in doing this, but it seems some caching mechanism is calling the
__wrapped__ attribute on the link manager, which in turn triggers the
deprecation warning. An additional clause in the condition to exclude
dunder methods fixes the behaviour.

@sphuber sphuber requested review from unkcpz and mbercx July 6, 2021 07:50
@sphuber
Copy link
Contributor Author

sphuber commented Jul 6, 2021

I think this affects v1.6.* so we might want to consider a patch release with this fix cherry-picked. I also think there have been some other bug fixes on develop that also affect the last release branch that we might want to include.

@chrisjsewell
Copy link
Member

cheers, I will have a look at these later today (once I have done my tutorial stint)

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #5011 (d292b4c) into develop (58df44c) will decrease coverage by 0.01%.
The diff coverage is 71.43%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5011      +/-   ##
===========================================
- Coverage    80.13%   80.12%   -0.00%     
===========================================
  Files          515      515              
  Lines        36694    36699       +5     
===========================================
+ Hits         29400    29403       +3     
- Misses        7294     7296       +2     
Flag Coverage Δ
django 74.60% <71.43%> (+0.01%) ⬆️
sqlalchemy 73.52% <71.43%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
aiida/orm/utils/managers.py 90.59% <71.43%> (-1.91%) ⬇️

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 58df44c...d292b4c. Read the comment docs.

@sphuber sphuber force-pushed the fix/5010/deprecation-warning-link-manager branch from 151386d to cb60226 Compare July 6, 2021 15:00
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber . I assume the added test cases are in order to test behaviour of return catch, the original not_existent is from nested__not_existent, so you may need to change either link_label='remote_folder' or separate them from the previous one.

tests/orm/utils/test_managers.py Outdated Show resolved Hide resolved
tests/orm/utils/test_managers.py Outdated Show resolved Hide resolved
tests/orm/utils/test_managers.py Outdated Show resolved Hide resolved
@sphuber sphuber force-pushed the fix/5010/deprecation-warning-link-manager branch from cb60226 to 95c6089 Compare July 8, 2021 10:13
@sphuber sphuber requested a review from unkcpz July 8, 2021 10:14
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Only a minor request, all looks good to me.

Hoping it get merged soon, when looking at this I just find because of too much of this depracated complaint I turn off the ipython warning.

tests/orm/utils/test_managers.py Outdated Show resolved Hide resolved
The link managers for the `Node` class which are used for the `inputs`
and `outputs` attributes and facilitate the tab-completion of incoming
and outgoing links, was recently changed to deprecate the direct use of
double underscores in link labels in favor of treating them as normal
nested dictionaries. The deprecation warning was thrown whenever the
label contained a double underscore, but this would therefore also
trigger on dunder methods, which is not desirable behaviour.

This inaccuracy manifested itself in the deprecation method being
printed even when just activating the tab-completion on `node.outputs`
or `node.inputs` without even specifying a label with a double
underscore. It is not fully understood how `_get_node_by_link_label` is
called in doing this, but it seems some caching mechanism is calling the
`__wrapped__` attribute on the link manager, which in turn triggers the
deprecation warning. An additional clause in the condition to exclude
dunder methods fixes the behaviour.
@sphuber sphuber force-pushed the fix/5010/deprecation-warning-link-manager branch from 95c6089 to d292b4c Compare July 8, 2021 14:58
@sphuber sphuber merged commit 53c5564 into aiidateam:develop Jul 8, 2021
@sphuber sphuber deleted the fix/5010/deprecation-warning-link-manager branch July 8, 2021 15:42
sphuber added a commit that referenced this pull request Aug 9, 2021
The link managers for the `Node` class which are used for the `inputs`
and `outputs` attributes and facilitate the tab-completion of incoming
and outgoing links, was recently changed to deprecate the direct use of
double underscores in link labels in favor of treating them as normal
nested dictionaries. The deprecation warning was thrown whenever the
label contained a double underscore, but this would therefore also
trigger on dunder methods, which is not desirable behaviour.

This inaccuracy manifested itself in the deprecation method being
printed even when just activating the tab-completion on `node.outputs`
or `node.inputs` without even specifying a label with a double
underscore. It is not fully understood how `_get_node_by_link_label` is
called in doing this, but it seems some caching mechanism is calling the
`__wrapped__` attribute on the link manager, which in turn triggers the
deprecation warning. An additional clause in the condition to exclude
dunder methods fixes the behaviour.

Cherry-pick: 53c5564
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 for the outputs attribute of a Node issues a warning
3 participants