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

Dynamically generated __all__ in opentelemetry sdk does not work with static type checkers #3141

Closed
jenshnielsen opened this issue Jan 25, 2023 · 5 comments · Fixed by #3143
Labels
bug Something isn't working

Comments

@jenshnielsen
Copy link
Contributor

Describe your environment

mypy with no-implicit-reexport enabled
pyright with default settings

Any OS or python version

Steps to reproduce

type check a line like

from opentelemetry.sdk.metrics import MeterProvider,

or open it in vscode with pylance and pyright enabled

What is the expected behavior?

Code to typecheck

What is the actual behavior?
Error such as (from pylance/pyright)

"MeterProvider" is not exported from module "opentelemetry.sdk.metrics"
  Import from "opentelemetry.sdk.metrics._internal" insteadPylance[reportPrivateImportUsage](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportPrivateImportUsage)

Additional context

This is caused by __all__ being dynamically generated for example here https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/metrics/__init__.py

The docs suggests to disable implicit_reexport as implemented here open-telemetry/opentelemetry.io#1611

This is problematic for a number of reasons.

It doesn't actually fix the issue with pyright/pylance

The implicit reexport feature is very useful and makes it much easier to spot that you are unintentionally importing symbols from the wrong location. Doing it like recommended disables this feature for all packages.

Would it be possible to replace those dynamically generated __all__s with static ones

@jenshnielsen jenshnielsen added the bug Something isn't working label Jan 25, 2023
@srikanthccv
Copy link
Member

Briefly discussed this earlier a couple of times(ex #3038 (comment)). I think we should use the statically declared explicit list. I would like to hear @ocelotl's thoughts on this since he evangelised the dynamic __all__ for the most part.

@ocelotl
Copy link
Contributor

ocelotl commented Jan 25, 2023

The dynamically generated list has the advantage of being shorter and less error prone (it may happen that we forget to add a symbol to the list if we do this statically). That being said, I am ok with using a statically declared list if this can be helpful to our users, no problem ✌️

@jenshnielsen
Copy link
Contributor Author

@srikanthccv and @ocelotl Thanks for the input, I agree that writing out __all__ explicitly is not ideal but there does not seem to be a realistic way for static tools to infer the dynamic all as it would essentially require executing the module

@jenshnielsen
Copy link
Contributor Author

(it may happen that we forget to add a symbol to the list if we do this statically)

The benefit of doing it statically is that you can now delete the pylint/flake8 comments about unused imports and these tools will now highlight that things are unused if you forget to add them to __all__

@aabmass
Copy link
Member

aabmass commented Feb 9, 2023

I am in favor of listing them out explicitly. It doesn't work well with autocomplete in VSCode either.

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

Successfully merging a pull request may close this issue.

4 participants