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

ENH add global pin for pybind11-abi #849

Merged
merged 3 commits into from
Dec 10, 2020
Merged

Conversation

beckermr
Copy link
Member

@beckermr beckermr commented Oct 9, 2020

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

cc @isuruf @SylvainCorlay @xhochy

@beckermr beckermr requested a review from a team as a code owner October 9, 2020 14:59
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Please create an issue in pybind11-feedstock about why we are doing this and ping conda-forge/pybind11 team there

@beckermr
Copy link
Member Author

beckermr commented Oct 9, 2020

@bluescarni do you have an example in code of what goes wrong or a link?

@bluescarni
Copy link
Contributor

bluescarni commented Oct 9, 2020

@beckermr @isuruf

I'll try to collect here a few useful links.

First, from the official pybind11 docs (https://pybind11.readthedocs.io/en/stable/upgrade.html#stricter-enforcement-of-hidden-symbol-visibility-for-pybind11-modules):

The namespace-scope hidden visibility is done automatically in pybind11’s headers and it’s generally transparent to users. It ensures that modules compiled with different pybind11 versions don’t clash with each other.

This is admittedly a bit vague. More concretely, see this issue report pybind/pybind11#796 where the pybind11 author states:

pybind11 versions with different minor versions live in different namespaces, i.e. they don't see each other and can't even exchange objeccts

The PYBIND11_INTERNALS_ID macro generates a version-specific ID that isolates the internals data structure from other ones

The PYBIND11_INTERNALS_ID macro is still there in the current version, see:

https://github.com/pybind/pybind11/blob/d65e34d61d5f49ffbe7c2e7864391366f1124b0e/include/pybind11/detail/internals.h#L202

where it seems like the versioning of the internals does not depend only on the pybind11 version, but also compiler, stdlib, etc.

I'll try to add some more info later if I find anything else.

@beckermr
Copy link
Member Author

beckermr commented Oct 9, 2020

Thanks @bluescarni!

I made an issue here @isuruf: conda-forge/pybind11-feedstock#53

Good idea to ping the dev team for their input!

@beckermr
Copy link
Member Author

beckermr commented Oct 9, 2020

bump here @conda-forge/core @conda-forge/pybind11

@isuruf
Copy link
Member

isuruf commented Oct 9, 2020

Let's wait for more info conda-forge/pybind11-feedstock#53
Besides, pybind11 is only in host, so pin_run_as_build wouldn't apply.

@beckermr
Copy link
Member Author

beckermr commented Oct 9, 2020

Besides, pybind11 is only in host, so pin_run_as_build wouldn't apply.

That can't be true, right? Most of the things in pin run as build now are only host too. Do all of those entries do nothing and we don't have proper run exports?

@beckermr
Copy link
Member Author

beckermr commented Oct 9, 2020

At least some of things there do have run exports, so those entries should be removed. There is at least one exception. Poppler ships libraries and doesn't have a run export AFAICT

@isuruf
Copy link
Member

isuruf commented Oct 24, 2020

We probably need a weak_constrains type for run_exports which would add the deps to the run_constrained section. @bluescarni, would you have some time to send a PR to conda-build? See https://github.com/conda/conda-build/blob/476cb9b09cc56f4fcc0f5cdb0840568e1c90a277/conda_build/render.py#L380

@henryiii
Copy link
Contributor

Pybind11 has a PYBIND11_INTERNALS_ID which is used to build an ABI string, along with compiler information, like __GXX_ABI_VERSION. It is compiled into the project using it - pybind11 is header only; it is not a "normal" Python package - there's (currently) no shared or precompiled part. So "MyClass" ends up with a name including the internals ID version and these bits of information. If you try to return a "MyClass" from a different extension, it looks for this symbol with matching "mangled" if you will, name - if it does not exist, it doesn't know how to return a MyClass. I don't think this functionality is heavily used except in tightly-coupled systems of packages, but if it is used, I think you would want to pin between packages that are compatible, rather than doing it conda-forge wide. Packages wanting to participate via pinning of some sort would also need to use the conda-forge package and not a subdirectory, as well.

Many projects are hardcoded to a specific Python version via git submodule, including "current master" much of the time. That's why the INTERNAL_IDs version exists - anytime the internal structure changes, it gets bumped. 2.5 seems to be the last time it was bumped (to 4, FYI). So a 2.5 extension should be able to work with a 2.6.1 one.

I do get these mixed sometimes, but I think this means that pybind11 could always be listed in "build" and not in "host".

@beckermr
Copy link
Member Author

I think you would want to pin between packages that are compatible, rather than doing it conda-forge wide.

@henryiii This same statement could be made about any ABI pin in conda-forge. If you don't ever plan on using two packages together, then they don't need to be pinned to the same ABI of some shared dependency.

However, nobody can really be sure that two packages won't eventually be co-installed. So if there is some ABI-like limitation, we need to manage it. Otherwise, we will get a bunch of problems that conda-forge was actually built to solve, namely

  1. environments are that are not solvable
  2. buggy environments (like the case discussed above) where the ABI or ABI-like constraints in the package metadata don't indicate the actual incompatibilities between packages

Thus we absolutely should create pins for common dependencies, even header-only dependencies, that generate ABI or ABI-like incompatibilities.

At minimum pybind11 needs some sort of export (either run or run constrained) to indicate in the metadata that things are incompatible. However, once you do that, you want all packages on the same version and so you need to pin.

At least this is my understanding of things. Other more knowledgable @conda-forge/core members should comment too!

@henryiii
Copy link
Contributor

If you don't ever plan on using two packages together, then they don't need to be pinned to the same ABI of some shared dependency.

The difference here is that there is not a strong coupling by default. If Boost-histogram and PyTorch have different versions of pybind11 (and, at least until recently, they did, quite drastically), it doesn't matter, there is no incompatibility at all. If you get both packages, you shouldn't even get pybind11; that's only a build dependency. Only if I write boost-histogram in such a way that it returns a PyTorch object, and then expect PyTorch to do the C++ -> Python translation for me does the ABI version matter. This sort of connection is pretty uncommon as far as I know, except in special cases, usually multiple extensions of the same package ecosystem. So pytorch-myext might need the same pybind11 that pytorch used. But that's locked in by pytorch version - the current version of pytorch has a commit in-between 2.3 and 2.4 baked in. The master version is finally on a reasonable 2.6.0 tag, I believe.

Unlike most "run exports" packages, where you have to load the same library at runtime, there is no runtime component for pybind11. A "user" should never have it - it's for building only.

I'm not actually saying that this is the wrong thing to do, by the way. It could be that if a package wants to participate in pybind11 pinning because there may be other packages returning items from them - for example, GooFit might return iMinuit objects after the iMinuit rewrite to pybind11, then there should be a way to opt into this.

@beckermr
Copy link
Member Author

Right. So we make the global pin and then people opt-in via using pybind11 in host without a pin. If they instead pin it themselves or vendor it in their build, then they are opting-out.

The global pin is not forcing people to use it, but it is the correct way to allow people to opt-in to it.

@henryiii
Copy link
Contributor

Can you use it in build: to opt out, and in host: to opt in? That would be ideal, I think. (I'm still a bit rough with the difference between host: and build:)

@beckermr
Copy link
Member Author

Can you use it in build: to opt out, and in host: to opt in? That would be ideal, I think. (I'm still a bit rough with the difference between host: and build:)

I am not sure @henryiii. Maybe someone on @conda-forge/core can weigh in?

@henryiii
Copy link
Contributor

henryiii commented Nov 12, 2020

There probably also is not a way to specify that, for example, 2.5 and 2.6 are compatible? On average the pinning has been bumped roughly every 1-2 minor versions (and never in a patch version, which is good / most important)

@beckermr
Copy link
Member Author

There probably also is not a way to specify that, for example, 2.5 and 2.6 are compatible? On average the pinning has been bumped roughly every 1-2 minor versions.

This can be changed in the run / run constrained export of bybind11 itself. We can also skip some pin changes as needed.

@isuruf
Copy link
Member

isuruf commented Nov 12, 2020

we can have a pybind11-internals meta package with the version being PYBIND11_INTERNALS_ID

@bluescarni
Copy link
Contributor

bluescarni commented Nov 13, 2020

The difference here is that there is not a strong coupling by default. If Boost-histogram and PyTorch have different versions of pybind11 (and, at least until recently, they did, quite drastically), it doesn't matter, there is no incompatibility at all. If you get both packages, you shouldn't even get pybind11; that's only a build dependency. Only if I write boost-histogram in such a way that it returns a PyTorch object, and then expect PyTorch to do the C++ -> Python translation for me does the ABI version matter. This sort of connection is pretty uncommon as far as I know, except in special cases, usually multiple extensions of the same package ecosystem. So pytorch-myext might need the same pybind11 that pytorch used. But that's locked in by pytorch version - the current version of pytorch has a commit in-between 2.3 and 2.4 baked in. The master version is finally on a reasonable 2.6.0 tag, I believe.

Chiming in just to say that this is the exact usage scenario that motivated the initial discussion on the conda forge chat channel. In our case, we have a stack of interdependent C++ libraries, each one on a different release schedule and each one with its own package on conda-forge, and each one with its own set of pybind11 bindings. On the Python side, we need to be able to pass objects of type A exposed from the C++ library a to functions in library b, and this is where the ABI version starts to matter.

At the moment we are doing a manual pinning in all our recipes to a specific pybind11 commit, but of course this is hackish and it requires a simultaneous update of all recipes if we ever want to change the pinned version of pybind11. Having a more robust solution for this scenario in conda would be great.

@beckermr
Copy link
Member Author

beckermr commented Nov 13, 2020

Ok. I propose we create the pybind11-abi metapackage. Then we can add run constrained on pybind11 itself for this package. Finally, we can use a strong run export of pybind11-abi on itself. Then if people need the abi constraint, then can include the pybind11-abi package in their host or build envs and it will propagate, forcing any downstream builds to require the same abi. Finally, we can put a pin on the abi package here.

@beckermr
Copy link
Member Author

beckermr commented Nov 13, 2020

@conda-forge/core @conda-forge/pybind11 Is the plan above satisfactory?

edit: copied here

Ok. I propose we create the pybind11-abi metapackage. Then we can add run constrained on pybind11 itself for this package. Finally, we can use a strong run export of pybind11-abi on itself. Then if people need the abi constraint, then can include the pybind11-abi package in their host or build envs and it will propagate, forcing any downstream builds to require the same abi. Finally, we can put a pin on the abi package here.

@henryiii
Copy link
Contributor

I think it's fine, @wjakob thought so too.

@beckermr
Copy link
Member Author

beckermr commented Nov 14, 2020

@henryiii Which internals IDs should we use?

Is the PYBIND11_INTERNALS_VERSION enough, or do we need to different ones for with and without threads, debug on windows etc?

Here is the code for reference

/// Tracks the `internals` and `type_info` ABI version independent of the main library version
#define PYBIND11_INTERNALS_VERSION 4

/// On MSVC, debug and release builds are not ABI-compatible!
#if defined(_MSC_VER) && defined(_DEBUG)
#   define PYBIND11_BUILD_TYPE "_debug"
#else
#   define PYBIND11_BUILD_TYPE ""
#endif

/// Let's assume that different compilers are ABI-incompatible.
#if defined(_MSC_VER)
#   define PYBIND11_COMPILER_TYPE "_msvc"
#elif defined(__INTEL_COMPILER)
#   define PYBIND11_COMPILER_TYPE "_icc"
#elif defined(__clang__)
#   define PYBIND11_COMPILER_TYPE "_clang"
#elif defined(__PGI)
#   define PYBIND11_COMPILER_TYPE "_pgi"
#elif defined(__MINGW32__)
#   define PYBIND11_COMPILER_TYPE "_mingw"
#elif defined(__CYGWIN__)
#   define PYBIND11_COMPILER_TYPE "_gcc_cygwin"
#elif defined(__GNUC__)
#   define PYBIND11_COMPILER_TYPE "_gcc"
#else
#   define PYBIND11_COMPILER_TYPE "_unknown"
#endif

#if defined(_LIBCPP_VERSION)
#  define PYBIND11_STDLIB "_libcpp"
#elif defined(__GLIBCXX__) || defined(__GLIBCPP__)
#  define PYBIND11_STDLIB "_libstdcpp"
#else
#  define PYBIND11_STDLIB ""
#endif

/// On Linux/OSX, changes in __GXX_ABI_VERSION__ indicate ABI incompatibility.
#if defined(__GXX_ABI_VERSION)
#  define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION)
#else
#  define PYBIND11_BUILD_ABI ""
#endif

#if defined(WITH_THREAD)
#  define PYBIND11_INTERNALS_KIND ""
#else
#  define PYBIND11_INTERNALS_KIND "_without_thread"
#endif

#define PYBIND11_INTERNALS_ID "__pybind11_internals_v" \
    PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI PYBIND11_BUILD_TYPE "__"

@beckermr
Copy link
Member Author

beckermr commented Dec 9, 2020

See this issue: conda-forge/pybind11-feedstock#63

@beckermr beckermr changed the title ENH add migration for pybind11 ENH add global pin for pybind11-abi Dec 9, 2020
@beckermr
Copy link
Member Author

@isuruf The abi package is made and just merged. I am going to merge this PR with the actual pin. We should add some repodata patches on the older pybind11 builds too. I will do that later today.

@beckermr beckermr merged commit b3acfd3 into conda-forge:master Dec 10, 2020
@beckermr beckermr deleted the pybind11 branch December 10, 2020 16:41
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.

5 participants