-
Notifications
You must be signed in to change notification settings - Fork 623
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
Use importlib_metadata for 3.8 and 3.9 only #4177
Conversation
95e887f
to
71dc0d2
Compare
opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py
Outdated
Show resolved
Hide resolved
bf0f738
to
67b3367
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think before landing this we need to update -contrib to use opentelemetry.util._importlib_metadata
so everything is aligned
opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Riccardo Magliocchetti <[email protected]>
Actually, we need this into core before we change contrib to use I have a PR numbered 2854 in contrib (which is on top of another contrib PR numbered 2181 tha does exactly that. Contrib 2854 PR does exactly that, it removes any direct usage of |
__all__ = ["entry_points", "version", "EntryPoint", "EntryPoints"] | ||
from sys import version_info | ||
|
||
if version_info < (3, 10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of not using importlib_metadata
for newer Python versions?
I think this branch alone would solve all of our problems and you wouldn't need the custom def entry_points()
since importlib_metadata has already provided a python version agnostic API for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed something, but I see the main benefit as simply relying on a built-in module instead of external dependency, given that importlib.metadata
is no longer provisional since Python 3.10 and we have a stable API. Also minimize issues related to the pinned version of importlib-metadata
package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely see your point, Emidio, but my vote is for the simpler solution here until importlib_metadata
is no longer needed. Seems like the cost is quite small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on only relying on one library throughout the codebase. I am FOR using importlib_metadata
for now for the reasons @aabmass pointed out in terms of consistency. I am concerned that future contributions will not always use this library and use the standard built-in one instead, after which we must always remember as reviewers that we still support Python versions in which that will not be possible to do. This might be a calculated risk, as the contributions related to these libraries are relatively small, so if the vast majority of us agrees on that and the benefits of using a single, simple library out weigh the review/mental overhead of forcing users to use the backport then we can go ahead with this suggestion.
Keep in mind that if we do somehow want to go with the "custom built in entrypoint()" function of the current PR as of writing this review, our message to contributors in the future will be "use this custom thing we built", which technically is amounts to the same amount of headache/overhead as the previous problem.
""" | ||
Same as entry_points but requires at least one argument | ||
|
||
For Python 3.8 or 3.9: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a proposal for a unified solution for the entry_points
API in the pkg_resources removal PR in the contrib repo which works for all currently supported python versions.
maybe this one could be used here?
Closing as we decided in 2024-09-12 SIG call to just use importlib_metadata everywhere until we need it. |
Fixes #3234