Initial Bazel configuration for Pybind11 and S2Point bindings #524
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Looks like a good start. I'll have a closer look tomorrow. How much are you planning on wrapping? |
Great!
For my purposes I mainly need S2Cell and its dependencies. But I'm happy to do more to get this to whatever you would consider a viable MVP. Open to suggestions there? |
deustis
force-pushed
the
deustis/pybind11_bazel_setup
branch
from
6535f67 to
7b92d38
Compare
src/python/README.md
Outdated
|
|
||
| ``` | ||
| python/ | ||
| ├── bindings/ # Dir for C++ pybind11 bindings |
There was a problem hiding this comment.
Why is there both bindings/ and s2geometry_pybind/?
It seems like one should do, and everything could probably go directly in python/, but I'm not sure.
There was a problem hiding this comment.
I believe we do do need the s2geometry_pybind to match the name of the python package. Open to suggestions on naming there.
The bindings and tests directories are not necessary. We can certainly flatten one or both of those into the python parent if you prefer. Happy to go with whatever layout you prefer here.
There was a problem hiding this comment.
Well, definitely flatten the extra dirs.
If it needs to be s2geometry_pybind, then so be it. If not, we can further flatten later.
What will be the bazel targets and what will people import in Python?
There was a problem hiding this comment.
I've flattened the directories!
What will be the bazel targets and what will people import in Python?
I'm thinking the bazel targets will be mainly for development but I suppose for a production use-case if a customer is already using bazel it should be possible to depend on the py_library target directly (s2geometry_pybind).
That said, I think most users would want a pip package so I mentioned we'll need to figure out how to build a wheel and upload to pypi.
Currently the python import is s2geometry_pybind as seen in the unit test:
https://github.com/google/s2geometry/pull/524/changes#diff-f1182576e6dc7943a0970fce1cdeee021cea55a7b88b9c50756d6cb9fa8d4126R4
I was thinking that once we have a full replacement for the SWIG version we will rename to s2geometry (and delete the former)
src/python/README.md
Outdated
| │ └── ... # Bindings for additional classes | ||
| ├── s2geometry_pybind/ # Dir for Python package | ||
| │ └── __init__.py # Package initialization | ||
| ├── tests/ # Dir for Python unit tests |
There was a problem hiding this comment.
tests/ subdir also seems inconsistent with the rest of the project. Why not keep tests with the code?
There was a problem hiding this comment.
Sure, we can drop. Just let me know what layout you prefer in the comment above.
src/python/bindings/BUILD.bazel
Outdated
| package(default_visibility = ["//python:__subpackages__"]) | ||
|
|
||
| pybind_extension( | ||
| name = "s2geometry_bindings", |
There was a problem hiding this comment.
Add the underscore prefix to indicate it is a private module
src/python/bindings/BUILD.bazel
Outdated
|
|
||
| pybind_extension( | ||
| name = "s2geometry_bindings", | ||
| srcs = ["module.cc"], |
There was a problem hiding this comment.
Does it make any sense to have fine-grained targets? In google3, we have fine-grained targets for the S2 library, which really helps compilation and development time. We don't use them for PyClif mostly because the C++ targets were sharded after the pyclif ones were added.
Do we just accept that it's monolithic in Python?
There was a problem hiding this comment.
We could consider adding some submodules. How about if we put the "core" classes directly under the top-level package (e.g. s2geometry.S2Cell), but add submodules for some of the utility classes (e.g. s2geometry.regions.S2CellCovering). Open to suggestions on how to break this up.
Once we align on the python module structure, I think we would then define one pybind_extension rule per module, plus separate pybind_library rules for each class.
|
@jmr, I made most of the changes you suggested. Let me know what you prefer for the directory layout and how to organize the python modules and I'll make those changes as well. |
|
@jmr, friendly ping? I'm looking for feedback on the directory layout and python modules (see above). |
| bazel_dep(name = "googletest", version = "1.17.0.bcr.2", dev_dependency = True) | ||
|
|
||
| # Python configuration | ||
| bazel_dep(name = "platforms", version = "1.0.0") |
There was a problem hiding this comment.
I believe this is needed for pybind.
Without this I get errors like:
ERROR: no such package '@@[unknown repo 'platforms' requested from @@]//os': The repository '@@[unknown repo 'platforms' requested from @@]' could not be resolved: No repository visible as '@platforms' from main repository
ERROR: /home/deustis/src/s2geometry/src/python/bindings/BUILD.bazel:6:17: errors encountered resolving select() keys for //python/bindings:_s2geometry_bindings.so
I see it declared upstream here, but I'm not entirely sure it also needs to be added as a dependency in this module:
https://github.com/pybind/pybind11_bazel/blob/master/MODULE.bazel#L9
I've added a comment about this.
|
@jmr, I've flattened the directories and responded to your other comments. This should be ready for another round! (Still waiting on the lawyers to sign that CLA...) |
2 participants
Initial Bazel configuration for Pybind11 and S2Point bindings.
#522
The S2 Geometry library is transitioning from SWIG-based bindings to pybind11-based bindings.
During this migration:
s2geometry): The current production bindings, built with CMake. Useimport s2geometryto access these.s2geometry_pybind): The new bindings under development, built with Bazel. Useimport s2geometry_pybindto access these.Once the pybind11 bindings are feature-complete and stable, the SWIG bindings will be deprecated and the pybind11 package will be renamed to
s2geometryto become the primary Python API.This PR adds Bazel configuration for building and testing the bindings on the local toolchain. For distribution we'll need to generate a wheel for compatibility with different OS and python versions.