Use pybind11 for C/C++ extensions by ianthomas23 · Pull Request #23787 · matplotlib/matplotlib · GitHub

This is a proof of concept of using pybind11 to wrap and compile MPL C/C++ extensions. It covers just the _qhull and _tri extensions as (1) they are relatively small and self-contained, and (2) I understand them well. The motivation is to provide information to other devs on what it would involve and then consider if this is a suitable approach for all of our C/C++ extensions or not.

Advantages:

Extensions no longer depend on the MPL bespoke array_view class in numpy_cpp.h as pybind11 includes its own NumPy array wrapper.
Don't have to deal with parsing of input arguments via e.g. PyArg_ParseTuple as we get this for free.
There are wrappers for python classes such as list and tuple, so no longer need to deal with Python C/API reference counting and error checking.
Exceptions and error handling are much simplified. Just throw any C++ exception and pybind11 catches it and converts it to a Python equivalent exception.
There is a net reduction in code, this PR has 357 additions and 775 deletions. Much of this is the much simpler module definition as the Python/C API boilerplate is no longer required.
The extension shared libraries are smaller. On my Linux system the _qhull.so and _tri.so are 1.6 MB and 1.3 MB on main but only 1.0 MB and 0.7 MB on this branch.

Disadvantages:

It is a change, which inherently introduces risk, and to an area of code that we would probably prefer to leave alone. I appreciate that this may trump all of the advantages listed above.
Change to build system including wheels and conda builds, which is also risky.

I have tried to keep the changes as small as possible to make it clear what the minimal set of changes is. But if we were to go ahead with this approach we would probably want to make more, particularly to typedefs and numpy array accessors, to standardise and make the code clearer.

Performance of a large qhull Delaunay triangulation is essentially unchanged by this PR. Performance of a large tricontour call is ~3% faster. There is a tradeoff here between performance and code clarity, in particular whether we access multi-dimensional numpy arrays via conventional-looking indexing (e.g. array(j, i)) which is slower than accessing via low-level C pointers where we do the index maths ourselves for speed.

I know of one core dev (@anntzer) and one occasional contributor (me) who have pybind11 experience, via mplcairo and contourpy respectively.

(Edited to add another disadvantage).

This is a new build dependency, but is not a runtime dependency.

We can specify the C++ standard to follow, so here we are explicitly restricting to C++11. This would probably be a configuration param at the top of the file to make it clear what C++ standard the project is currently working to.

Good example of improved handling of creating a tuple to return.

Argument type checking is handled automatically by pybind11. Here we need to check that array shapes are consistent. These checks were originally in the wrapper functions that no longer exist.

Can install using pip install -ve . but not using pip install --no-deps -ve . which is what is used in CI. I will temporary hack tests.yml to build and run the tests.

Edit: this is no longer a problem.

I am weakly in favor of this. I think getting away from hand-rolled c-extensions is a good idea.

Another down side is that this makes the bet that pybind11 is going to last but given that we already have a transient dependency and scipy has picked up a pybind11 dependency, I think we are past avoiding that risk.

These can stay static (i.e. module-private)?

Sure.

FWIW I have a small helper in mplcairo for string formatting:
https://github.com/matplotlib/mplcairo/blob/3d9151da64587d0e1116ccd7a425c6e899efdfa0/src/_util.cpp#L93-L95
https://github.com/matplotlib/mplcairo/blob/3d9151da64587d0e1116ccd7a425c6e899efdfa0/src/_mplcairo.cpp#L336

That's nice! I hadn't considered dropping back to Python to do the string formatting.

You can use https://pybind11.readthedocs.io/en/stable/basics.html instead to specify argument names (possibly together with https://pybind11.readthedocs.io/en/stable/reference.html?highlight=kw_only#_CPPv48pos_only if you want to keep them positional-only).

OK, done this. It does improve the docstring.

I would just make this a lambda []() { return qh_version; } (returning a char* is OK).

Very happy to use a lambda! There are no lambdas yet in our C++ codebase, but if you think other devs are happy to have them...

I usually convert my arrays to use unchecked access (https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html#direct-access) (which of course means that out of bounds access can result in anything) and implicitly perform the dimensionality check at the same time as the conversion.

I've included examples of both .unchecked<>() and .data() to illustrate the possibilities and to minimise the code changes. I would default to using .data() for these 2 modules as all 2D arrays used have a fixed shape(1) of either 2 or 3, and use of data and a hardcoded multiple of 2 or 3 is much faster than using unchecked which needs to get the strides. For other extensions which accept arbitrarily-shaped arrays then probably unchecked would be preferred.

The lifetimes of these arrays is strictly controlled by the calling code so I may as well check the dims are OK just once, in the constructor.

See also #9763 which did the same for ft2font.

Apart from the question of pybind11 lasting, which I cannot really judge beyond that it seems to quite actively maintained at the moment (and historically), I think this is a good idea!

@anntzer it is realistic to get #9763 up and running again if it is decided to go with pybind11? (Quite a lot of conflicts, but no idea if they are just from the import section or more complicated than that.)

That may need a bit of work, but is certainly doable. Or it could be a small project if any newcomer (well-versed in C++ and/or freetype) wants to pick it up :)

