-
Notifications
You must be signed in to change notification settings - Fork 32
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
[bug] python/openinference-instrumentation/src/openinference/instrumentation/__init__.py does not conform to namespace packaging #398
Comments
Hi @ewianda I am not able to reproduce this issue. As shown in the screenshot below, it has no issue on colab. Do you have more context on how the failure occurred? (Are you using editable install?) |
I'm closing this issue for now, but please feel free to re-open if you need further assistance. Thanks! |
Sorry for the late response. This issue will not be observed when running a pip install or any other package manager. This is primarily a problem with bazel build tool that attempts to reconstruct the python path with the assumption that all namespace packages conform to the standard packaging |
@ewianda Thanks for the reply. Can you give us more detailed information on when exactly you hit this issue? E.g., are you using Bazel to build an OCI image with both |
No, it is just a regular py_binary. I can create a GitHub repo to quickly demonstrate the issue if that helps |
we are using the this patch to get around the issue --- a/openinference/instrumentation/__init__.py
+++ b/openinference/instrumentation/__init__.py
@@ -1,3 +1,4 @@
+__path__ = __import__('pkgutil').extend_path(__path__, __name__)
import contextlib
from typing import Iterator |
Based on this reading, it is a problem specific to Bazel but is otherwise working as intended for Python. So I am not sure how we should be supporting this going forward. |
Wil not be possible to apply the above patch as a solution ? |
yeah the solution in the post is already implemented in bazel, the relevant logic is def add_pkgutil_style_namespace_pkg_init(dir_path: Path) -> None:
"""Adds 'pkgutil-style namespace packages' init file to the given directory
See: https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
Args:
dir_path: The directory to create an __init__.py for.
Raises:
ValueError: If the directory already contains an __init__.py file
"""
ns_pkg_init_filepath = os.path.join(dir_path, "__init__.py")
if os.path.isfile(ns_pkg_init_filepath):
raise ValueError("%s already contains an __init__.py file." % dir_path)
with open(ns_pkg_init_filepath, "w") as ns_pkg_init_f:
# See https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
ns_pkg_init_f.write(
textwrap.dedent(
"""\
# __path__ manipulation added by bazelbuild/rules_python to support namespace pkgs.
__path__ = __import__('pkgutil').extend_path(__path__, __name__)
"""
)
)
As you can see the logic expects that namespace packages should have no |
Our packages sharing the same namespace does not automatically make them "namespace packages". They're intended to be installed and used as "regular packages", and are configured as such (see PEP 420 shown below). They only become "namespace packages" as a result of Bazel's operating requirements, which were not factored into our original design. This is not to say that we wouldn't consider modifications in light of this new limitation, but I think we'll need some time to think this one through. |
Removing the triage flag for now. Let us know if we can help in any other way! |
Describe the bug
Based on
https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages
OR
native namespace packages.
Without this we get
when openinference-instrumentation and openinference.instrumentation-llama_index are used together
Additional context
Similar issue aws/jsii#3881
The text was updated successfully, but these errors were encountered: