fix(python): Address new pybind11 float/int auto-conversion behavior#5058
Open
lgritz wants to merge 4 commits intoAcademySoftwareFoundation:mainfrom
Open
fix(python): Address new pybind11 float/int auto-conversion behavior#5058lgritz wants to merge 4 commits intoAcademySoftwareFoundation:mainfrom
lgritz wants to merge 4 commits intoAcademySoftwareFoundation:mainfrom
Conversation
It's not in any pybind11 tagged release yet, but in its master, a change has been introduced that causes it to allow functions with float paramters to automatically match int arguments. This is fatal for us, as we have set up several cases of methods that overload on int vs float, and with this change, the float one gets called when an int is passed. Pybind11 docs say that the `.noconvert()` modifier on the argument will do the trick, but I'm finding that it doesn't help for overloaded function names. The solution is to find these cases, change our declaration to a generic py::object, and then sort out what type was passed with py::isinstance. It seems to mainly affect `attribute()` style calls. In the process, minor changes to the pybind11 auto-builder allow it to take a git commit hash directly. Also, I took the opportunity to remove a few reference outputs that are no longer needed because they were python2-specific. Meanwhile, just in case anybody had been paying attention, the problem in pybind11 master that had caused us to temporarily stop CI testing against master seems to have been resolved. So re-enable the bleeding edge test to use pybind11 master (which will fail if not for this PR here), and bump the "latest version" tests t the new pybind11 3.0.2. Signed-off-by: Larry Gritz <lg@larrygritz.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the project’s Python bindings to be resilient to a new pybind11 behavior that implicitly matches int arguments to float parameters, which breaks int vs float overloads (notably attribute()-style APIs). It also refreshes CI pybind11 version coverage and cleans up legacy Python2-related reference outputs.
Changes:
- Replace several
attribute(name, int/float/string)pybind11 overload sets with a singleattribute(name, object)binding that dispatches viapy::isinstanceto preserveint/floatdistinction. - Extend the local pybind11 CMake dependency builder to accept an explicit git commit hash.
- Update CI to test pybind11
v3.0.2and re-enable testing against pybind11master; remove python3-specific reference output files and normalize some ref output formatting.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/python-imagespec/ref/out.txt | Minor ref output normalization (removes stray blank line). |
| testsuite/python-imagespec/ref/out-python3.txt | Removes python3-only reference output. |
| testsuite/python-imageinput/ref/out.txt | Updates reference output formatting (removes L suffixes, aligns formatting). |
| testsuite/python-imageinput/ref/out-python3.txt | Removes python3-only reference output. |
| testsuite/python-imagebuf/ref/out.txt | Updates array pretty-print formatting in reference output. |
| testsuite/python-imagebuf/ref/out-python3.txt | Removes python3-only reference output. |
| testsuite/python-imagebuf/ref/out-alt.txt | Same reference-output formatting updates as out.txt. |
| testsuite/python-imagebuf/ref/out-alt-python3.txt | Removes python3-only reference output. |
| src/python/py_texturesys.cpp | Routes TextureSystem.attribute(name, value) through object-based dispatch to avoid int→float overload capture. |
| src/python/py_paramvalue.cpp | Uses the shared object-based dispatch for ParamValueList.attribute(name, value). |
| src/python/py_oiio.h | Introduces attribute_onearg() helper that dispatches by Python runtime type. |
| src/python/py_oiio.cpp | Updates global OpenImageIO.attribute(name, value) to use the same dispatch helper. |
| src/python/py_imagespec.cpp | Updates ImageSpec.attribute(name, value) to use runtime dispatch helper. |
| src/python/py_imagecache.cpp | Updates ImageCache.attribute(name, value) to use runtime dispatch helper. |
| src/cmake/build_pybind11.cmake | Passes GIT_COMMIT through to the dependency build macro. |
| .github/workflows/ci.yml | Bumps “latest releases” pybind11 to v3.0.2 and sets “bleeding edge” pybind11 to master. |
Comments suppressed due to low confidence (3)
src/python/py_oiio.h:526
attribute_oneargno longer accepts Pythonbytesvalues (onlystr), whereasdelegate_setitemin the same header explicitly supportspy::bytes. Previously theattribute(..., const std::string&)overloads would typically accept bothstrandbytesvia pybind11’s string caster, so this is an API compatibility regression. Consider handlingpy::byteshere (and converting tostd::string) soattribute(name, b"...")continues to work consistently with__setitem__.
// Dispatch a single-value attribute() call based on the type of the
// Python object (int, float, or string).
template<typename T>
inline void
attribute_onearg(T& myobj, string_view name, const py::object& obj)
{
if (py::isinstance<py::float_>(obj))
myobj.attribute(name, float(obj.template cast<py::float_>()));
else if (py::isinstance<py::int_>(obj))
myobj.attribute(name, int(obj.template cast<py::int_>()));
else if (py::isinstance<py::str>(obj))
myobj.attribute(name, std::string(obj.template cast<py::str>()));
else
throw std::invalid_argument("Bad type for attribute");
}
src/python/py_oiio.h:526
- The thrown exception message
"Bad type for attribute"is quite generic and doesn’t indicate which Python types are accepted. Since this will surface as a Python exception for callers, consider including the allowed types (e.g., int/float/str[/bytes]) and/or the actual received type to make debugging easier.
myobj.attribute(name, std::string(obj.template cast<py::str>()));
else
throw std::invalid_argument("Bad type for attribute");
}
src/python/py_paramvalue.cpp:87
ParamValueList_attribute_oneargis a non-staticfree function in this translation unit. Since it’s only used as a binding helper within this file, consider making itstatic(or placing it in an anonymous namespace) to avoid unnecessarily exporting a symbol and to match the existing pattern of other file-local helpers (e.g.,ParamValue_from_pyobject).
void
ParamValueList_attribute_onearg(ParamValueList& self, const std::string& name,
const py::object& obj)
{
attribute_onearg(self, name, obj);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Any comments on this? It's been up for a week. |
Signed-off-by: Larry Gritz <lg@larrygritz.com>
…tside this file Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's not in any pybind11 tagged release yet, but in its master, a change has been introduced that causes it to allow functions with float paramters to automatically match int arguments. This is fatal for us, as we have set up several cases of methods that overload on int vs float, and with this change, the float one gets called when an int is passed. Pybind11 docs say that the
.noconvert()modifier on the argument will do the trick, but I'm finding that it doesn't help for overloaded function names.The solution is to find these cases, change our declaration to a generic py::object, and then sort out what type was passed with py::isinstance. It seems to mainly affect
attribute()style calls.In the process, minor changes to the pybind11 auto-builder allow it to take a git commit hash directly. Also, I took the opportunity to remove a few reference outputs that are no longer needed because they were python2-specific.
Meanwhile, just in case anybody had been paying attention, the problem in pybind11 master that had caused us to temporarily stop CI testing against master seems to have been resolved. So re-enable the bleeding edge test to use pybind11 master (which will fail if not for this PR here), and bump the "latest version" tests to the latest pybind11 3.0.2.
I diagnosed the problem myself, wrote the solution for one class and verified that it solved the issues with that class. Then I used Claude Code to make the analogous set of changes to the other classes that needed them and I reviewed that it was correct.