get_entry_point: add proper deprecation pathway for old entry points
#5206
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5206 +/- ##
===========================================
+ Coverage 81.21% 81.22% +0.01%
===========================================
Files 532 532
Lines 37347 37337 -10
===========================================
- Hits 30329 30322 -7
+ Misses 7018 7015 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
sphuber
force-pushed
the
fix/5202/deprecation-support-get-entry-point
branch
from
5637cfe to
ab8d4a4
Compare
sphuber
changed the title
get_entry_point: integrate deprecation pathway for old entry pointsget_entry_point: add proper deprecation pathway for old entry points
aiida/plugins/entry_point.py
Outdated
|
|
||
| This method should be removed in ``aiida-core==3.0`` | ||
| """ | ||
| deprecated_entry_points_mapping = { |
There was a problem hiding this comment.
I would move this out of the function, so you don't have to recreate the dictionary on every function call. Probably a marginal gain, but don't want to slow down the CLI
There was a problem hiding this comment.
Thanks! Looks good to me in general, my only question is if it makes sense to have two separate functions doing this. I understand that maybe this is somehow "inherited" of the previous mechanism in which warn_deprecated_entry_point was called by many different factories with different lists of possible entry-points. But now that this is only required in get_entry_point, having that intermediate convert_potentially_deprecated_entry_point that only selects and passes the correct list of entry-points feels like bit forced separation of concerns. Could these be made a single function?
sphuber
force-pushed
the
fix/5202/deprecation-support-get-entry-point
branch
from
ab8d4a4 to
ab098e5
Compare
|
Thanks @chrisjsewell and @ramirezfranciscof , addressed both your comments. |
aiida/plugins/entry_point.py
Outdated
| from warnings import warn | ||
|
|
||
| from aiida.common.warnings import AiidaDeprecationWarning |
There was a problem hiding this comment.
Is there a reason to not have these as top-level imports?
There was a problem hiding this comment.
Because this function is the only place that uses them and when the function gets removed the obsolete imports are also automatically taken care off. Not a huge deal, but why not?
There was a problem hiding this comment.
Meh, pylint will flag unused imports, so that's not an issue.
Yeh not a big deal, but that's the python coding standard that we use (outside reasons like CLI response and circular imports), whereas "single use imports should be internal to functions" is not.
Up to you
There was a problem hiding this comment.
Thanks! Looks good to me! Feel free to merge after you finish your discussion with @chrisjsewell
BTW this seems the best way to do multi-people review, right? I mean, each approves separately and it is understood the author would wait for approval from all before merging (this would be opposed to something like giving "approving" messages as comment but the last reviewer leaves theirs as "approve")
Yes, I wouldn't go and merge this before having resolved the conversation with @chrisjsewell |
All entry points that ship with aiida-core were deprecated for v2.0 as they were changed to be properly prefixed with core.. To make sure the old entry points would still be automatically loaded, the factories were updated to automatically catch them, print a deprecation warning, and load the new entry point instead. However, the factory was not the correct place to put this logic, since the `get_entry_point` method, which the factories call and is the lowest function in the stack that actually retrieves the entry point, can also be called directly, circumventing the deprecation mechanic added to the factories. This would result in the deprecated entry points raising an exception when being loaded, for example in the parameter types of the command line that have support for specific entry points, such as the `IdentifierParamType`. The solution is to move the deprecation mechanic from the factories to the lowest layer of `get_entry_point`.
sphuber
force-pushed
the
fix/5202/deprecation-support-get-entry-point
branch
from
d41e424 to
c858832
Compare
Fixes #5202
All entry points that ship with aiida-core were deprecated for v2.0
as they were changed to be properly prefixed with core.. To make sure
the old entry points would still be automatically loaded, the factories
were updated to automatically catch them, print a deprecation warning,
and load the new entry point instead.
However, the factory was not the correct place to put this logic, since
the
get_entry_pointmethod, which the factories call and is the lowestfunction in the stack that actually retrieves the entry point, can also
be called directly, circumventing the deprecation mechanic added to the
factories. This would result in the deprecated entry points raising an
exception when being loaded, for example in the parameter types of the
command line that have support for specific entry points, such as the
IdentifierParamType.The solution is to move the deprecation mechanic from the factories to
the lowest layer of
get_entry_point.