I added it to today's agenda.

If we are going to adopt pybind11 we should write up issues to move all of our wrappers over.

I added it to today's agenda.

As @anntzer can't make today's meeting we should probably bump discussing pybind11 to next week?

I am inferring @anntzer is in favor of picking up the dependency, but bumping to next week is reasonable.

I am +0.5 on picking the dependency. I really like pybind11; OTOH in our specific case the C code is already there (on the third hand switching to pybind11 is probably better for maintainability).

on the third hand switching to pybind11 is probably better for maintainability

the examples @ianthomas23 gave made it seem that way, plus the easier maintainability may make that part of the code base more approachable

See also #9763 which did the same for ft2font.

I didn't know about this, but I am glad that it exists as I think that wrapping ft2font is a larger task than the 2 extensions in this PR.

I think we should discourage use of C++17. NumPy is using C++11
https://github.com/numpy/numpy/blob/551a4a040308eeef6318a3bb6503da7288f6d6bc/numpy/distutils/ccompiler_opt.py#L22
and SciPy uses C++14 as of last year (https://docs.scipy.org/doc/scipy/dev/toolchain.html#c-language-standards). IIRC C++11 is all we need for most of our extension code, but maybe there are a few C++14 calls around somewhere.

This is building and passing tests in CI on Linux and Windows but not macOS. Problem is the mixture of C and C++ files in qhull extension. You need to pass the -std=c++11 compiler option to the compiler for the C++ compilation to succeed, but that flag is not recognised by the C compiler which errors out rather than just printing a warning as occurs on Linux/Windows. Ref pybind/python_example#21.

I can think of a few ways of attempting to solve this, all of them slightly unpleasant.

This now passes all CI. Some of it probably isn't yet suitable for production code, but it is a good starting point to demonstrate that the approach could work.

why the pin?

I don't recall the precise reason, presumably I was experimenting when the build didn't work. But it doesn't matter now, since merging of #24102 we don't need to preinstall pybind11 for editable installs so changes like this can be reverted.

if None is not accepted anymore this warrants a changelog entry

This is the internal C++ Triangulation API (matplotllib._tri.Triangulation) that is used by the Python Triangulation class (matplotlib.tri.Triangulation). It is all private and nobody should be using it, so it doesn't need a changelog entry.

Python Triangulation class still accepts None for triangles, etc.

You can get rid of this manually typed signature. (Ditto everywhere.)

Ah yes, following the use of py::arg.

You don't even need to allocate dims on the stack; I usually just write IndexArray triangle{{ntri, 3}};. Ditto below.

I know. Remember that the idea of this PR is to demonstrate the new approach works and show how to map from the old to the new implementation. There used to be an explicit line declaring the dims, so the new code has the equivalent line declaring the dims.

I am going to stop working on this PR. I don't like the solution I came up with for compiling the qhull wrapper (composed of both C and C++ files) as I ended up altering distutils private methods to prevent C++ specific compiler flags being used to compile C code. Instead I will submit a PR just for the triangulation module part of this PR, and I can consider a better approach for the qhull wrapper in time.

I'm happy just to close this PR and leave the branch hanging around for future comparison. Alternatively it could be kept open but labelled in some way to indicate that it won't ever be merged?

Just close the PR. Open PRs anticipate a follow up action on the PR, even if that action is only "decide what to do".

ianthomas23 added 2 commits August 28, 2022 21:06

Use pybind11 for qhull wrapper

8381ef9

Use pybind11 for tri module

f2f597f

ianthomas23 commented Aug 31, 2022

View reviewed changes

tacaswell added this to the v3.7.0 milestone Aug 31, 2022

Add CI dependencies

cdfaf20

anntzer reviewed Sep 1, 2022

View reviewed changes

Fix module wrapper

b15fd9c

ianthomas23 force-pushed the pybind11_poc branch from 91683d3 to b15fd9c Compare September 1, 2022 20:31

ianthomas23 added 2 commits September 2, 2022 10:11

Avoid using -std=c++11 when compiling C files

293dffc

Address review comments

7da3539

Try altering CI so pybind11 extensions build OK

f8226d8

Remove C++ flags when compiling C files in qhull extension

2f8cfec

ianthomas23 force-pushed the pybind11_poc branch from c3028c3 to 2f8cfec Compare September 4, 2022 14:36

More CI installation of pybind11

846f4c7

ianthomas23 force-pushed the pybind11_poc branch from de3f39e to 846f4c7 Compare September 4, 2022 20:18

ianthomas23 mentioned this pull request Sep 9, 2022

[MNT]: Pybind11 transition plan #23846

Closed

13 tasks done

github-actions Bot added the status: needs rebase label Oct 18, 2022

anntzer reviewed Oct 25, 2022

View reviewed changes

jklymak mentioned this pull request Nov 5, 2022

Add Axes.get_tick_params() method. #23692

Merged

5 tasks done

ianthomas23 closed this Nov 12, 2022

ianthomas23 mentioned this pull request Nov 20, 2022

Use pybind11 for tri module #24522

Merged

QuLogic mentioned this pull request Oct 25, 2023

Use pybind11 for qhull wrapper #27184

Merged

2 tasks done

QuLogic modified the milestones: v3.7.0, v3.9.0 Oct 25, 2023

QuLogic added the status: superseded label Oct 25, 2023

                   setup_requires=[
                       "certifi>=2020.06.20",
                       "numpy>=1.19",
+                      "pybind11>=2.6",

-                      ext = Extension(
+                      ext = Pybind11Extension(
                           "matplotlib._qhull", ["src/_qhull_wrapper.cpp"],
+                          cxx_std=11,

-                  PyTuple_SET_ITEM(tuple, 0, triangles.pyobj());
-                  PyTuple_SET_ITEM(tuple, 1, neighbors.pyobj());
-                  return tuple;
+                  return py::make_tuple(triangles, neighbors);

                * if it is false then they are written to stderr. */
-              static PyObject*
-              delaunay_impl(npy_intp npoints, const double* x, const double* y,
+              py::tuple

-                                   qhull_error_msg[exitcode], exitcode,
-                                   hide_qhull_errors ? "; use python verbose option (-v) to see original qhull error." : "");
-                      return NULL;
+                      std::ostringstream oss;

                     _edges(edges),
                     _neighbors(neighbors)
               {
+                  if (_x.ndim() != 1 || _y.ndim() != 1 || _x.shape(0) != _y.shape(0))

+                  m.doc() = "Computing Delaunay triangulations.\n";
+                  m.def("delaunay", &delaunay,
+                      "delaunay(x, y, /)\n"

+                      "triangles, neighbors : int arrays, shape (ntri, 3)\n"
+                      "    Indices of triangle vertices and indices of triangle neighbors.\n");
+                  m.def("version", &version,

-                                        &yarray.converter_contiguous, &yarray)) {
-                      return NULL;
-                  }
+                  if (x.ndim() != 1 || y.ndim() != 1)

                 - echo %PYTHON_VERSION% %TARGET_ARCH%
                 # Install dependencies from PyPI.
                 - python -m pip install --upgrade -r requirements/testing/all.txt %EXTRAREQS% %PINNEDVERS%
+                - python -m pip install "pybind11<2.10"

                   with pytest.raises(
                           ValueError, match=r'x and y must be 1D arrays of the same length'):
-                      mpl._tri.Triangulation([], [1], [[]], None, None, None, False)

+                          py::arg("edges"),
+                          py::arg("neighbors"),
+                          py::arg("correct_triangle_orientations"),
+                          "Triangulation(x, y, triangles, mask, edges, neighbors)\n"

-                  numpy::array_view<int, ndim> triangles(dims);
-                  int* triangles_ptr = triangles.data();
+                  int dims[2] = {ntri, 3};
+                  IndexArray triangles(dims);

Use pybind11 for C/C++ extensions by ianthomas23 · Pull Request #23787 · matplotlib/matplotlib · GitHub

Repository navigation

Use pybind11 for C/C++ extensions - #23787

Use pybind11 for C/C++ extensions#23787ianthomas23 wants to merge 9 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom ianthomas23:pybind11_pocianthomas23/matplotlib:pybind11_pocCopy head branch name to clipboard

Conversation

ianthomas23 commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianthomas23 commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Sep 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Sep 1, 2022

Uh oh!

oscargus commented Sep 1, 2022

Uh oh!

anntzer commented Sep 1, 2022

Uh oh!

tacaswell commented Sep 1, 2022

Uh oh!

ianthomas23 commented Sep 1, 2022

Uh oh!

tacaswell commented Sep 1, 2022

Uh oh!

anntzer commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

story645 commented Sep 1, 2022

Uh oh!

ianthomas23 commented Sep 2, 2022

Uh oh!

ianthomas23 commented Sep 2, 2022

Uh oh!

ianthomas23 commented Sep 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Use pybind11 for C/C++ extensions#23787
ianthomas23 wants to merge 9 commits into
matplotlib:mainfrom
ianthomas23:pybind11_poc

ianthomas23 commented Aug 31, 2022 •
edited
Loading

ianthomas23 commented Aug 31, 2022 •
edited
Loading

anntzer commented Sep 1, 2022 •
edited
Loading