-
Notifications
You must be signed in to change notification settings - Fork 146
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
Pin pybind11 version to v2.10.0 commit id to avoid unanticipated changes #260
Conversation
Nice find! So this is pinning to a commit before the locale changes were introduced?
Can you point to the configuration you're referring to? |
I pinned to the v2.10.0 release commit id - I believe the change came in around v2.10.2 or v2.10.3. I figured that was the original intent. For configuration, we'd have to change the constructor from the default to one that passes in a PyConfig object: I think actually the bug has been fixed upstream as my guess is that the intent of the pybind initialization without arguments was to be backwards compatible. This is master: Rationale described here: pybind/pybind11#4473 But this is the branch we were pulling from: Starting from an isolated config and adding in "use environment" I believe is not quite enough. Though the environment is passed my guess is that the configure_locale option also needs to be set: https://docs.python.org/3/c-api/init_config.html#c.PyPreConfig.configure_locale Looks like in master they have switched to use the python config as base - which behaves closer to the python interpreter instead of the isolated mode in v2.10 branch. I figured since we hadn't intentionally moved forward - the safest was to move back to the v2.10.0 commit id (but we can decide to upgrade / create our own config instead) (also waiting on CI to test a build - so for now a placeholder) |
Using a tag or branch in the GIT_TAG FetchContents CMake setting can introduce unwanted changes. Thus pinning to a specific commit id is recommended.
https://cmake.org/cmake/help/latest/module/ExternalProject.html#command:externalproject_add
In particular - this change (merged into the v2.10 branch of pybind11)
pybind/pybind11@8869984
changed the behavior of locale setting in the python stub process.
Alternately we can explicit configuration before initialization but in either case pinning the version of pybind is desireable.
Fixes triton-inference-server/server#5321