conda-forge / pybind11-feedstock

A conda-smithy repository for pybind11.
BSD 3-Clause "New" or "Revised" License
5 stars 24 forks source link

pybind11 2.12.0 on Windows creates extensions that are not ABI compatible with extensions compiled with pybind11 2.11.0 and/or with a different version of MSVC #95

Open traversaro opened 5 months ago

traversaro commented 5 months ago

Solution to issue cannot be found in the documentation.

Issue

There is an extensive discussion on this in https://github.com/pybind/pybind11/pull/4779 and https://github.com/pybind/pybind11/pull/4953 . I open this issue as this change got released in conda-forge as part of https://github.com/conda-forge/pybind11-feedstock/pull/94, so this started hitting usage downstream of conda-forge (for example https://github.com/conda-forge/bipedal-locomotion-framework-feedstock/pull/61#issuecomment-2033754996)

Installed packages

pybind11 2.12.0

Environment info

Not relevant (I guess).
henryiii commented 5 months ago

I think there's a mechanism for an ABI bump that we should have / should use. Everything needs to be rebuilt with 2.12, since it's required for NumPy 2.0 compat.

traversaro commented 5 months ago

Sorry, I edited the issue after creating, now I added a few links for context.

traversaro commented 5 months ago

I think there's a mechanism for an ABI bump that we should have / should use. Everything needs to be rebuilt with 2.12, since it's required for NumPy 2.0 compat.

The defaults PR contains some reference to the pybind11-abi version changes: https://github.com/AnacondaRecipes/pybind11-feedstock/pull/15#issue-2213935393 .

henryiii commented 5 months ago

Nice, that looks right. We should do the same.

traversaro commented 5 months ago

To add some more info, it seems that the bump to the 5 abi for Python 3.12+ already occured in 2.11.0 : https://github.com/pybind/pybind11/blob/v2.11.0/include/pybind11/detail/internals.h#L39 .

henryiii commented 5 months ago

No, that means it happened in the development between releases, not that it was in a 2.11 release. Ahh, you mean for Python 3.12, yes.

henryiii commented 5 months ago

That's complicated, actually, since we've probably been considering extensions build with 3.12 to be pybind11-abi==4, which is fine (since they never talk to extensions built with the actual ABI 4), but now fixing it the new extensions will need to be recompiled when they are actually compatible. But I assume that's okay, since the rest of the versions need recompilation anyway.

JeanChristopheMorinPerso commented 5 months ago

What a nice coincidence! We (anaconda) were planning on creating an issue to discuss getting our changes into conda-forge :)

traversaro commented 2 months ago

Ok, some time has passed, and my reckless workaround of just pinning pybind11 to <2.12.0 in my feedstock now fails as we need to migrate to numpy 2.12, that indeed is supported by pybind11 >=2.12.0 . So I need to look into this.

The quickest fix is to do something similar to https://github.com/AnacondaRecipes/pybind11-feedstock/pull/15 in this recipe. This will ensure that downstream packages that build against future builds of pybind11 will have the correct export of pybind-abi (i.e. 5 for Windows and Python>=3.12, 4 for the rest).

The downside of this is what @henryiii reported in https://github.com/conda-forge/pybind11-feedstock/issues/95#issuecomment-2034770732, this will create a conda incompatibility between packages that actually are built with pybind-abi==5 but they run export with pybind-abi==4. This may mitigate with some kind of repodata patch, but to be honest I am not sure how to prepare a repodata patch that looks into the rendered_recipe to decide if it is necessary to change the pybind-abi dependency.

Another related downside is that since the 2.12.0 release pybind11 extensions built with different version of MSVC (even just minor, like if an extension is compiled with Visual Studio 2022 version 17.9 and another with Visual Studio 2022 version 17.10, see https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170) are incompatible, even if those versions are ABI compatible. On one hand, this is similar to what happens already for gcc and clang (see https://github.com/conda-forge/pybind11-feedstock/issues/77), however for MSVC this behavior is particularly problematic:

However, the MSVC incompatibility is already there since https://github.com/conda-forge/pybind11-feedstock/pull/94, and I wonder why no one reported any issue related to that. Perhaps it is not so common in conda-forge to have two pybind11-based extensions that interact (I know at least two cases of these kind of interactions, but are all from recipe that I mantain), or the errors that emerge are so confusing that users do not track it back to this feedstock?

traversaro commented 2 months ago

Ah, we need alto to change the global pin (see https://github.com/conda-forge/pybind11-feedstock/issues/53) to be 5 on Windows or in Python 3.12 . Does this mean that we need to add pybind11-abi to the python zip_keys ?

traversaro commented 5 days ago

This issue probably will become more relevant as pybind >= 2.12.0 is required for numpy 2.0 compatibility.