Skip to content

Commit 7c4b306

Browse files
lgritzlgritz
authored
fix(python): Address new pybind11 float/int auto-conversion behavior (#5058)
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. --------- Signed-off-by: Larry Gritz <[email protected]>
1 parent 8ab986b commit 7c4b306

File tree

16 files changed

+83
-1030
lines changed

16 files changed

+83
-1030
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ jobs:
439439
fmt_ver: 12.1.0
440440
opencolorio_ver: v2.5.0
441441
openexr_ver: v3.4.3
442-
pybind11_ver: v3.0.1
442+
pybind11_ver: v3.0.2
443443
python_ver: "3.12"
444444
simd: avx2,f16c
445445
setenvs: export LIBJPEGTURBO_VERSION=3.1.2
@@ -465,7 +465,7 @@ jobs:
465465
fmt_ver: master
466466
opencolorio_ver: main
467467
openexr_ver: main
468-
pybind11_ver: v3.0.1
468+
pybind11_ver: master
469469
python_ver: "3.12"
470470
simd: avx2,f16c
471471
benchmark: 1
@@ -524,7 +524,7 @@ jobs:
524524
fmt_ver: 12.1.0
525525
opencolorio_ver: v2.5.0
526526
openexr_ver: v3.4.3
527-
pybind11_ver: v3.0.1
527+
pybind11_ver: v3.0.2
528528
python_ver: "3.12"
529529
setenvs: export LIBJPEGTURBO_VERSION=3.1.2
530530
LIBPNG_VERSION=v1.6.50
@@ -545,7 +545,7 @@ jobs:
545545
fmt_ver: 12.1.0
546546
opencolorio_ver: v2.5.0
547547
openexr_ver: v3.4.3
548-
pybind11_ver: v3.0.1
548+
pybind11_ver: v3.0.2
549549
python_ver: "3.12"
550550
setenvs: export LIBJPEGTURBO_VERSION=3.1.2
551551
LIBPNG_VERSION=v1.6.50

‎src/cmake/build_pybind11.cmake‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ build_dependency_with_cmake(pybind11
2121
VERSION ${pybind11_BUILD_VERSION}
2222
GIT_REPOSITORY ${pybind11_GIT_REPOSITORY}
2323
GIT_TAG ${pybind11_GIT_TAG}
24+
GIT_COMMIT ${pybind11_GIT_COMMIT}
2425
CMAKE_ARGS
2526
-D PYBIND11_PYTHON_VERSION=${PYTHON3_VERSION}
2627
# Don't built unnecessary parts of Pybind11

‎src/python/py_imagecache.cpp‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -80,21 +80,11 @@ declare_imagecache(py::module& m)
8080
.def_static("destroy", &ImageCacheWrap::destroy, "cache"_a,
8181
"teardown"_a = false)
8282

83-
.def("attribute",
84-
[](ImageCacheWrap& ic, const std::string& name, float val) {
85-
if (ic.m_cache)
86-
ic.m_cache->attribute(name, val);
87-
})
88-
.def("attribute",
89-
[](ImageCacheWrap& ic, const std::string& name, int val) {
90-
if (ic.m_cache)
91-
ic.m_cache->attribute(name, val);
92-
})
9383
.def("attribute",
9484
[](ImageCacheWrap& ic, const std::string& name,
95-
const std::string& val) {
85+
const py::object& obj) {
9686
if (ic.m_cache)
97-
ic.m_cache->attribute(name, val);
87+
attribute_onearg(*ic.m_cache, name, obj);
9888
})
9989
.def("attribute",
10090
[](ImageCacheWrap& ic, const std::string& name, TypeDesc type,

‎src/python/py_imagespec.cpp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,13 +175,9 @@ declare_imagespec(py::module& m)
175175
// For now, do not expose auto_stride. It's not obvious that
176176
// anybody will want to do pointer work and strides from Python.
177177

178-
.def("attribute", [](ImageSpec& spec, const std::string& name,
179-
float val) { spec.attribute(name, val); })
180-
.def("attribute", [](ImageSpec& spec, const std::string& name,
181-
int val) { spec.attribute(name, val); })
182178
.def("attribute",
183179
[](ImageSpec& spec, const std::string& name,
184-
const std::string& val) { spec.attribute(name, val); })
180+
const py::object& obj) { attribute_onearg(spec, name, obj); })
185181
.def("attribute",
186182
[](ImageSpec& spec, const std::string& name, TypeDesc type,
187183
const py::object& obj) {

‎src/python/py_oiio.cpp‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -249,12 +249,24 @@ oiio_getattribute_typed(const std::string& name, TypeDesc type = TypeUnknown)
249249
}
250250

251251

252-
// Wrapper to let attribute_typed work for global attributes.
252+
// Wrapper to let attribute_typed/attribute_onearg work for global attributes.
253253
struct oiio_global_attrib_wrapper {
254254
bool attribute(string_view name, TypeDesc type, const void* data)
255255
{
256256
return OIIO::attribute(name, type, data);
257257
}
258+
bool attribute(string_view name, int val)
259+
{
260+
return OIIO::attribute(name, val);
261+
}
262+
bool attribute(string_view name, float val)
263+
{
264+
return OIIO::attribute(name, val);
265+
}
266+
bool attribute(string_view name, const std::string& val)
267+
{
268+
return OIIO::attribute(name, val);
269+
}
258270
};
259271

260272

@@ -297,13 +309,9 @@ OIIO_DECLARE_PYMODULE(PYMODULE_NAME)
297309

298310
// Global (OpenImageIO scope) functions and symbols
299311
m.def("geterror", &OIIO::geterror, "clear"_a = true);
300-
m.def("attribute", [](const std::string& name, float val) {
301-
OIIO::attribute(name, val);
302-
});
303-
m.def("attribute",
304-
[](const std::string& name, int val) { OIIO::attribute(name, val); });
305-
m.def("attribute", [](const std::string& name, const std::string& val) {
306-
OIIO::attribute(name, val);
312+
m.def("attribute", [](const std::string& name, const py::object& obj) {
313+
oiio_global_attrib_wrapper wrapper;
314+
attribute_onearg(wrapper, name, obj);
307315
});
308316
m.def("attribute",
309317
[](const std::string& name, TypeDesc type, const py::object& obj) {

‎src/python/py_oiio.h‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,26 @@ attribute_typed(T& myobj, string_view name, TypeDesc type, const POBJ& dataobj)
509509

510510

511511

512+
// Dispatch a single-value attribute() call based on the type of the
513+
// Python object (int, float, or string).
514+
template<typename T>
515+
inline void
516+
attribute_onearg(T& myobj, string_view name, const py::object& obj)
517+
{
518+
if (py::isinstance<py::float_>(obj))
519+
myobj.attribute(name, float(obj.template cast<py::float_>()));
520+
else if (py::isinstance<py::int_>(obj))
521+
myobj.attribute(name, int(obj.template cast<py::int_>()));
522+
else if (py::isinstance<py::str>(obj))
523+
myobj.attribute(name, std::string(obj.template cast<py::str>()));
524+
else if (py::isinstance<py::bytes>(obj))
525+
myobj.attribute(name, std::string(obj.template cast<py::bytes>()));
526+
else
527+
throw py::type_error("attribute() value must be int, float, or str");
528+
}
529+
530+
531+
512532
// `data` points to values of `type`. Make a python object that represents
513533
// them.
514534
py::object

‎src/python/py_paramvalue.cpp‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ ParamValue_from_pyobject(string_view name, TypeDesc type, int nvalues,
7979

8080

8181

82+
static void
83+
ParamValueList_attribute_onearg(ParamValueList& self, const std::string& name,
84+
const py::object& obj)
85+
{
86+
attribute_onearg(self, name, obj);
87+
}
88+
89+
90+
8291
// Based on attribute_typed in py_oiio.h, but with nvalues.
8392
template<typename T, typename POBJ>
8493
bool
@@ -240,18 +249,7 @@ declare_paramvalue(py::module& m)
240249
"value"_a, "casesensitive"_a = true)
241250
.def("sort", &ParamValueList::sort, "casesensitive"_a = true)
242251
.def("merge", &ParamValueList::merge, "other"_a, "override"_a = false)
243-
.def("attribute",
244-
[](ParamValueList& self, const std::string& name, float val) {
245-
self.attribute(name, TypeFloat, &val);
246-
})
247-
.def("attribute", [](ParamValueList& self, const std::string& name,
248-
int val) { self.attribute(name, TypeInt, &val); })
249-
.def("attribute",
250-
[](ParamValueList& self, const std::string& name,
251-
const std::string& val) {
252-
const char* s = val.c_str();
253-
self.attribute(name, TypeString, &s);
254-
})
252+
.def("attribute", ParamValueList_attribute_onearg, "name"_a, "val"_a)
255253
.def("attribute",
256254
[](ParamValueList& self, const std::string& name, TypeDesc type,
257255
const py::object& obj) {

‎src/python/py_texturesys.cpp‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -177,21 +177,11 @@ declare_texturesystem(py::module& m)
177177
.def(py::init<bool>(), "shared"_a = true)
178178
.def_static("destroy", &TextureSystemWrap::destroy)
179179

180-
.def("attribute",
181-
[](TextureSystemWrap& ts, const std::string& name, float val) {
182-
if (ts.m_texsys)
183-
ts.m_texsys->attribute(name, val);
184-
})
185-
.def("attribute",
186-
[](TextureSystemWrap& ts, const std::string& name, int val) {
187-
if (ts.m_texsys)
188-
ts.m_texsys->attribute(name, val);
189-
})
190180
.def("attribute",
191181
[](TextureSystemWrap& ts, const std::string& name,
192-
const std::string& val) {
182+
const py::object& obj) {
193183
if (ts.m_texsys)
194-
ts.m_texsys->attribute(name, val);
184+
attribute_onearg(*ts.m_texsys, name, obj);
195185
})
196186
.def("attribute",
197187
[](TextureSystemWrap& ts, const std::string& name, TypeDesc type,

0 commit comments

Comments
 (0)