search

open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.81k stars 628 forks source link

`importlib-metadata` breaks code #3234

Open lzchen opened 1 year ago

lzchen commented 1 year ago 
          Was there a need to unconditionally use `importlib-metadata`, even on Python >= 3.10? This change broke my application framework (Asphalt) because it does `isinstance(value, EntryPoint)`, checking against `importlib.metadata.EntryPoint`, but after `importlib_metadata` is imported, iterating entry points (even through the stdlib facilities) yields `EntryPoint` instances from that third party library, which are NOT compatible with the stdlib ones!

EDIT: I see that importlib.metadata.EntryPoint is not directly documented in the API docs.

Originally posted by @agronholm in https://github.com/open-telemetry/opentelemetry-python/issues/3167#issuecomment-1481335345

agronholm commented 1 year ago

Given that importlib.metadata is no longer provisional in Python 3.10, its API is stable, so the workaround should only be used for Python 3.9 and earlier.

mvilanova commented 1 year ago

Pinning to 1.16.0 temporarily fixed the issue for us in Python 3.11.

aabmass commented 1 year ago

Given that importlib.metadata is no longer provisional in Python 3.10, its API is stable, so the workaround should only be used for Python 3.9 and earlier.

That makes sense to me 👍

Just to make sure I understand @agronholm, your original description of the issue does not sound like an issue with OpenTelemetry's usage of the library. It kind of sounds like an issue in either Asphalt (you mentioned EntryPoint is not documented looks fixed in this commit) in combination with some weird side effects between importlib.metadata and importlib-metadata.

Is that right?

agronholm commented 1 year ago

Just to make sure I understand @agronholm, your original description of the issue does not sound like an issue with OpenTelemetry's usage of the library. It kind of sounds like an issue in either Asphalt (you mentioned EntryPoint is not documented looks fixed in this commit) in combination with some weird side effects between importlib.metadata and importlib-metadata.

Yeah, it was only after upgrading the opentelemetry libs to 0.38b0 that I encountered this issue. Asphalt itself only uses the backport on Python 3.9 and earlier. It's not needed on 3.10 or later, and is just extra baggage there. I don't know if anybody is doing isinstance() with EntryPoint elsewhere, but if they do, they'll encounter the same issue.

agronholm commented 1 year ago

At any rate, the latest Asphalt no longer uses the EntryPoint class for comparisons so it's not an issue for me any longer.

aabmass commented 1 year ago

@mvilanova did you encounter this outside of Asphalt? If not I think we should close this bug and open a separate one to make it conditional import for python<3.10

jsjeannotte commented 1 year ago

Hi folks! Any update on this? We're still stuck with pinning to 1.16.0 and would love to un-pin.

lzchen commented 1 year ago

@jsjeannotte

There was a pr that was closed so this issue still exists. @ocelotl for visibility. Seems like import-metadata still causing headaches :D

lzchen commented 1 year ago

@jsjeannotte

Can you elaborate if you are experiencing breaking changes? Can you try upgrading to the latest opentelemetry to see if importlib-metadata is causing you issues?

jsjeannotte commented 1 year ago

I'm reaching out internally for the details, but a colleague using one of my python library reported this issue again when I unpinned opentelemetry-api on Tuesday (and picked up 1.20.0). They're running on Python 3.11. This is the stack trace they sent me:

  File "/apps/sirtdispatch/src/dispatch/extensions.py", line 3, in <module>
    import nflxlog
  File "/apps/sirtdispatch/lib/python3.11/site-packages/nflxlog/__init__.py", line 14, in <module>
    from opentelemetry import trace
  File "/apps/sirtdispatch/lib/python3.11/site-packages/opentelemetry/trace/__init__.py", line 87, in <module>
    from opentelemetry import context as context_api
  File "/apps/sirtdispatch/lib/python3.11/site-packages/opentelemetry/context/__init__.py", line 25, in <module>
    from opentelemetry.util._importlib_metadata import entry_points
  File "/apps/sirtdispatch/lib/python3.11/site-packages/opentelemetry/util/_importlib_metadata.py", line 17, in <module>
    from importlib_metadata import (  # type: ignore
ImportError: cannot import name 'EntryPoints' from 'importlib_metadata' (/apps/sirtdispatch/lib/python3.11/site-packages/importlib_metadata/__init__.py)

My guess is another dependency in their project is pinning to an incompatible version of importlib_metadata? Anyhow, I'm wondering if proactively fixing opentelemetry-api by using importlib.metadata for Python > 3.7 and conditionally including importlib_metadata for Python 3.7 would remove the issue completely.

agronholm commented 1 year ago

Is there a reason for continuing to support a Python release (3.7) well past its EOL date?

aabmass commented 1 year ago

My guess is another dependency in their project is pinning to an incompatible version of importlib_metadata?

It would be helpful to figure out what is going on in their dependency tree and what version is actually being installed. We are not pinning to a single version but to 6.x https://github.com/open-telemetry/opentelemetry-python/blob/d054dff47d2da663a39b9656d106c3d15f344269/opentelemetry-api/pyproject.toml#L31

Anyhow, I'm wondering if proactively fixing opentelemetry-api by using importlib.metadata for Python > 3.7 and conditionally including importlib_metadata for Python 3.7 would remove the issue completely.

I don't think dropping Python 3.7 would actually fix things because importlib.metadata has breaking changes between python versions, which is exactly the reason that people are pinning to a major version of the PyPI package. See this note and the compatibility matrix here. IIRC @ocelotl was running into a lot of issues with missing imports and features. By using importlib-metadata we can just program against one API regardless of python version.

That said, I just noticed https://pypi.org/project/backports.entry-points-selectable/ which might support the feature set we need and be less invasive.

mlasch commented 5 months ago

I am also experiencing issues with the pinned version of importlib-metadata. Because the opentelemetry packages depend on importlib-metadata, it makes it impossible to use the importlib.metadata library from the standard library in my projects.

I work with a recent Yocto build with Python 3.11 and I cannot easily use the external importlib-metadata package.

The build package from pypa has a good solution which might work here as well: https://github.com/pypa/build/blob/main/src/build/_compat/importlib.py They use the correct metadata module depending on the Python version and package availability.

xrmx commented 2 months ago

@mlasch Is this https://github.com/python/importlib_metadata/issues/489 the issue (fixed in 8.1.0) that wouldn't let you use importlib-metadata in yocto?

mlasch commented 3 weeks ago

@xrmx No, https://github.com/python/importlib_metadata/issues/489 is not the issue. The problem is not that I am unable to use the backported package importlib-metadata at all in Yocto. The problem is that I'm building an embedded system and unnecessary dependencies are generally not preferred. importlib is already available from the standard library. I think the correct solution would be to use importlib.metadata for Python versions where it is available and use the package importlib_metadata for older Python versions.

mlasch commented 3 weeks ago

Actually https://github.com/open-telemetry/opentelemetry-python/pull/4177 could be a solution. I agree it makes the code more complex, but modern Python versions would not have to use importlib_metadata.

xrmx commented 3 weeks ago

@mlasch we decided to use importlib-metadata on all python versions to simplify things on our side. From the next release importlib-metadata is used instead of pkg-resources.

mlasch commented 3 weeks ago

@xrmx That's unfortunate. However, nice to see that https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py is the single place which imports importlib_metadata that makes patching much easier.