Pyright flags load as reportPrivateImportUsage

[edran]

Bug report

Bug summary

Pyright flags any usage of load as reportPrivateImportUsage. I believe it's because load and other such functions are not exported as public in yt/__init__.py.

Code for reproduction

Example pyproject.toml with poetry-controlled deps:

[tool.poetry]
name = "foo"
version = "0.1.0"
description = ""
authors = ["foo"]
packages = [{include = "foo"}]

[tool.poetry.dependencies]
python = ">=3.11,<3.12"
yt = "^4.2.2"

[tool.poetry.group.dev.dependencies]
pyright = "^1.1.329"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"

[tool.pyright]
venvPath = "."
venv = ".venv"

Error can be reproduced with the following snippet:

import yt
yt.load("")

poetry run pip freeze output:

appnope==0.1.3
asttokens==2.4.0
backcall==0.2.0
cmyt==1.4.0
comm==0.1.4
contourpy==1.1.1
cycler==0.12.0
decorator==5.1.1
ewah-bool-utils==1.1.0
executing==2.0.0
fonttools==4.43.0
ipython==8.16.1
ipywidgets==8.1.1
jedi==0.19.1
jupyterlab-widgets==3.0.9
kiwisolver==1.4.5
matplotlib==3.8.0
matplotlib-inline==0.1.6
more-itertools==10.1.0
mpmath==1.3.0
nodeenv==1.8.0
numpy==1.26.0
packaging==23.2
parso==0.8.3
pexpect==4.8.0
pickleshare==0.7.5
Pillow==10.0.1
prompt-toolkit==3.0.39
ptyprocess==0.7.0
pure-eval==0.2.2
Pygments==2.16.1
pyparsing==3.1.1
pyright==1.1.329
python-dateutil==2.8.2
setuptools-scm==8.0.4
six==1.16.0
stack-data==0.6.3
sympy==1.12
tomli_w==1.0.0
tqdm==4.66.1
traitlets==5.11.2
typing_extensions==4.8.0
unyt==2.9.5
wcwidth==0.2.8
widgetsnbextension==4.0.9
yt==4.2.2

Actual outcome

$ cat foo.py
import yt
yt.load("")
$ poetry run pyright
/path/to/foo.py
  /path/to/foo.py:3:4 - error: "load" is not exported from module "yt" (reportPrivateImportUsage)
1 error, 0 warnings, 0 informations

Expected outcome

No error from pyright.

Version Information

• Operating System: MacOS 12.6.2
• Python Version: 3.11
• yt version: 4.2.2
• Other Libraries (if applicable): N/A

[welcome]

Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue.

[neutrinoceros]

I believe it's because load and other such functions are not exported as public in yt/init.py.

It is exported in yt.__init__.py (otherwise calling yt.load would raise AttributeError):

yt/yt/__init__.py

Lines 78 to 79 in 416a6a4

	from yt.loaders import (
	load,

Maybe the flagging is caused by the future_positional_only decorator (which is private API) ?

[chrishavlin]

I think it's that pyright still considers those imports private, I think we'd need to change all the from X import A imports to from X import A as A or add an __all__ attribute that lists all the modules we want to be considered public.

https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface doesn't explicitly state that but if you search for pyright reportPrivateImport errors you can find various discussions around this.

EDIT: this is one of the clearer discussions of it, microsoft/pyright#2639

[matthewturk]

I'm not sure I would like to see this change made at this time. It seems rather large, and also outside of our typical CI/lint/style stuff.

[edran]

The alternative would be to declare __all__ in __init__.py with all the objects you wish to make public. I'm happy to make a PR if that's acceptable.

EDIT: I saw that @chrishavlin proposed this already. @matthewturk this might make for a fairly inconsequential change?

[neutrinoceros]

from https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface

Imported symbols are considered private by default.

Today I learned. This is the opposite of what I thought type-checkers did. It actually makes more sense, as it's more conservative than the alternative (it's supposed to be possible to introduce type-checking gradually). If we were to explicitly declare our public API in the whole code base, I would prefer using __all__ over redundant re-exports (from a import A as A), however I agree with Matt that this would be quite an undertaking. That said it probably wouldn't hurt to do it in a couple modules at a time if it helps with type-checking downstream code.
Are there any known errors of this type at the moment ? If not, we could probably get away with just adding an explicit __all__ to loaders.py

[neutrinoceros]

The alternative would be to declare all in init.py with all the objects you wish to make public.

__init__.py is where we put all our main public API, so anything that lives there and isn't deleted is considered public API

I wouldn't mind a PR adding __all__ there if we know it's sufficient to make pypright happy !

[chrishavlin]

If adding __all__ to loaders.py is sufficient and doesn't just end up exposing more pyright failures then I think it's worth it. We also don't test with pyright, so we're not going to be able to guarantee that it won't fail in the future (and at present I don't think we should expand our type checking tests beyond the exisitng mypy check).

[matthewturk]

OK, I have come around. It's not that big a deal to change, and probably having __all__ is not that bad, either. Thanks for persisting, @edran !