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

Use pybind11-stubgen for generating stubs #1821

Merged
merged 2 commits into from
Sep 8, 2024
Merged

Use pybind11-stubgen for generating stubs #1821

merged 2 commits into from
Sep 8, 2024

Conversation

ProfFan
Copy link
Collaborator

@ProfFan ProfFan commented Sep 3, 2024

Use pybind11-stubgen to generate the stubs, which is a stub generator specifically built for pybind11 types.

Also changed the way stub is generated. Should hopefully fix the issues with mypy's generator.

Tested locally on both VSCode and JupyterLab (macOS M1):

image

@tianyilim
Copy link

Responding here to track #1814

I manually had to create the folders python-stubs and python-unstable-stubs in build/python/CMakeFiles, I guess it's not too difficult to fix.

However, by default in VSCode the stub info for me disappears by default (no error, but it's reported as Unknown)

Perhaps this is more of a Pylance thing now, where it doesn't know where to find the generated stubs?

@ProfFan
Copy link
Collaborator Author

ProfFan commented Sep 3, 2024

@tianyilim It works for me in Pylance under macOS M1. Are you sure that you are using the correct Python environment?

image

Please do the following:

Make sure you uninstall ALL of your previous GTSAM python bindings by doing pip uninstall gtsam

Reinstall the python bindings. In VSCode, check bottom right if you are in the correct Python environment:

image

Choose the correct one, then Cmd+Shift+P and say Python: Restart Language Server.

If this does not work, check:

  1. Remove your entire build folder (your 1st problem is because of this)
  2. Build GTSAM with Python bindings
  3. Install with make python-install or ninja python-install

@tianyilim
Copy link

Okay, I tried with a fresh conda environment and it seems to have worked:

conda create --name gtsamStubs python=3.10 -y
conda activate gtsamStubs
cd gtsam/
git fetch -ap
git checkout fan/gen_stubs
mkdir build
python -m pip install -r python/dev_requirements.txt
python -m pip install pyparsing
cd build/
cmake .. -DGTSAM_BUILD_PYTHON=1
make python-install -j$(nproc)

I now get similar output to when gtsam-stubs was installed externally.

Previously, I was using the flags -DGTSAM_PYTHON_VERSION=3.10 and -DCMAKE_INSTALL_PREFIX="./install" but I removed them this time.

Thanks for the help! 😄

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I’ll try on my linux env and Mac to be sure - in conda env, then merge if works

@varunagrawal
Copy link
Collaborator

I'm a bit lost. If the issue is gtsam.noiseModel not showing up properly, why are we looking at the way pybind11 type stubs are generated? What is this PR trying to fix?

@ProfFan
Copy link
Collaborator Author

ProfFan commented Sep 7, 2024

I'm a bit lost. If the issue is gtsam.noiseModel not showing up properly, why are we looking at the way pybind11 type stubs are generated? What is this PR trying to fix?

Just in my experience pybind11-stubgen generates better type annotations (a lot of C++ types are set to just Any with stubgen). This PR fixes the noiseModel issue, refactors the stub generation into their own targets, and replaced mypy's stubgen to pybind11-stubgen.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for one thing, looks good to me.

@@ -25,6 +25,7 @@
#include <boost/serialization/nvp.hpp>
#endif
#include <memory>
#include <algorithm>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah to fix latest clang 15 on Linux, I tested on my arch. (we are using std::find)

@ProfFan ProfFan merged commit e3dd4e1 into develop Sep 8, 2024
34 checks passed
@ProfFan ProfFan deleted the fan/gen_stubs branch September 8, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants