nanobind_example icon indicating copy to clipboard operation
nanobind_example copied to clipboard

add `__all__` to package file

Open burgholzer opened this issue 1 year ago • 8 comments

Populating the __all__ attribute stops IDEs from complaining with messages like

Cannot find reference 'add' in 'imported module nanobind_example | __init__.py'

burgholzer avatar Jun 16 '23 14:06 burgholzer

Which IDE is that specifically? Do you know why it complains about this? (AFAIK the module is perfectly well-formed even without the __all__ attribute)

wjakob avatar Jun 16 '23 15:06 wjakob

Sorry, should have been more specific. It's CLion from jetbrains.

Before the change: image After the change: image

Just tried in PyCharm as well. There, everything works properly without the __all__ attribute. So it might really be a quirk in CLion. But maybe it is still worth adding.

burgholzer avatar Jun 16 '23 15:06 burgholzer

Just thought about this a bit more. A further possible argument for adding this is static type checking. Without the __all__ attribute, the add function is considered an implicit reexport. Anyone using a strict mypy configuration will receive a warning from this.

To reproduce:

git clone https://github.com/wjakob/nanobind_example
cd nanobind_example
python3 -m venv .venv
. .venv/bin/activate
pip install mypy
python -m mypy src/nanobind_example tests --strict

will yield

src/nanobind_example/__init__.py:1: error: Cannot find implementation or library stub for module named "nanobind_example.nanobind_example_ext"  [import]
src/nanobind_example/__init__.py:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
tests/test_basic.py:3: error: Function is missing a return type annotation  [no-untyped-def]
tests/test_basic.py:3: note: Use "-> None" if function does not return a value
tests/test_basic.py:4: error: Module "nanobind_example" does not explicitly export attribute "add"  [attr-defined]
Found 3 errors in 2 files (checked 2 source files)

The first error could be fixed by simply adding a stub file for the compiled library (happy to contribute that if you like). The second error is simply a missing -> None in the test file (also happy to add that if you like). The important one for this PR here is the third issue:

tests/test_basic.py:4: error: Module "nanobind_example" does not explicitly export attribute "add" [attr-defined]

This PR eliminates that warning.

burgholzer avatar Jun 20 '23 07:06 burgholzer

@wjakob any updates/news from your end regarding 👆🏼 ?

burgholzer avatar Jun 30 '23 14:06 burgholzer

This is kind of a tooling-related question, and I am not 100% sure. I would be curious if @henryiii has thoughts on it? (Any change here should likely be mirrored over to the pybind11 example repositories as well).

wjakob avatar Jun 30 '23 14:06 wjakob

I've be okay with it - I recently wrote https://learn.scientific-python.org/development/patterns/exports which basically recommends all (public) python files have an __all__ :)

henryiii avatar Jun 30 '23 15:06 henryiii

Also, the pybind11 example has had __all__ for the last seven months: https://github.com/pybind/scikit_build_example/blob/f8aa843a0267c0158743a48c675bc07d66ad8401/src/scikit_build_example/init.py

henryiii avatar Jun 30 '23 21:06 henryiii

Hey @wjakob, I just added the __doc__ and __version__ attributes here as well to bring this on par with what @henryiii pointed out above from pybind11. Could we get this merged some time?

burgholzer avatar Aug 17 '23 07:08 burgholzer

I am closing this issue. The right solution going forward will be to add stubs using the new nanobind_add_stub command. I will upgrade the nanobind_example repo to use this feature once nanobind 2.0 has been released.

wjakob avatar Mar 18 '24 21:03 wjakob