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

Include generated stubs in python package #6917

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

timohl
Copy link
Contributor

@timohl timohl commented Aug 14, 2024

Type

Motivation and Context

Currently, the Open3D python package does not come with typing stubs.
Stubs can be generated manually using pybind11-stubgen (with currently wip fixes in #6896).
This pull request tries to integrate the generated typing stubs into the python package.
In this way, Open3D can ship with typing support out of the box.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

This adds the following steps to the build process:

  • Add pybind11-stubgen to requirements_build.txt
  • Generate stubs using pybind11-stubgen
  • Write stubs into python package
  • Add py.typed file to mark typing support (see PEP 561

TODO:

  • In vscode imports do not work correctly when integrating stubs. Usage of, e.g., open3d.camera instead of open3d.cpu.pybind.camera in stubs are not resolved correctly. Further investigation requiered...
  • Maybe add build option IGNORE_STUBGEN_ERRORS to toggle whether any errors in pybind11-stubgen call should stop the build. This might be usefull to add a CI check so typing stubs stay clean of errors.

Copy link

update-docs bot commented Aug 14, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@timohl timohl mentioned this pull request Aug 14, 2024
9 tasks
@timohl timohl marked this pull request as ready for review August 15, 2024 09:56
@timohl
Copy link
Contributor Author

timohl commented Aug 15, 2024

This is ready for review now.

In 85dd074 I moved the namespace fixing (open3d.[cpu|cuda].pybind.* -> open3d.*) to sphinx autodoc callbacks, since the imports in the generated stubs were not working correctly.

I added a cmake flag IGNORE_STUBGEN_ERRORS which might be useful when a CI check for stub generation is added.

@timohl
Copy link
Contributor Author

timohl commented Aug 15, 2024

The open3d python package installs the following now:

open3d:
__init__.py
__init__.pyi
_build_config.py
app.py
cpu
examples
libc++.so.1
libc++abi.so.1
ml
py.typed
resources
tools
visualization
web_visualizer.py

Note the following new files:

  • *.pyi files: Contain the typing stub information (more can be found in subdirectories)
  • py.typed file: Marks that this package supports typing (see PEP 561)

@timohl
Copy link
Contributor Author

timohl commented Aug 15, 2024

I fixed some build errors from CI checks.
For that, I changed the workflows to always use requirements_build.txt instead of locally defining the build dependencies.
I applied this to all locally defined python dependencies, so the requirements*.txt become the single source of truth for dependencies.

There is still an issue when open3d-ml is included.
pybind11-stubgen requires runtime dependencies to be available, so the open3d-ml requirements have to be installed in docker/Dockerfile.wheel.
See Ubuntu Wheel / Build wheel (3.11, false) with error from pybind11-stubgen:
ModuleNotFoundError: No module named 'sklearn'

I will fix that later today.

@timohl
Copy link
Contributor Author

timohl commented Aug 16, 2024

I fixed some CI stuff, but there are still two issues left in the Windows checks:

  • With BUILD_CUDA_MODULE enabled pybind11-stubgen throws: [Open3D Error] (void __cdecl open3d::core::__OPEN3D_CUDA_CHECK(enum cudaError,const char *,const int)) D:\a\Open3D\Open3D\cpp\open3d\core\CUDAUtils.cpp:310: D:\a\Open3D\Open3D\cpp\open3d\core\CUDAUtils.cpp:25 CUDA runtime error: CUDA driver version is insufficient for CUDA runtime version
  • With BUILD_SHARED_LIBS pybind11-stubgen throws: ImportError: DLL load failed while importing pybind: The specified module could not be found.

Not sure right now how to fix those.

@timohl
Copy link
Contributor Author

timohl commented Aug 16, 2024

I disabled stubgen for windows shared libs and windows cuda workflows by introducing the WITH_STUBGEN cmake flag.

Currently, I cannot really check if stubgen works for CUDA since I do not have access to a GPU supporting CUDA at home right now.
The __init__.py seems to only import open3d.cuda.pybind if there is any CUDA device available:

if _pybind_cuda.open3d_core_cuda_device_count() > 0:
from open3d.cuda.pybind import (core, camera, data, geometry, io,
pipelines, utility, t)
from open3d.cuda import pybind
__DEVICE_API__ = "cuda"

So pybind11-stubgen does not work if no device is available.

At this point, I would love to get some feedback.
Most importantly, those commits which add the stub generation/inclusion:
564af88
9a9cc95
85dd074
7966fc0
But also about the integration into the build process and CI.

@ManuCorrea
Copy link

Looking forward to this to get merged! This will improve a lot the safety and use of Open3D in large applications.

@ssheorey ssheorey self-requested a review August 19, 2024 21:08
@ssheorey ssheorey added this to the v0.19 milestone Aug 19, 2024
@ssheorey
Copy link
Member

Hi @timohl thanks for this very useful contribution!

The goal of this PR is to improve type checking support when using Open3D. One way to test this is by running mypy over the Python examples and looking at the output with and without this PR (i.e. with open3d v0.18 installed vs the Python wheel from this PR installed.) When I tried that, I ran into this error with the wheel from this PR:

$ mypy examples/python/geometry/triangle_mesh_with_numpy.py 
/Users/ssheorey/.pyenv/versions/3.11.4/envs/o3d-311/lib/python3.11/site-packages/open3d/__init__.pyi:51: error: invalid syntax  [syntax]
Found 1 error in 1 file (errors prevented further checking)

Here's what it's complaining about (last line of __init__.pyi looks incomplete):

open3d = 

Can you take a look?

@timohl
Copy link
Contributor Author

timohl commented Aug 31, 2024

Honestly, I am not sure why there is this open3d = at the end.
I am no expert in pybind11-stubgen internals and the __init__.py from which the __init__.pyi is generated, is quite complex.
When I add del open3d in 288d51d it seems to be resolved and everything else is working.

Now mypy has a new errror:

mypy examples/python/geometry/triangle_mesh_with_numpy.py 
/home/timohl/projects/Open3D/.venv/lib/python3.10/site-packages/open3d/cpu/pybind/core/__init__.pyi:732: error: non-default argument follows default argument  [syntax]
Found 1 error in 1 file (errors prevented further checking)

refering to:

@staticmethod
@typing.overload
def arange(start: int | None = None, stop: int, step: int | None = None, dtype: Dtype | None = None, device: Device | None = None) -> Tensor:
        """
        Create a 1D tensor with evenly spaced values in the given interval.
        """

I will try to fix all mypy errors, but soonest I can find time for that might be next weekend.

@timohl
Copy link
Contributor Author

timohl commented Aug 31, 2024

I fixed some mypy errors:

Changed Tensor.arange signature to be like in numpy (230a9a6).

This is how numpy defines arange:

@overload
def arange(  # type: ignore[misc]
    stop: _IntLike_co,
    /, *,
    dtype: None = ...,
    device: None | L["cpu"] = ...,
    like: None | _SupportsArrayFunc = ...,
) -> _1DArray[int, signedinteger[Any]]: ...
@overload
def arange(  # type: ignore[misc]
    start: _IntLike_co,
    stop: _IntLike_co,
    step: _IntLike_co = ...,
    dtype: None = ...,
    *,
    device: None | L["cpu"] = ...,
    like: None | _SupportsArrayFunc = ...,
) -> _1DArray[int, signedinteger[Any]]: ...

(see https://github.com/numpy/numpy/blob/b3ddf2fd33232b8939f48c7c68a61c10257cd0c5/numpy/_core/multiarray.pyi#L769-L786)

Note how start is not optional in the second signature.
Here, optional is not needed, because the first overload handles that case.

Also, it adds /, *, in the first signature and *, in the second.
/ forces all args before to be positional-only and * forces all arguments after to be keyword-only.
In pybind this can be achieved using py::pos_only() and py::kw_only().

Changed lambda to lambda_penalty in orient_normals_consistent_tangent_plane (6df9d85).

This is a straight forward argument rename, since lambda is a reserved keyword in python.
I also checked for other parameters named lambda and this seems to be the only one.

Changed lookat, up, front, and zoom to optional args (54eab24).

mypy comlained that there are arg without default value after args with default value.
I changed those to be optional (passing none/nullptr).
This is in line with the cpp implementation.
Note that I used utility::optional instead of a simple pointer, which generates correct types in stubgen (<type> | None = None instead of <type> = None which mypy also complains about).

@timohl
Copy link
Contributor Author

timohl commented Aug 31, 2024

mypy now does not complain about anything in the stub files (I applied #6896 as well though), but has two errors in the example code:

mypy examples/python/geometry/triangle_mesh_with_numpy.py 
examples/python/geometry/triangle_mesh_with_numpy.py:14: error: Argument 1 to "read_triangle_mesh" has incompatible type "str"; expected "PathLike[Any]"  [arg-type]
examples/python/geometry/triangle_mesh_with_numpy.py:15: error: Argument 1 to "paint_uniform_color" of "TriangleMesh" has incompatible type "list[float]"; expected "ndarray[Any, Any]"  [arg-type]
Found 2 errors in 1 file (checked 1 source file)

The first error comes from pybind11 generating os.PathLike from fs::path, however it should be os.PathLike | str since C++ supports implicit conversion from std::string to fs::path.
So I think this is a bug in pybind11.

We could change cpp/pybind/pybind_filesystem.h (not sure how easy it is to play with template/macro magic there).
Or I think the easier route would be to change cpp/pybind/docstring.cpp to replace any os.PathLike in argument types to os.PathLike | str.

The second error involves this signature in the stubs:

def paint_uniform_color(self, arg0: numpy.ndarray[numpy.float64[3, 1]]) -> MeshBase:
...

pybind11 uses numpy.ndarray[numpy.float64[3, 1]] for Eigen::Vector3d.
Apparently there is also some implicit conversion going on, but I cannot easily find it in the pybind11/Eigen code.
A solution using numpy.typing.ArrayLike would probably be nice, however shape info would be lost.
Not sure how to proceed here.

@timohl
Copy link
Contributor Author

timohl commented Aug 31, 2024

288d51d also seems to cause an error in CI:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/runner/work/Open3D/Open3D/open3d_test.venv/lib/python3.11/site-packages/open3d/__init__.py", line 61, in <module>
    load_cdll(str(next((Path(__file__).parent / "cpu").glob("pybind*"))))
  File "/home/runner/work/Open3D/Open3D/open3d_test.venv/lib/python3.11/site-packages/open3d/__init__.py", line 36, in load_cdll
    raise FileNotFoundError(f"Shared library file not found: {path}.")
FileNotFoundError: Shared library file not found: /home/runner/work/Open3D/Open3D/open3d_test.venv/lib/python3.11/site-packages/open3d/cpu/pybind.

https://github.com/isl-org/Open3D/actions/runs/10647126362/job/29514998533?pr=6917

@timohl
Copy link
Contributor Author

timohl commented Sep 2, 2024

70b3f81 fixes fs::path python typing by changing it from os.PathLike to Union[os.PathLike, str] which is translated to os.PathLike | str by pybind11-stubgen and sphinx-docs. It was actually easier to change than I previously thought.

The major amount of remaining mypy errors are implicit conversions from list to np.ndarray.
Having dimensions in docs is very nice (e.g., numpy.ndarray[numpy.float64[3, 1]]) but since shapes are not checked by mypy.
So, maybe numpy.typing.ArrayLike could be used just in stubs and not docs.

Another common error is mypy not resolving imports correctly:

mypy examples/python/io/realsense_io.py 
examples/python/io/realsense_io.py:28: error: Cannot find implementation or library stub for module named "open3d.t.io"  [import-not-found]
examples/python/io/realsense_io.py:28: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
examples/python/io/realsense_io.py:28: error: Cannot find implementation or library stub for module named "open3d.t"  [import-not-found]
examples/python/io/realsense_io.py:29: error: Cannot find implementation or library stub for module named "open3d.t.geometry"  [import-not-found]
examples/python/io/realsense_io.py:30: error: Cannot find implementation or library stub for module named "open3d.visualization.gui"  [import-not-found]
examples/python/io/realsense_io.py:31: error: Cannot find implementation or library stub for module named "open3d.visualization.rendering"  [import-not-found]
examples/python/io/realsense_io.py:69: error: "Callable[[Any, Any, Any, Any, Any], Any]" has no attribute "fid"  [attr-defined]
Found 6 errors in 1 file (checked 1 source file)

mypy is able to import open3d as o3d, but any import like from open3d.t.geometry import PointCloud is not getting resolved.
Not sure what the source of those import errors are, but maybe I can find some info at https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

@timohl
Copy link
Contributor Author

timohl commented Sep 3, 2024

Some notes (mostly for myself, but happy for any help/advice) regarding implicit list -> np.ndarray conversions causing mypy errors like this:

mypy examples/python/geometry/triangle_mesh_with_numpy.py 
examples/python/geometry/triangle_mesh_with_numpy.py:15: error: Argument 1 to "paint_uniform_color" of "TriangleMesh" has incompatible type "list[float]"; expected "ndarray[Any, Any]"  [arg-type]
Found 1 errors in 1 file (checked 1 source file)

list is not always convertible to np.ndarray, but only when using Eigen types

Example:

import numpy as np
import open3d as o3d

#[1] Uses Eigen::Vector3d
bb = o3d.geometry.AxisAlignedBoundingBox([0, 0, 0], np.array([1, 1, 1]))
print(bb)
#[2] Uses explicit overloads for py::list
t1 = o3d.core.Tensor([0, 1.5, 0], dtype=o3d.core.float32)
print(t1)
#[3] Same as [2]. shape is a ambigous here, since this will not set the shape but initilize the Tensor with those values
t2 = o3d.core.Tensor(shape=[0, 1, 0], dtype=o3d.core.float32)
print(t2)
#[4] Uses py::array -> Error when passing list
t3 = o3d.core.Tensor.from_numpy([0, 5, 0])
print(t3)

Output:

AxisAlignedBoundingBox: min: (0, 0, 0), max: (1, 1, 1)
[0 1.5 0]
Tensor[shape={3}, stride={1}, Float32, CPU:0, 0x5623bc51ff80]
[0 1 0]
Tensor[shape={3}, stride={1}, Float32, CPU:0, 0x5623bc562150]
Traceback (most recent call last):
  File "/home/timohl/projects/pybind11-stubgen/o3d_test.py", line 10, in <module>
    t3 = o3d.core.Tensor.from_numpy([0, 5, 0])
TypeError: from_numpy(): incompatible function arguments. The following argument types are supported:
    1. (arg0: numpy.ndarray) -> open3d.cpu.pybind.core.Tensor

Invoked with: [0, 5, 0]

How could annotation using ArrayLike for Eigen types look like?

Using imports this is a more compact view and could easily be displayed in sphinx docs like this:

import numpy as np
import numpy.typing as npt
from typing import Annotated

def f1(arg0: np.ndarray[np.float64[3, 1]] | npt.ArrayLike) -> None: ...
def f2(arg0: Annotated[npt.ArrayLike, np.float64[3, 1]]) -> None: ...

Right now I would tend to f2.

How to insert ArrayLike annotations?

I am not sure, however, how to get there, so that only Eigen Arguments and not Return values are using ArrayLike.

  • Wrapping all occurences of Eigen types in the o3d binding code seems like a lot of work.
  • Customizing pybind11/eigen/matrix.h could be an option, but it is pretty complicated template magic code and diverging from pybind11 could be bad for updates.
  • Another option would be to allow lists for every np.ndarray argument. Then changing to ArrayLike would be easily possible as a post-processing step after pybind11 compilation. But then all functions taking arrays must be made compatible with lists.

@timohl
Copy link
Contributor Author

timohl commented Sep 6, 2024

I have opened a pull request at pybind/pybind11#5358 to add a feature for adding different type hints for args and return types.
That PR would be the cleanest solution in my opinion to resolve a lot of mypy issues.
Hope it gets merged soon.

@timohl
Copy link
Contributor Author

timohl commented Oct 24, 2024

Just a quick update:
I do not have so much time this month but I hopefully can start working on this again next month.

Some notes on what to do next:
The base idea of pybind/pybind11#5358 was good but it was hard to get all edge cases within the template magic used in pybind11.
I think a simpler scope would be easier to implement and test:
Just support specialized arg+return type hints for types that are not template parameters.
Something like std::vectorstd::filesystem::path or std::vector<std::vectorstd::filesystem::path> is hard to get right in a generalized manner.
So, using specialized arg+return type hints for just std::filesystem::path and falling back to the default type hints for (nested) templates like std::vectorstd::filesystem::path would be a safe way to add this without breaking a lot of other stuff.

With specialized arg+return type hints most mypy errors should be easy to fix and generates more precise documentation on what types are actually valid for function parameters.

@timohl
Copy link
Contributor Author

timohl commented Dec 2, 2024

My pybind11 pull request pybind/pybind11#5450 to fix the type hint for fs::path is making good progress and lays the foundation to improve typing of Eigen/Numpy classes.
That part might take more time, though.

Meanwhile, I could create some smaller pull requests to already merge some fixes, that also improve the documentation.
The following fixes could already improve the documentation:

Maybe it would be easier to merge multiple smaller PRs than just one big PR.
There is also #6896 including some fixes.
@ssheorey what do you think about that?

@timohl
Copy link
Contributor Author

timohl commented Dec 16, 2024

My pull request pybind/pybind11#5450 fixing fs::path type hints was successfully merged into pybind11. :)
Next, I will try to work correct type hints for np.array/Eigen into pybind11.

I also created some smaller pull requests for commits that change the Python API.
That way, the scope of this PR would only be to integrate stubgen into the packaging and CI.
For now, until the smaller PRs are merged and the correct np.array/Eigen typing is in pybind11, I will convert this PR to a draft.

@timohl timohl marked this pull request as draft December 16, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
3 participants