open-telemetry / opentelemetry-python

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

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

Closed jenshnielsen closed 1 year ago

jenshnielsen commented 1 year ago

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 https://github.com/open-telemetry/opentelemetry.io/pull/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

srikanthccv commented 1 year ago

Briefly discussed this earlier a couple of times(ex https://github.com/open-telemetry/opentelemetry-python/pull/3038#discussion_r1033959341). 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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

(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 commented 1 year ago

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