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

PyLance / mypy doesn't accept opentelemetry as a namespace package #2591

Closed
haf opened this issue Apr 7, 2022 · 18 comments · Fixed by #3385
Closed

PyLance / mypy doesn't accept opentelemetry as a namespace package #2591

haf opened this issue Apr 7, 2022 · 18 comments · Fixed by #3385
Labels
bug Something isn't working good first issue Good first issue help wanted

Comments

@haf
Copy link

haf commented Apr 7, 2022

The opentelemetry-api package, exporting opentelemetry doesn't successfully import into vscode.

  1. In vanilla VSCode, the import opentelemetry is white

image

  1. In VSCode with mypy / pylance enabled with strict mode:

image

gives

image

It would seem this was solved a way back microsoft/pylance-release#555 but it's not

Describe your environment Describe any aspect of your environment relevant to the problem, including your Python version, platform, version numbers of installed dependencies, information about your cloud hosting provider, etc. If you're reporting a problem with a specific version of a library in this repo, please check whether the problem has been fixed on main.

  • Python v3.9.10, with venv
  • VSCode on macOS
  • Settings: image
  • Latest of all dependencies I could find

Steps to reproduce

Install "opentelemetry" and see the import in VSCode.

What is the expected behavior?

I'd expect typing to work.

Additional context
Add any other context about the problem here.

@haf haf added the bug Something isn't working label Apr 7, 2022
@srikanthccv
Copy link
Member

I am not familiar with pylance but mypy checking should be working with opentelemetry-api.

@pygoov
Copy link

pygoov commented Apr 10, 2022

I faced the same issue with pylance.

This problem occurs because pylance does not see the file __init__.py in the /opentelemetry/ folder. If you add an file __init__.py to the root folder, then the pylance will work correctly.

I assume that this is due to the fact that two packages (api and sdk), create files to the root directory of opentelemetry. If they added at least one place from api and sdk, then the problem would be solved.

The same thing happens when installing exporters with not have the file __init__.py in the /opentelemetry/exporter/ folder!

@ocelotl
Copy link
Contributor

ocelotl commented Apr 11, 2022

We intentionally don't have a __init__.py file in the opentelemetry-api package, because it is a namespace package.

We don't intend to change that, this looks like an issue of Pylance or mypy. I suggest we close this issue, thoughts @srikanthccv, @lzchen ?

@pygoov
Copy link

pygoov commented Apr 12, 2022

Instead of not including the __init__.py file, you can include it with uses a pkgutil-style.
Most likely this will solve this problem and it is recommended in the documentation itself.
image

@owais
Copy link
Contributor

owais commented Apr 12, 2022

It would be nice if someone could take this as something to research and come back with all the pros and cons between init.py and pkgutil-style.

@haf
Copy link
Author

haf commented Apr 12, 2022

I'll start by asking Microsofties how their thinking goes. My guess: namespace packages is just not that common yet.

@haf
Copy link
Author

haf commented Apr 14, 2022

This was fixed in microsoft/pylance-release#2562 — and there's an action point for this project to be more PEP-compatible:

The opentelemetry library is placing the "py.typed" file in the top-level namespace directory rather than within the individual submodules. Pyright is currently looking for it in the submodule directories, consistent with PEP 561.

Then it should all work

@aabmass
Copy link
Member

aabmass commented Apr 29, 2022

@haf thanks for looking into this. I think that action item is reasonable, anyone want to take this issue?

@jenshnielsen
Copy link
Contributor

jenshnielsen commented Aug 5, 2022

I ran into an issue typechecking import from opentelemetry-sdk

Running

mypy .\test.py

where test.py simply contains an import from opentelemetry-sdk

from opentelemetry.sdk.trace import TracerProvider

results in

test.py:1:1: error: Cannot find implementation or library stub for module named "opentelemetry.sdk.trace"
test.py:1:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

Adding a __init__.pyi note the i to the toplevelfolder of opentelemetry here https://github.com/open-telemetry/opentelemetry-python/tree/main/opentelemetry-api/src/opentelemetry whould resolve the issue and the code type checks correctly.

Are there strong arguments against adding such a file ? I understand thata adding a __init__.py file is not a good idea but addign a __init__.pyi file seems fine? I note that opentelemetry-sdk it self ships such a file here which seems to have been created when that package it self was converted to a namespace package.

Seen with

python 3.8 and the following versions

opentelemetry-api                     1.12.0rc2
opentelemetry-instrumentation         0.32b0
opentelemetry-instrumentation-logging 0.32b0
opentelemetry-sdk                     1.12.0rc2
opentelemetry-semantic-conventions    0.32b0
mypy                                  0.971
mypy-extensions                       0.4.3

@sk-
Copy link
Contributor

sk- commented Aug 10, 2022

@jenshnielsen for mypy you need to set namespace_packages = True or the command line option --namespace-packages. See #1608 (comment)

I also had to add:

[mypy-opentelemetry.sdk.*]
implicit_reexport = True

It's a pity this info is not readily available in docs.

@cartermp
Copy link
Contributor

@sk- Agreed! I actually came here to see if there was a good recommendation on where to put this in the python docs on the website (troubleshooting, additional section in getting started, additional section in manual instrumentation, something else, etc.)

@jenshnielsen
Copy link
Contributor

jenshnielsen commented Aug 11, 2022

@sk- Thanks I figured out the same but forgot to post here. I find it a bit surprising that mypy requires you to set flags depending on details of your dependencies but that seems more like a mypy issue than a opentelemetry one.

As in it's way more obvious that I have to set the namespace flag if I am typechecking a package that is it self a namespace package than when typechecking a package that happens to depend on a namespace package

@cartermp
Copy link
Contributor

I opened this PR to see if we can add the necessary docs for mypy use: open-telemetry/opentelemetry.io#1611

@jenshnielsen
Copy link
Contributor

Coming back to this I ran this in pyright (which pylance uses internally) microsoft/pyright#4520 which was closed as working as designed.

Pyright considers opentelemetry.trace as untyped since there is no py.typed file in that module.
This is consistent with pep561 which states that

For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.

Could we please add py.typed files to the individual modules to conform with pep561

@adriangb
Copy link
Contributor

I think the issue is that opentelemetry-api installs itself as opentelemetry instead of opentelemetry.api which wreaks havoc with namespace packaging. I don’t think it can be fixed without breaking changes…

@adriangb
Copy link
Contributor

adriangb commented Jan 26, 2023

Well actually since there’s no __init__.py at that level it should work. I’ve just never seen this structure in another project. It might be worth trying to move the single py.typed file into each subdirectory, which might also require making version.py a folder.

@rajat315315
Copy link
Contributor

I have raised a PR - Link.
Can someone please help if I am in the right direction?

@ocelotl
Copy link
Contributor

ocelotl commented Aug 7, 2023

@sk @jenshnielsen @adriangb @haf @pygoov can you give it a try with #3385?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good first issue help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.