diff --git a/.github/workflows/build-steps.yml b/.github/workflows/build-steps.yml index 891cb3dbd2..7bd26db687 100644 --- a/.github/workflows/build-steps.yml +++ b/.github/workflows/build-steps.yml @@ -54,6 +54,8 @@ on: type: string pybind11_ver: type: string + python_bindings_backend: + type: string python_action_ver: type: string python_ver: @@ -133,6 +135,7 @@ jobs: OPENEXR_VERSION: ${{inputs.openexr_ver}} OPENIMAGEIO_VERSION: ${{inputs.openimageio_ver}} PYBIND11_VERSION: ${{inputs.pybind11_ver}} + OSL_PYTHON_BINDINGS_BACKEND: ${{inputs.python_bindings_backend}} PYTHON_VERSION: ${{inputs.python_ver}} USE_BATCHED: ${{inputs.batched}} ABI_CHECK: ${{inputs.abi_check}} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5b3bb484bd..cb6f892e4f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,6 +64,7 @@ jobs: openexr_ver: ${{ matrix.openexr_ver }} openimageio_ver: ${{ matrix.openimageio_ver }} pybind11_ver: ${{ matrix.pybind11_ver }} + python_bindings_backend: ${{ matrix.python_bindings_backend }} python_ver: ${{ matrix.python_ver }} setenvs: ${{ matrix.setenvs }} simd: ${{ matrix.simd }} @@ -148,6 +149,7 @@ jobs: openexr_ver: ${{ matrix.openexr_ver }} openimageio_ver: ${{ matrix.openimageio_ver }} pybind11_ver: ${{ matrix.pybind11_ver }} + python_bindings_backend: ${{ matrix.python_bindings_backend }} python_ver: ${{ matrix.python_ver }} setenvs: ${{ matrix.setenvs }} simd: ${{ matrix.simd }} @@ -196,9 +198,7 @@ jobs: python_ver: "3.11" simd: avx2,f16c batched: b8_AVX2 - setenvs: export CTEST_EXCLUSIONS="broken|python-oslquery" - # ^^ exclude python-oslquery test until the ASWF container properly - # includes OIIO's python bindings, then we can remove that. + setenvs: export CTEST_EXCLUSIONS="broken" - desc: VP2026 gcc14/C++20 llvm20 py3.13 oiio-3.1 avx2 nametag: linux-vfx2026 runner: ubuntu-latest @@ -209,9 +209,7 @@ jobs: batched: b8_AVX2 # OSL_TEST_CPP_BACKEND=1 also exercises the C++ source-gen backend # (debug_output_cpp=3) on this variant, validating the Linux .so path. - setenvs: export CTEST_EXCLUSIONS="broken|python-oslquery" OSL_TEST_CPP_BACKEND=1 - # ^^ exclude python-oslquery test until the ASWF container properly - # includes OIIO's python bindings, then we can remove that. + setenvs: export CTEST_EXCLUSIONS="broken" OSL_TEST_CPP_BACKEND=1 # Address and leak sanitizers (debug build) - desc: sanitizers nametag: sanitizer @@ -432,6 +430,7 @@ jobs: openexr_ver: ${{ matrix.openexr_ver }} openimageio_ver: ${{ matrix.openimageio_ver }} pybind11_ver: ${{ matrix.pybind11_ver }} + python_bindings_backend: ${{ matrix.python_bindings_backend }} python_ver: ${{ matrix.python_ver }} setenvs: ${{ matrix.setenvs }} simd: ${{ matrix.simd }} @@ -512,6 +511,7 @@ jobs: openexr_ver: ${{ matrix.openexr_ver }} openimageio_ver: ${{ matrix.openimageio_ver }} pybind11_ver: ${{ matrix.pybind11_ver }} + python_bindings_backend: ${{ matrix.python_bindings_backend }} python_ver: ${{ matrix.python_ver }} setenvs: ${{ matrix.setenvs }} simd: ${{ matrix.simd }} @@ -582,6 +582,7 @@ jobs: openexr_ver: ${{ matrix.openexr_ver }} openimageio_ver: ${{ matrix.openimageio_ver }} pybind11_ver: ${{ matrix.pybind11_ver }} + python_bindings_backend: ${{ matrix.python_bindings_backend }} python_ver: ${{ matrix.python_ver }} setenvs: ${{ matrix.setenvs }} simd: ${{ matrix.simd }} diff --git a/INSTALL.md b/INSTALL.md index 486188b488..9960df8b4a 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -69,8 +69,20 @@ NEW or CHANGED minimum dependencies since the last major release are **bold**. * (optional) Python: If you are building the Python bindings or running the testsuite: * **Python >= 3.9** (tested through 3.14) - * pybind11 >= 2.7 (tested through 3.0) * NumPy (tested through 2.4) + * A binding framework, depending on `OSL_PYTHON_BINDINGS_BACKEND` (see + [Python binding backends](#python-binding-backends) below): + * pybind11 >= 2.7 (tested through 3.0) -- needed for the `pybind11` + backend and for `both`. It is the auto-selected default when + OpenImageIO is older than 3.2 or Python is older than 3.10. + * nanobind >= 2.8.0 (tested through 3.0), with Python >= 3.10 -- needed + for the `nanobind` backend and for `both`. It is the auto-selected + default when OpenImageIO is 3.2 or newer and Python is 3.10 or newer. + Usually installed as a Python package (`pip install nanobind`, or + `brew install nanobind`), which is enough: the build locates it by + asking the interpreter. If it is not installed, the build fetches and + builds it locally (it is a small header/CMake package, not a + compiled library). * (optional) Qt5 >= 5.6 or Qt6 (tested Qt5 through 5.15 and Qt6 through 6.10). If not found at build time, the `osltoy` application will be disabled. @@ -125,6 +137,56 @@ Here are the steps to check out, build, and test the OSL distribution: make test +Python binding backends +----------------------- + +OSL's Python bindings (the `oslquery` module, wrapping `OSLQuery`) can be +built with either [pybind11](https://github.com/pybind/pybind11) or +[nanobind](https://github.com/wjakob/nanobind). Both are generated from one +set of sources and expose exactly the same Python API; which one you get is a +build-time choice: + + cmake -B build -S . # auto (see below) + cmake -B build -S . -DOSL_PYTHON_BINDINGS_BACKEND=nanobind + cmake -B build -S . -DOSL_PYTHON_BINDINGS_BACKEND=pybind11 + cmake -B build -S . -DOSL_PYTHON_BINDINGS_BACKEND=both + +or equivalently by setting an environment variable of the same name. + +When `OSL_PYTHON_BINDINGS_BACKEND` is left unset, OSL auto-selects `nanobind` +when both of these hold, and `pybind11` otherwise: + +* OpenImageIO is 3.2 or newer. Reading `OSLQuery.Parameter.type` (see below) + needs OSL's and OpenImageIO's Python modules to have been built with the + same binding framework, and OpenImageIO switched its own default to nanobind + in 3.2; matching it keeps `type` working out of the box. +* Python is 3.10 or newer (nanobind's minimum). + +nanobind does not have to be installed for this -- if it is missing the build +fetches and builds it locally. If that local build is not possible in your +environment, configure with `-DOSL_PYTHON_BINDINGS_BACKEND=pybind11`. + +With `pybind11` or `nanobind`, you get a single `oslquery` module installed in +the usual place, and it makes no difference to Python code which one it is. +With `both`, the pybind11 module keeps the ordinary location and the nanobind +one is installed alongside it under a `nanobind/` subdirectory of the +site-packages directory; put that subdirectory on `PYTHONPATH` to import it +instead. `both` exists so that the testsuite can run against each backend and +confirm they agree; it is not intended for deployment. + +Why this is a choice at all: `OSLQuery.Parameter.type` returns an OpenImageIO +`TypeDesc`, and reading that attribute only works if OpenImageIO's own Python +module has been imported *and* was built with the same binding framework as +OSL's. (Each framework keeps its own registry of bound C++ types, and they +cannot see each other's.) So if you use that attribute, build OSL's bindings +to match whatever OpenImageIO you are pairing them with. Otherwise the +attribute raises `TypeError`. + +Everything else in the module is free of that constraint, and +`Parameter.type_name` -- a plain string such as `"color"` or `"float[4]"` -- +gives you the same information with no coupling to OpenImageIO at all. Prefer +it. `type` is retained for backward compatibility. + Conda Environment ----------------- diff --git a/Makefile b/Makefile index c02e4080ab..8615af1acd 100644 --- a/Makefile +++ b/Makefile @@ -293,6 +293,9 @@ test: build PYTHONPATH=${working_dir}/${build_dir}/lib/python/site-packages:${PYTHONPATH} \ ctest -E broken ${TEST_FLAGS} \ ) + # PYTHONPATH here is a convenience for interactive use; the python` tests + # set their own via a CTest ENVIRONMENT property, which wins,` and which + # is how a backend-specific variant finds its module.` @ ( if [[ "${CODECOV}" == "1" ]] ; then \ cd ${build_dir} ; \ lcov -b . -d . -c -o cov.info ; \ diff --git a/docs/dev/.gitignore b/docs/dev/.gitignore index fc616b7d97..aac4ec393d 100644 --- a/docs/dev/.gitignore +++ b/docs/dev/.gitignore @@ -11,3 +11,4 @@ specs/* # we add specs that we intend to commit and make available to all project # developers, they must be individually added here. !specs/002-backend-cpp +!specs/003-nanobind-python-bindings diff --git a/docs/dev/specs/003-nanobind-python-bindings/checklists/requirements.md b/docs/dev/specs/003-nanobind-python-bindings/checklists/requirements.md new file mode 100644 index 0000000000..a14126c11b --- /dev/null +++ b/docs/dev/specs/003-nanobind-python-bindings/checklists/requirements.md @@ -0,0 +1,46 @@ +# Specification Quality Checklist: nanobind Python bindings (dual-backend) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-08-02 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- **On "no implementation details"**: this feature is itself a build-and-packaging + change, so its stakeholders are OSL builders, packagers, and maintainers rather than + end users. The names pybind11 and nanobind appear in the spec because they are the + subject matter - the thing being selected between - not because they are an + implementation choice made while writing the spec. Everything else is stated in terms + of observable outcomes: module identity, public surface equality, install layout, + which tests run, and what fails at configure time. Specific CMake variable names, + macro names, file paths, and header names are deliberately confined to plan.md. +- Two clarifications were resolved with the requester before the spec was written and + are recorded as Assumptions rather than as open questions: the default backend stays + pybind11 (FR-002), and `Parameter.type` is retained as-is in both backends with the + interoperability constraint handled by documentation (FR-015, FR-026). +- All items pass on the first validation iteration. diff --git a/docs/dev/specs/003-nanobind-python-bindings/plan.md b/docs/dev/specs/003-nanobind-python-bindings/plan.md new file mode 100644 index 0000000000..b0022134df --- /dev/null +++ b/docs/dev/specs/003-nanobind-python-bindings/plan.md @@ -0,0 +1,495 @@ +# Implementation Plan: nanobind Python bindings (dual-backend) + +**Branch**: `lg-nanobind` | **Date**: 2026-08-02 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `docs/dev/specs/003-nanobind-python-bindings/spec.md` + +## Summary + +Add `OSL_PYTHON_BINDINGS_BACKEND` (`pybind11` | `nanobind` | `both`; when unset, +auto-selected as of 2026-09-09 - nanobind when OIIO >= 3.2 and Python >= 3.10, +else pybind11; originally shipped with a fixed `pybind11` default) and make +`src/liboslquery/py_osl.{h,cpp}` compile unchanged under either +framework by routing every framework-specific spelling through a new +`src/liboslquery/py_backend.h`. The same sources are compiled once per selected backend +into separate CMake targets. The single Python test is registered once per selected +backend, differentiated only by `PYTHONPATH`, sharing one test script and one reference +output. Structure, naming, and known pitfalls follow OpenImageIO's implementation +(see [research.md](./research.md)). + +## Technical Context + +**Language/Version**: C++17 (OSL's `DOWNSTREAM_CXX_STANDARD`), Python >= 3.9, CMake + +**Primary Dependencies**: pybind11 >= 2.7 (existing, now conditional); nanobind >= 2.8.0 +(new, conditional); OpenImageIO (existing, unchanged) + +**Storage**: N/A + +**Testing**: CTest via `testsuite/runtest.py`; the single test `python-oslquery` + +**Target Platform**: Linux (x86_64, aarch64), macOS (x86_64, arm64), Windows (x64) + +**Project Type**: C++ library with a Python extension module + +**Performance Goals**: N/A - `OSLQuery` is a metadata-inspection API, not a hot path. +No shader execution is involved. + +**Constraints**: The default configuration must be behaviorally identical to the +pre-change build. The two backends must expose an identical public Python surface. +Exactly one copy of the binding source. + +**Scale/Scope**: 2 binding source files, ~350 lines (~120 live). 1 class family +(`OSLQuery` + `OSLQuery::Parameter`), 8 module attributes, 27 bound members. 1 test. + +## Constitution Check + +| Gate | Status | Notes | +|------|--------|-------| +| **I. Backward Compatibility** | PASS | No public C++ header changes. No Python API changes: same module name, same classes, same attributes, same semantics. `Parameter.type` is retained with its existing (pre-existing) OIIO-Python coupling. When the option is unset the default backend is auto-selected (as of 2026-09-09) from the OpenImageIO and Python versions; the Python API is identical whichever backend is chosen, only the binding framework differs. CHANGES.md entry required for the new option and conditional dependency (Principle IV also requires calling out the new dependency minimum). | +| **II. Physical Accuracy** | N/A | No shader execution, no closures, no numerics. `OSLQuery` reads compiled-shader metadata. | +| **III. Test-Driven Quality** | PASS | `python-oslquery` runs against every selected backend against one shared `ref/out.txt` - byte-identical output is the equivalence proof (SC-002, SC-006). Additionally re-enables the long-disabled `for p in q:` iteration coverage (FR-024). No reference images change. | +| **IV. Cross-Platform Portability** | PASS | nanobind is documented with a minimum version (>= 2.8.0) in INSTALL.md and CHANGES.md per the dependency-version rule. The CI jobs default `python_bindings_backend` to `both`, so every job on all three platforms builds and tests both the nanobind default and pybind11 (SC-011). Windows multi-config handling for the second module is explicitly addressed (T019). | +| **V. Performance & Scalability** | PASS | Nothing on any execution path changes. The only build-time cost is compiling ~350 lines a second time, and only when `both` is selected. | + +No violations; Complexity Tracking is omitted. + +## Project Structure + +### Documentation (this feature) + +```text +docs/dev/specs/003-nanobind-python-bindings/ +├── spec.md # Feature specification +├── plan.md # This file +├── research.md # Phase 0 output +├── tasks.md # Phase 2 output (/speckit-tasks) +└── checklists/ + └── requirements.md # Spec quality checklist +``` + +### Source Code (repository root) + +```text +src/liboslquery/ +├── py_backend.h # NEW - the compatibility shim +├── py_osl.h # MODIFIED - includes py_backend.h; helpers de-pybind11-ed +├── py_osl.cpp # MODIFIED - macro substitution; dual module entry point +├── __init__.py # unchanged (pybind11 / single-backend package init) +├── nanobind/ +│ └── __init__.py # NEW - `both`-mode package init for the second module +├── CMakeLists.txt # MODIFIED - two conditional module blocks +└── MIGRATION_STATUS.md # NEW - maintainer conventions + +src/cmake/ +├── pythonutils.cmake # MODIFIED - backend option, nanobind discovery, second setup macro +├── externalpackages.cmake # MODIFIED - conditional pybind11 / nanobind finds +├── build_nanobind.cmake # NEW - local build recipe (copied from OIIO) +└── testing.cmake # MODIFIED - PYTHONPATH helper, per-backend test registration + +testsuite/python-oslquery/ # test script, run.py, ref/ all shared unchanged + +.github/workflows/ # MODIFIED - build-steps.yml input, ci.yml matrix entries +src/build-scripts/ # MODIFIED - ci-build.bash flag, dependency installs +INSTALL.md, CHANGES.md # MODIFIED - dependency + option documentation +``` + +**Structure Decision**: OSL's bindings live inside `src/liboslquery/` alongside the C++ +library, not in a top-level `src/python/`. This plan keeps them there. Unlike OIIO - +which needed a whole second directory (`src/python-nanobind/`) to hold a differing +package `__init__.py` - OSL needs only a one-file `nanobind/` subdirectory for the same +purpose, because the sources can be listed once and compiled twice from a single +`CMakeLists.txt`. + +--- + +## Phase 0 - Prep under pybind11 (no behavior change) + +Lands first; every step is verifiable with the existing test before nanobind enters the +picture. Justification for each item is in [research.md §1](./research.md). + +**`src/liboslquery/py_osl.cpp`** + +1. Delete the five `py::return_value_policy::reference_internal` arguments + (`Parameter.metadata`, `OSLQuery.parameters`, `OSLQuery.metadata`, and both + `__getitem__` overloads). Every decorated lambda returns *by value*, so the policy + never governs a reference into internal state; it is already a no-op. Removing it + deletes a shim entry and the single riskiest nanobind interaction. +2. Replace the value-returning factory constructor with the templated form: + ```cpp + .def(py::init(), + "shadername"_a, "searchpath"_a = "") + ``` + `OSLQuery(string_view, string_view = {})` exists at `src/include/OSL/oslquery.h:118` + and `std::string` converts implicitly. This matters: nanobind has no equivalent of + pybind11's value-returning `py::init(lambda)`, so doing this now avoids needing a + placement-new `__init__` shim later. +3. Delete the unused `#include `. + +**`src/liboslquery/py_osl.h`** + +4. Delete `python_array_code()` / `typedesc_from_python_array_code()` (declared, never + defined, never called) and `object_classname()` (unused, and uses the `.cast<>()` + member syntax nanobind lacks). +5. Delete the unused `` and `` includes. +6. Delete the `C_to_tuple` specialization (never instantiated). + +**`testsuite/python-oslquery/src/test_oslquery.py`** + +7. Fix the stale comment describing the printed type as "an OpenImageIO::TypeDesc but it + can print like a string" - since 642ab36f the test prints `type_name`, a `str`. +8. Re-enable the `for p in q:` loop disabled by a 2020-era `FIXME(pybind11)` about a + macOS crash with pybind11 2.6, and drop the `for i in range(len(q))` workaround. This + is the only coverage of `__iter__` / `make_iterator` / `keep_alive<0,1>` - the + construct that differs most between the backends (FR-024). `ref/out.txt` should not + change; if it does, the change is a bug, not a reference update. If the macOS crash + recurs, leave the loop disabled and record it - it is pre-existing. + +**Gate**: `python-oslquery` passes against an unmodified `ref/out.txt`. + +--- + +## Phase 1 - The compatibility shim + +### New: `src/liboslquery/py_backend.h` + +Modeled on `~/code/oiio/oiio.lg/src/python/py_backend.h`, trimmed to what OSL uses. +Selects on `OSL_PY_BACKEND_NANOBIND`. Provides: + +| Shim name | pybind11 | nanobind | +|---|---|---| +| `namespace py` | `pybind11` | `nanobind` | +| `py_module` | `pybind11::module` | `nanobind::module_` | +| `OSL_PY_RW` | `def_readwrite` | `def_rw` | +| `OSL_PY_PROP_RO` | `def_property_readonly` | `def_prop_ro` | +| `OSL_PY_PROP_RW` | `def_property` | `def_prop_rw` | +| `osl_py::str(s)` | `py::str(s)` | `py::str(s.c_str(), s.size())` - nanobind's `str` has no `std::string` ctor | +| `osl_py::make_tuple(n, fn)` | `py::tuple t(n); t[i] = fn(i);` | `py::list` + `PyList_AsTuple` + `py::steal` | +| `osl_py::make_iterator(b, e)` | `py::make_iterator(b, e)` | `py::make_iterator(py::type(), "Iterator", b, e)` | +| `osl_py::throw_key_error(s)` | `throw py::key_error(s)` | `throw py::key_error(s.c_str())` | + +nanobind include set (opt-in, unlike `pybind11/stl.h`): +``, ``, ``, +``. **`stl/vector.h` is load-bearing** - it is what converts +`std::vector` for `.parameters` and `.metadata`; omitting it fails at +runtime, not at compile time. + +Needing no shim, because both frameworks spell them the same: `py::int_`, `py::float_`, +`py::str`, `py::none`, `py::object`, `py::tuple`, `py::index_error`, `py::class_`, +`py::init<...>`, `py::keep_alive<0,1>`, and the `_a` literal. + +`osl_py::make_iterator`'s scope argument must be a **bound** type - +`py::type()`. `py::type>()` would be null since that +type is never registered. + +### Modified: `src/liboslquery/py_osl.h` + +Replace the pybind11 include block, the `namespace py` alias, and the `PY_STR` define +with `#include "py_backend.h"`. (`PY_STR` disappears entirely: it was doing double duty +as a type in `PyTypeForCType` and as a constructor at call sites. The type uses become +plain `py::str`, valid in both backends; the call sites become `osl_py::str()`.) `PyTypeForCType<>` and `C_to_val_or_tuple` are unchanged. +`C_to_tuple` bodies switch to `osl_py::make_tuple`. `declare_oslquery()` takes +`py_module&`, not `py::module&`. + +### Modified: `src/liboslquery/py_osl.cpp` + +Mechanical substitution per the table above, plus: + +- Factor the eight `m.attr(...)` assignments into + `void declare_module_attributes(py_module& m)` so each arm of the module-entry `#if` + is three lines. +- The module entry point: + +```cpp +#if defined(OSL_PY_BACKEND_NANOBIND) +} // namespace PyOSL -- NB_MODULE must be at global scope, unlike PYBIND11_MODULE + +# if defined(OSL_PY_NANOBIND_ISOLATED_PACKAGE) +NB_MODULE(_oslquery, m) +# else +NB_MODULE(oslquery, m) +# endif +{ +# if PY_VERSION_HEX < 0x030a0000 + // Python 3.9's shutdown/refcounting order produces bogus nanobind leak + // warnings that do not occur on 3.10+. wjakob/nanobind#1405 + py::set_leak_warnings(false); +# endif + PyOSL::declare_module_attributes(m); + PyOSL::declare_oslqueryparam(m); + PyOSL::declare_oslquery(m); +} +#else +PYBIND11_MODULE(oslquery, m) +{ + declare_module_attributes(m); + declare_oslqueryparam(m); + declare_oslquery(m); +} +} // namespace PyOSL +#endif +``` + +**Invariant (FR-018, SC-005)**: `#if defined(OSL_PY_BACKEND_NANOBIND)` appears at +**one** site in `py_osl.cpp` (the module macro; the Python-version guard nested inside +it is not backend-conditional) and **zero** sites in `py_osl.h`. Every other difference +is absorbed by `py_backend.h`, which carries six. Any new conditional site outside the +shim needs a comment naming the framework difference that forces it. + +--- + +## Phase 2 - Build system + +### `src/cmake/pythonutils.cmake` + +**The option** - mirroring OIIO exactly. `set_cache` routes through +`set_utils.cmake:80 super_set` -> `set_from_env`, which is what makes an environment +variable of the same name work; CI depends on that (FR-003). + +```cmake +set_cache (OSL_PYTHON_BINDINGS_BACKEND "pybind11" + "Which Python binding backend(s) to build: pybind11, nanobind, or both" VERBOSE) +set_property (CACHE OSL_PYTHON_BINDINGS_BACKEND PROPERTY STRINGS pybind11 nanobind both) +string (TOLOWER "${OSL_PYTHON_BINDINGS_BACKEND}" OSL_PYTHON_BINDINGS_BACKEND) +if (NOT OSL_PYTHON_BINDINGS_BACKEND MATCHES "^(pybind11|nanobind|both)$") + message (FATAL_ERROR + "OSL_PYTHON_BINDINGS_BACKEND must be one of: pybind11, nanobind, both") +endif () +``` + +plus derived `OSL_BUILD_PYTHON_PYBIND11` / `OSL_BUILD_PYTHON_NANOBIND` booleans, which +are what the rest of the build tests. `CMakeLists.txt:209 include (pythonutils)` already +precedes `include (externalpackages)`, so no reordering is needed. + +**`find_python()`** - when `OSL_BUILD_PYTHON_NANOBIND`, add a second +`find_package (Python ${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR} EXACT REQUIRED COMPONENTS ...)`. +nanobind's CMake package wants the unversioned `Python::Module` target, not OSL's +`Python3::*`. + +**`discover_nanobind_cmake_dir()`** - copy OIIO's `pythonutils.cmake:112-145` verbatim. +It runs `python -m nanobind --cmake_dir` to find nanobind installed as a pip/brew Python +package (FR-006). Keep it a `function`, not a `macro`: a macro's early `return()` would +abort whatever file included pythonutils.cmake and called it at file scope. OIIO +documents this in a comment worth carrying over. + +**`setup_python_module_nanobind()`** - beside the existing `setup_python_module()`. Same +body except: +- `nanobind_add_module(${target} ${sources})` takes no `${PYLIB_LIB_TYPE}` argument. +- `target_compile_options (nanobind-static PUBLIC -Wno-format-nonliteral)` for + Clang/AppleClang/IntelLLVM - nanobind's own sources warn otherwise. +- Output and install directories per the layout table below. + +### `src/cmake/externalpackages.cmake:89-93` + +```cmake +find_python () +if (USE_PYTHON AND OSL_BUILD_PYTHON_PYBIND11) + checked_find_package (pybind11 REQUIRED VERSION_MIN 2.7) +endif () +if (USE_PYTHON AND OSL_BUILD_PYTHON_NANOBIND) + discover_nanobind_cmake_dir() + checked_find_package (nanobind CONFIG REQUIRED VERSION_MIN 2.8.0 BUILD_LOCAL missing) +endif () +``` + +### New: `src/cmake/build_nanobind.cmake` + +Copy OIIO's verbatim. OSL already has the full +`build_dependency_with_cmake()` / `BUILD_LOCAL` / `_REFIND` machinery in +`src/cmake/dependency_utils.cmake` (lines 278-473, 605-800), and the same +`${PROJECT_NAME}_LOCAL_DEPS_ROOT` variable name, so it drops in unchanged. Preserve both +of OIIO's workarounds and their explanatory comments: + +1. Pre-clone the source and run `git submodule update --init --depth 1 -- ext/robin_map`; + nanobind's `CMakeLists.txt` hard-errors without that submodule, and + `build_dependency_with_cmake()`'s plain `git clone` does not init submodules. +2. `set (nanobind_DIR "${nanobind_LOCAL_INSTALL_DIR}/nanobind/cmake" CACHE PATH ... FORCE)`; + nanobind installs its package config to a subdirectory that generic prefix search + does not check. + +This is OSL's first use of `BUILD_LOCAL`. If it misbehaves, the fallback is a +`src/build-scripts/build_nanobind.bash` in the style of the existing `build_pybind11.bash`. + +### Layouts + +| Backend | Targets | Init name | Build tree | Install | +|---|---|---|---|---| +| `pybind11` | `pyoslquery` | `oslquery` | `lib/python/site-packages/oslquery.so` | `${PYTHON_SITE_DIR}/oslquery/` | +| `nanobind` | `pyoslquery` | `oslquery` | `lib/python/site-packages/oslquery.so` | `${PYTHON_SITE_DIR}/oslquery/` | +| `both` | `pyoslquery` + `pyoslquery_nanobind` | `oslquery` + `_oslquery` | pybind as above; nanobind at `lib/python/nanobind/oslquery/{__init__.py,_oslquery.so}` | pybind as above; nanobind at `${PYTHON_SITE_DIR}/nanobind/oslquery/` | + +Single-backend layouts are byte-identical to today's (FR-008, SC-001). The `both`-mode +install deliberately deviates from OIIO, which installs its nanobind `__init__.py` into +the same site directory as pybind11's, where the two collide; a `nanobind/` +subdirectory avoids that (FR-009, SC-010). Build-tree layout matches OIIO either way. + +### `src/liboslquery/CMakeLists.txt:42-50` + +Split into two conditional blocks over the *same* `file(GLOB py_*.cpp)` source list. The +nanobind block adds `-DOSL_PY_BACKEND_NANOBIND`, plus +`-DOSL_PY_NANOBIND_ISOLATED_PACKAGE` when the backend is `both`, and in `both` mode +`configure_file(nanobind/__init__.py ... COPYONLY)` into +`${CMAKE_BINARY_DIR}/lib/python/nanobind/oslquery/`. + +### New: `src/liboslquery/nanobind/__init__.py` + +`from ._oslquery import *`, plus the same Windows `add_dll_directory` block as the +existing `src/liboslquery/__init__.py`, plus OIIO's trick of appending +`Release`/`Debug`/`RelWithDebInfo`/`MinSizeRel` to `__path__` so the per-configuration +`.pyd` resolves on MSVC multi-config builds - handled in Python rather than by fighting +CMake's output-directory layout. + +--- + +## Phase 3 - Tests + +**The blocker**: CMake never sets `PYTHONPATH` for tests today. It is set only in +`Makefile:293`, for the `make test` target - which is why a bare `ctest` in the build +directory cannot run `python-oslquery` at all. Per-backend selection is entirely +`PYTHONPATH`-driven, so this must move into CMake (FR-023). + +### `src/cmake/testing.cmake` + +Add `osl_tests_pythonpath_env_entry(out_var prefix_dir)`, mirroring OIIO's +`testing.cmake:28`: emits one `PYTHONPATH=:$ENV{PYTHONPATH}` string, and on Windows +uses `` alone, because semicolon-separated values get split by CMake list +processing when used as a CTest `ENVIRONMENT` entry. + +Replace the registration at `testing.cmake:474`: + +```cmake +if (USE_PYTHON AND Python3_Development_FOUND AND NOT SANITIZE) + set (_py_testsrc "${CMAKE_SOURCE_DIR}/testsuite/python-oslquery") + osl_tests_pythonpath_env_entry (_pybind_pypath + "${CMAKE_BINARY_DIR}/lib/python/site-packages") + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + osl_tests_pythonpath_env_entry (_nb_pypath + "${CMAKE_BINARY_DIR}/lib/python/nanobind") + else () + set (_nb_pypath "${_pybind_pypath}") + endif () + set (_nb_suffix ".nanobind") + if (OSL_BUILD_PYTHON_PYBIND11) + add_one_testsuite ("python-oslquery" "${_py_testsrc}" + ENV TESTSHADE_OPT=0 "${_pybind_pypath}") + else () + set (_nb_suffix "") # nanobind-only: the test takes the plain name + endif () + if (OSL_BUILD_PYTHON_NANOBIND) + add_one_testsuite ("python-oslquery${_nb_suffix}" "${_py_testsrc}" + ENV TESTSHADE_OPT=0 "${_nb_pypath}") + endif () +endif () +``` + +The empty-suffix trick is OIIO's and satisfies FR-022: with a single backend the test +keeps its historical name regardless of which backend that is; with `both`, CTest shows +`python-oslquery` and `python-oslquery.nanobind`. Distinct names give distinct sandbox +directories automatically - `add_one_testsuite` derives `testdir` from `testname` +(`testing.cmake:37`) - while `OSL_TESTSUITE_SRC` still points at the one source +directory, so both variants share one `run.py` and one `ref/` (FR-019, FR-021). + +Calling `add_one_testsuite()` directly rather than going through `TESTSUITE()` is +deliberate: `TESTSUITE()` has no `ENV` pass-through, and it currently also generates a +`python-oslquery.rs_bitcode` variant of a test that executes no shader. Dropping that +halves the variant count before `both` doubles it. The `NOOPTIMIZE` and `NOOPTIX` marker +files in `testsuite/python-oslquery/` become vestigial; remove them and say so in the +commit. + +### Everything else in the testsuite is untouched + +`run.py`, `src/test_oslquery.py`, and `ref/out.txt` are shared verbatim. The test does +`import oslquery` with no backend awareness whatsoever (FR-020) - that is the whole +point of the design, and it is why `runtest.py` needs no changes either. + +`Makefile:293` keeps its `PYTHONPATH` prefix for interactive use, but the CTest +`ENVIRONMENT` property now takes precedence for these tests. Note that in the commit. + +--- + +## Phase 4 - CI and documentation + +**`.github/workflows/build-steps.yml`**: new `python_bindings_backend` input +(default `''`), exported as env `OSL_PYTHON_BINDINGS_BACKEND` alongside the existing +`PYBIND11_VERSION` / `PYTHON_VERSION` at lines 135-136. + +**`src/build-scripts/ci-build.bash`**: after the `USE_SIMD` block, + +```bash +if [[ -n "${OSL_PYTHON_BINDINGS_BACKEND:-}" ]] ; then + OSL_CMAKE_FLAGS="$OSL_CMAKE_FLAGS -DOSL_PYTHON_BINDINGS_BACKEND=${OSL_PYTHON_BINDINGS_BACKEND}" +fi +``` + +**`.github/workflows/ci.yml`**: forward the input at the ~5 job-level sites that already +forward `pybind11_ver` (lines 66, 150, 435, 515, 585), and set +`python_bindings_backend: both` on exactly three matrix entries - one recent Linux, one +macOS-arm, one Windows (SC-011). Leave the other ~18 `pybind11_ver` entries alone; they +continue to exercise the default. `analysis.yml` is untouched. + +**Dependency installation**: `install_homebrew_deps.bash` gains a conditional +`brew install nanobind`; `gh-installdeps.bash` and `gh-win-installdeps.bash` gain a +pinned `pip install`, following OIIO's `ci-requirements-nanobind.txt` sha256-pinning +pattern. The `BUILD_LOCAL missing` path covers anything these miss. + +**Stale CI exclusion**: `ci.yml:199-201, 212-214` exclude `python-oslquery` on two +ASWF-container jobs "until the ASWF container properly includes OIIO's python bindings". +Since 642ab36f the test no longer imports OpenImageIO. Verify and remove - as a separate +commit, since it is an independent fix that stands on its own. + +**Documentation**: +- `INSTALL.md:69-73` - nanobind as a conditional dependency with its minimum version + (Principle IV), and an `OSL_PYTHON_BINDINGS_BACKEND=pybind11|nanobind|both` entry in + the build-options section (FR-025). +- The `Parameter.type` interoperability note (FR-026): it returns an OIIO `TypeDesc` and + therefore needs OIIO's Python module, built with the same binding backend and a + compatible internals/ABI version, to have been imported; `type_name` is the + coupling-free alternative. Also as a comment at the binding site. +- `CHANGES.md` - **deferred to release preparation** at the maintainer's direction; + CHANGES.md is written as part of the release process rather than per-PR. +- `src/liboslquery/MIGRATION_STATUS.md` (FR-028) - a short version of OIIO's, carrying + the maintainer conventions: which macros to use, `py_module&` rather than + `py::module&`, keep `#if` sites minimal and commented, and "consumer-visible + differences: none intended - if you find one, it is a bug; add a regression test." + +--- + +## Risks and mitigations + +| # | Risk | Mitigation | Fallback | +|---|---|---|---| +| 1 | nanobind's list caster rejects or mishandles `std::vector` | Phase 0 removes the `reference_internal` policies that would conflict | Convert to a list explicitly in the lambda, or bind the vector as an opaque type | +| 2 | `make_iterator` scope handle resolves to null | Shim passes `py::type()`, a bound type | Implement `__iter__` as iteration over the already-converted list | +| 3 | First use of `BUILD_LOCAL` in OSL | Machinery is present and identical to OIIO's | `src/build-scripts/build_nanobind.bash`, matching `build_pybind11.bash` | +| 4 | Re-enabled `for p in q:` still crashes on macOS | Pre-existing condition, not a regression | Leave disabled, record it; the nanobind variant still exercises iteration if it is only the pybind11 path that fails | +| 5 | Windows `both` mode - two modules, per-config output dirs | Handled Python-side in `nanobind/__init__.py`, as OIIO does | Restrict `both` to non-Windows in CI and document it | + +## Verification + +Three full configure/build/test cycles from the repository root, with +`OpenImageIO_ROOT=~/code/oiio/oiio.lg/dist`: + +```bash +make OSL_PYTHON_BINDINGS_BACKEND=pybind11 && make test TEST=python-oslquery +make OSL_PYTHON_BINDINGS_BACKEND=nanobind && make test TEST=python-oslquery +make OSL_PYTHON_BINDINGS_BACKEND=both && make test TEST=python-oslquery +``` + +Expected: one test in the first two cases (named `python-oslquery` in both), two in the +third (`python-oslquery` and `python-oslquery.nanobind`), all diffing clean against the +single unmodified `testsuite/python-oslquery/ref/out.txt`. + +Then, by hand: + +- `oslquery.__file__` resolves to the expected module for each `PYTHONPATH`. +- `sorted(dir(oslquery.OSLQuery))` and `sorted(dir(oslquery.Parameter))` and the sorted + module attribute list compare equal across backends, with zero differences (SC-003). +- `p.type` raises the same error class under both backends when `OpenImageIO` has not + been imported (FR-015). +- `cmake --build build --target install` for each backend; the installed layout matches + the table above, and the `both` install leaves both `__init__.py` files intact + (SC-010). +- Configuring with `-DOSL_PYTHON_BINDINGS_BACKEND=garbage` fails immediately with a + message naming the three accepted values (SC-007). +- A pybind11 build succeeds with nanobind absent, and a nanobind build succeeds with + pybind11 absent (SC-008, SC-009). diff --git a/docs/dev/specs/003-nanobind-python-bindings/research.md b/docs/dev/specs/003-nanobind-python-bindings/research.md new file mode 100644 index 0000000000..cc36dcb058 --- /dev/null +++ b/docs/dev/specs/003-nanobind-python-bindings/research.md @@ -0,0 +1,377 @@ +# Research: nanobind Python bindings for OSL + +**Feature**: 003-nanobind-python-bindings | **Date**: 2026-08-02 + +This is the Phase 0 output. It records (a) what OSL's bindings actually consist of, +(b) how OpenImageIO implemented the same dual-backend arrangement, and (c) which of the +pybind11/nanobind differences OIIO hit are relevant to OSL. + +--- + +## 1. OSL's current binding surface (complete inventory) + +The bindings live in `src/liboslquery/`, not in a `src/python/` directory. + +| File | Lines | Role | +|---|---|---| +| `src/liboslquery/py_osl.h` | 158 | Python.h/pybind11 includes, `PY_STR`, C-to-Python conversion helpers | +| `src/liboslquery/py_osl.cpp` | 193 | The whole binding: `Parameter`, `OSLQuery`, `PYBIND11_MODULE(oslquery, m)` | +| `src/liboslquery/__init__.py` | 18 | Windows DLL-path workaround, `from .oslquery import *` | +| `src/liboslquery/CMakeLists.txt` | 51 | Lines 42-50 build the module via `setup_python_module()` | + +**Module name**: `oslquery`, hardcoded in `PYBIND11_MODULE`. Note that +`setup_python_module()` defines `PYMODULE_NAME` (`pythonutils.cmake:89-90`) but nothing +in the source ever references it. + +**Everything bound** (`py_osl.cpp`): + +- Module attributes: `osl_version`, `VERSION`, `VERSION_STRING`, `VERSION_MAJOR`, + `VERSION_MINOR`, `VERSION_PATCH`, `INTRO_STRING`, `__version__`. +- `Parameter`: `__init__()`, `__init__(Parameter)`, `name`, `type`, `type_name`, + `isoutput`, `varlenarray`, `isstruct`, `isclosure`, `value`, `spacename`, `fields`, + `structname`, `metadata`. +- `OSLQuery`: `__init__()`, `__init__(shadername, searchpath="")`, `open()`, + `open_bytecode()`, `shadertype()`, `shadername()`, `nparams`, `parameters`, + `metadata`, `__len__`, `__getitem__(int)`, `__getitem__(str)`, `__iter__`, + `geterror(clear_error=True)`. + +**pybind11 features actually used**: `py::class_`, `py::init<...>` and value-returning +`py::init(lambda)`, `def_readwrite`, `def_property`, `def_property_readonly`, +`return_value_policy::reference_internal`, `keep_alive<0,1>`, `make_iterator`, +`py::tuple`/`str`/`int_`/`float_`/`none`/`object`, `py::index_error`, `py::key_error`, +`pybind11::literals` (`_a`), and `pybind11/stl.h` auto-conversion of +`std::vector` and `std::string`. + +**Not used at all**: buffer protocol, numpy, custom type casters, `py::enum_`, +`py::implicitly_convertible`, operators, GIL scope objects, submodules, +`py::module::import`. This is why OSL's port is far smaller than OIIO's - OIIO hit +almost all of those, OSL hits none of them. + +**Dead code found while inventorying** (delete during the port): + +- `py_osl.h:54-55` - `python_array_code()` and `typedesc_from_python_array_code()` are + declared, defined nowhere in the repo, called nowhere. +- `py_osl.h:58-62` - `object_classname()` unused, and uses the `.cast()` + member-function syntax nanobind does not have. +- `py_osl.h:107-116` - the `C_to_tuple` specialization is never instantiated; + it is also the only `py::cast` of a `TypeDesc` in a conversion path. +- `py_osl.h:32-33` - `` and `` unused. +- `py_osl.cpp:7` - `` unused. + +**Finding: all five `reference_internal` policies are already no-ops.** Every lambda +they decorate returns *by value*: `return p.metadata;` deduces `std::vector`, +`return self.parameters();` likewise (the function returns a const ref, the lambda's +deduced return type strips it), and `return *p;` deduces `Parameter`. The policy +therefore never governs a reference to internal state. Removing them is a no-op under +pybind11 and removes the single riskiest nanobind interaction (see §4.1). + +**Finding: the factory-lambda constructor is avoidable.** +`src/include/OSL/oslquery.h:118` declares +`OSLQuery(string_view shadername, string_view searchpath = string_view())`, and +`std::string` converts implicitly to `string_view`. So +`py::init()` works and is portable to nanobind, +whereas the current value-returning `py::init(lambda)` is a pybind11-only form. + +--- + +## 2. Prior art in OSL: commit 642ab36f + +`642ab36f "python: migration of oslquery python bindings away from OpenImageIO types"` +touched four files and did the essential prep: + +- `src/include/OSL/oslquery.h:97-101` - added `std::string type_name() const` and + `void type_name(const std::string&)` to `OSLQuery::Parameter`. +- `src/liboslquery/oslquery.cpp` - trivial implementations (`return type.c_str();` / + `type = TypeDesc(typestring);`). +- `src/liboslquery/py_osl.cpp:29-32` - added the `type_name` property; **removed** the + forced `py::module oiio = py::module::import("OpenImageIO");` that used to sit at the + top of `PYBIND11_MODULE`. +- `testsuite/python-oslquery/src/test_oslquery.py` - switched `p.type` to `p.type_name`. + +Its commit message states that `Parameter.type` is "the one and only spot in which OSL's +OSLQuery python API *requires* use of anything provided by the OIIO python bindings", +and that mixing a nanobind OIIO with a pybind11 OSL "DOES NOT WORK" on main before that +PR. That is the problem this feature closes. + +**Side effect worth acting on**: `.github/workflows/ci.yml:199-201, 212-214` exclude +`python-oslquery` on two ASWF-container jobs with the comment "until the ASWF container +properly includes OIIO's python bindings". After 642ab36f the test no longer imports +OpenImageIO at all, so those exclusions are probably stale. Verify and remove separately. + +--- + +## 3. OpenImageIO's implementation (the reference) + +Source: `~/code/oiio/oiio.lg`. Relevant commits, in order: + +``` +409621b0f feat: Nanobind for python bindings (first steps -- pybind11 still working) (#5084) +6ea45b11c ci: Run both tests pybind11 and nanobind in CI (#5176) +667365f45 Unify pybind11 and nanobind into single-source bindings (#5254) +1cb1da122 feat(python): Expand dual-backend support in Python bindings (#5310) +62d139599 python: nanobind tidying, auto-build, CI testing +``` + +**Important trajectory lesson**: #5084 created `src/python-nanobind/` as a *full +duplicate* of every binding source file. #5254 then deleted all of those duplicates and +made both backends compile from the single `src/python/` tree via a compatibility +header. OSL should skip straight to the end state; there is no reason to repeat the +duplicate-then-unify detour on a 350-line binding. + +Today `src/python-nanobind/` contains only `CMakeLists.txt` and `__init__.py` - it +exists purely to build a second, isolated module when the backend is `both`. + +### 3.1 The option + +`src/cmake/pythonutils.cmake:6-35`. Name `OIIO_PYTHON_BINDINGS_BACKEND`, default +`"pybind11"`, values `pybind11|nanobind|both`, lowercased then validated with +`FATAL_ERROR`. Declared with `set_cache(...)`, which routes through +`set_utils.cmake:80 super_set` -> `set_from_env`, meaning **an environment variable of +the same name sets it** - that is exactly how CI drives it +(`ci-startup.bash:56-57 export OIIO_PYTHON_BINDINGS_BACKEND=both`). Two derived +booleans, `OIIO_BUILD_PYTHON_PYBIND11` and `OIIO_BUILD_PYTHON_NANOBIND`, are what the +rest of the build tests. + +Include order matters: `include (pythonutils)` precedes `include (externalpackages)` so +the derived booleans exist when dependency resolution runs. OSL already has this order +(`CMakeLists.txt:209`). + +### 3.2 Dependency discovery + +`src/cmake/externalpackages.cmake:116-130` - both finds are guarded by the derived +booleans; nanobind is `checked_find_package (nanobind CONFIG REQUIRED VERSION_MIN 2.8.0 BUILD_LOCAL missing)`. + +`discover_nanobind_cmake_dir()` (`pythonutils.cmake:112-145`) handles nanobind installed +as a pip/brew *Python package* by running `python -m nanobind --cmake_dir` and setting +`nanobind_DIR` from the result. It is deliberately a `function`, not a `macro`, with +this comment: a macro's early `return()` would abort whatever file included +pythonutils.cmake and called it at file scope. + +`find_python()` (`pythonutils.cmake:78-86`) does a *second* +`find_package (Python . EXACT REQUIRED COMPONENTS ...)` when nanobind is +enabled, because nanobind's CMake package expects the unversioned `Python::Module` +targets, not the versioned `Python3::*` ones. + +### 3.3 Local build of nanobind + +`src/cmake/build_nanobind.cmake`. Two non-obvious workarounds, both of which OSL will +need verbatim: + +1. nanobind vendors `tsl::robin_map` as a git submodule and its `CMakeLists.txt` + hard-errors if it is not checked out; `build_dependency_with_cmake()`'s plain + `git clone` does not init submodules. So the file clones the source itself and runs + `git submodule update --init --depth 1 -- ext/robin_map` before delegating. +2. nanobind installs its CMake package config to `/nanobind/cmake`, a layout + generic prefix search does not check, so `nanobind_DIR` is set explicitly with + `FORCE`. + +Pinned at `nanobind_BUILD_VERSION 2.13.0`, with a `nanobind_GIT_COMMIT` hash verified +against the tag. Configured with `-D NB_TEST=OFF`; there is effectively nothing to +compile, the "build" just copies headers/sources/CMake helpers to a prefix. + +Related: OIIO switched robin-map to `checked_find_package (Robinmap CONFIG ... NAMES tsl-robin-map ...)` +specifically so nanobind's own CMake reuses that `tsl::robin_map` target instead of its +private vendored copy. + +### 3.4 Module targets and layout + +`src/python/CMakeLists.txt` builds the pybind11 module from `file(GLOB *.cpp)`. In +nanobind-**only** mode it builds the same files again through +`setup_python_module_nanobind()` with `-DOIIO_PY_BACKEND_NANOBIND`. In **both** mode it +skips that and `src/python-nanobind/CMakeLists.txt` builds them instead, referencing the +sources by explicit `../python/py_*.cpp` paths, adding `-DOIIO_PY_NANOBIND_ISOLATED_PACKAGE`. + +So: **the same .cpp files, two CMake targets, two sets of object files, different +`-D` flags.** + +| Backend | Targets | Init name | Build tree | Install | +|---|---|---|---|---| +| pybind11 | `PyOpenImageIO` | `OpenImageIO` | `lib/python/site-packages` | `${PYTHON_SITE_DIR}` | +| nanobind | `PyOpenImageIO` | `OpenImageIO` | `lib/python/site-packages` | `${PYTHON_SITE_DIR}` (drop-in) | +| both | `PyOpenImageIO` + `PyOpenImageIONanobind` | `OpenImageIO` + `_OpenImageIO` | pybind in `lib/python/site-packages`; nanobind in `lib/python/nanobind/OpenImageIO/` | both under `${PYTHON_SITE_DIR}` | + +`setup_python_module_nanobind()` (`pythonutils.cmake:230+`) mirrors the pybind11 macro +except that `nanobind_add_module()` takes no `MODULE`/`SHARED` type argument, and it +applies `target_compile_options (nanobind-static PUBLIC -Wno-format-nonliteral)` for +Clang - nanobind's own sources warn otherwise. + +**Deviation OSL should make**: in `both` mode OIIO installs its nanobind `__init__.py` +to the same `${PYTHON_SITE_DIR}` as pybind11's. The extension modules do not collide +(`OpenImageIO.so` vs `_OpenImageIO.so`) but the two `__init__.py` files do. OSL should +install the nanobind package under a `nanobind/` subdirectory of the site dir instead. + +### 3.5 The compatibility header + +`src/python/py_backend.h` (4.6K). Selects on `OIIO_PY_BACKEND_NANOBIND`. Provides a +`namespace py` alias, a `py_module` typedef, a set of `OIIO_PY_*` macros for the binding +verbs, and a `namespace oiio_py` of small inline helpers. + +Full mapping table (OIIO names; OSL will use `OSL_PY_*` / `osl_py`): + +| Shim name | pybind11 | nanobind | +|---|---|---| +| `namespace py` | `pybind11` | `nanobind` | +| `py_module` | `pybind11::module` | `nanobind::module_` | +| `OIIO_PY_RW` | `def_readwrite` | `def_rw` | +| `OIIO_PY_RO` | `def_readonly` | `def_ro` | +| `OIIO_PY_PROP_RO` | `def_property_readonly` | `def_prop_ro` | +| `OIIO_PY_PROP_RW` | `def_property` | `def_prop_rw` | +| `OIIO_PY_PROP_RW_NONE` | `def_property` | `def_prop_rw(..., py::for_setter(py::arg().none()))` | +| `OIIO_PY_RO_STATIC` | `def_property_readonly_static` | `def_prop_ro_static` | +| `oiio_py::ref` | `return_value_policy::reference` | `rv_policy::reference` | +| `oiio_py::ref_internal` | `return_value_policy::reference_internal` | `rv_policy::reference_internal` | +| `oiio_py::str(x)` | `py::str(x)` | `std::string(x)` | +| `oiio_py::str_to_stdstring(h)` | `std::string(cast(h))` | `std::string(cast(h).c_str())` | +| `oiio_py::bytes_to_stdstring(b)` | `std::string(b)` | `std::string(b.c_str(), b.size())` | +| `oiio_py::throw_key_error(s)` | `py::key_error(std::string)` | `py::key_error(const char*)` | +| `oiio_py::make_tuple(n, fn)` | `py::tuple(n)` + indexed assign | `py::list` + `PyList_AsTuple` + `py::steal` | +| `oiio_py::make_iterator(c)` | `py::make_iterator(b, e)` | `py::make_iterator(py::type(), "iterator", b, e)` | +| `oiio_py::return_object(o)` | identity | `py::borrow(o)` | +| `oiio_py::make_numpy_array(p,n)` | `py::array_t` | `py::ndarray>` + `rv_policy::move` | +| `PY_STR(x)` | `py::str` | `oiio_py::str(x)` | + +nanobind's stl casters are **opt-in headers**, unlike `pybind11/stl.h`. OIIO includes +`` plus +``. + +`declare_*()` free functions take `py_module&`, never `py::module&`. + +### 3.6 How much `#if` survived in OIIO + +Across 13 binding `.cpp` files: + +``` +py_oiio.cpp 6 +py_imagebufalgo.cpp 2 +py_imagebuf.cpp 1 +py_paramvalue.cpp 1 +py_typedesc.cpp 1 +everything else 0 +``` + +That is the bar: nine conditional sites across ~15k lines of bindings. OSL's target is +at most three across 350 lines, and realistically two. + +### 3.7 Test registration + +The test scripts contain **zero** backend awareness - they just +`import OpenImageIO as oiio`. Selection is entirely `PYTHONPATH`. + +`src/cmake/testing.cmake:28` defines `oiio_tests_pythonpath_env_entry()`, which builds +one `PYTHONPATH=:$ENV{PYTHONPATH}` string, and on Windows uses `` alone +because semicolon-separated values get split by CMake list processing when used as a +CTest `ENVIRONMENT` entry. + +The registration block (`testing.cmake:246-335`) uses this trick: + +```cmake +set (nanobind_python_test_suffix ".nanobind") +if (OIIO_BUILD_PYTHON_PYBIND11) + oiio_add_tests ( ENVIRONMENT "${_pybind_tests_pythonpath}") +else () + set (nanobind_python_test_suffix "") # nanobind-only: take the plain names +endif () +if (OIIO_BUILD_PYTHON_NANOBIND) + oiio_add_tests ( SUFFIX ${nanobind_python_test_suffix} + ENVIRONMENT "${_nanobind_tests_pythonpath}") +endif () +``` + +The suffix also names a distinct build-tree run directory while `OIIO_TESTSUITE_SRC` +still points at the single source directory - so both variants share one `run.py` and +one `ref/` and run in isolated scratch dirs. `testsuite/runtest.py` has no backend +awareness whatsoever. + +The `both`-mode `__init__.py` (`src/python-nanobind/__init__.py`) does +`from ._OpenImageIO import *`, and on Windows appends `Release`/`Debug`/`RelWithDebInfo`/`MinSizeRel` +to `__path__` so the per-configuration `.pyd` location resolves - handled in Python +rather than by fighting CMake's multi-config output layout. + +### 3.8 Docs and CI in OIIO + +- `INSTALL.md:47-50` conditional nanobind dependency; `:166-170` the option; `:262` a + make-wrapper row. +- `CHANGES.md` three entries (one per PR). +- `src/python/MIGRATION_STATUS.md` - the living maintainer doc. Its "Conventions" section + is the checklist worth copying, and it states: "**Consumer-visible differences: None + intended.** ... If you find a behavioral difference, treat it as a bug and add a + regression test." +- CI: a `oiio_python_bindings_backend` workflow input (`build-steps.yml:96-98`) exported + as an env var (`:159`), forwarded from `ci.yml` at three job sites, set to `both` on + one linux, one macos14-arm, and one windows-2025 matrix entry. + `ci-build.bash:28-30` turns the env var into a `-D` flag. + `install_homebrew_deps.bash:60-61` brew-installs nanobind conditionally; + `ci-requirements-nanobind.txt` pip-pins it with a sha256 hash for Linux/Windows. +- `pyproject.toml` still requires only pybind11 - wheels remain pybind11-only. Stub + generation is likewise still pybind11-driven, with the same `.pyi` installed for both. + Both are out of scope for OSL (it has neither). + +--- + +## 4. pybind11/nanobind differences: which ones OSL actually hits + +OIIO's tree documents about twenty. Sorted by relevance to OSL: + +### 4.1 Hit by OSL + +| # | Difference | OSL's exposure | Resolution | +|---|---|---|---| +| 1 | `PYBIND11_MODULE` can live inside a namespace; `NB_MODULE` must be at global scope | `py_osl.cpp:9,176,193` wraps the macro in `namespace PyOSL` | Close the namespace before the nanobind arm. One of the two permitted `#if` sites. | +| 2 | No value-returning `py::init(lambda)` in nanobind; it needs a placement-new `__init__` | `py_osl.cpp:98-102` | **Avoid entirely** - use `py::init()`, valid in both. See §1. | +| 3 | `make_iterator` signature: nanobind needs a type handle and a name | `py_osl.cpp:158-164` | Shim helper. The scope handle must be a *bound* type - `py::type()`, not `py::type>()` which would be null. | +| 4 | `py::tuple` cannot be built by indexed assignment in nanobind | `py_osl.h:88-103, 121-128` | `osl_py::make_tuple(n, fn)` shim building a `py::list` then `PyList_AsTuple` + `py::steal`. | +| 5 | `py::key_error` takes `const char*` in nanobind, `std::string` in pybind11 | `py_osl.cpp:153` | `osl_py::throw_key_error(std::string)` shim. | +| 6 | stl casters are opt-in headers in nanobind | `pybind11/stl.h` at `py_osl.h:35` is what converts `std::vector` and `std::string` | Include `` and `` in the shim. **Load-bearing** - omit and `.parameters` silently fails to convert. | +| 7 | nanobind's `py::str` has no `std::string` constructor (pybind11's does) | `PY_STR(p.name.string())` and friends | `osl_py::str()` shim: `py::str(s)` vs `py::str(s.c_str(), s.size())`. **Do not "fix" this by passing `ustring::c_str()`** - see #7b. | +| 7b | `ustring::c_str()` returns `nullptr` for an empty or default-constructed ustring; `ustring::string()` is null-safe | `Parameter::structname` on any non-struct param, and any empty `ustring` in `sdefault`/`spacename`/`fields` | Found the hard way: routing `PY_STR` through `.c_str()` segfaults inside `PyUnicode_FromString`. The existing testsuite does **not** catch it - `test_oslquery.py` only prints `structname` inside the `isstruct` branch. The shim's `str()` takes `const std::string&` and its `const char*` overload maps null to `""`. | +| 8 | Python 3.9 emits spurious nanobind leak warnings at interpreter shutdown | any 3.9 build | `py::set_leak_warnings(false)` guarded on `PY_VERSION_HEX < 0x030a0000`. The second permitted `#if` site. (wjakob/nanobind#1405) | +| 9 | `.cast()` member function does not exist in nanobind; only free `py::cast(obj)` | `py_osl.h:61` `object_classname()` | Dead code - delete it. | +| 10 | `reference_internal` interacting with stl container casters | five sites in `py_osl.cpp` | All five are already no-ops (§1). Delete them in Phase 0, before nanobind exists. | +| 11 | nanobind's CMake wants unversioned `Python::` targets | `find_python()` uses `Python3::*` | Second `find_package(Python ... EXACT REQUIRED)`. | +| 12 | Clang warns inside nanobind's own sources | any Clang/AppleClang build | `target_compile_options (nanobind-static PUBLIC -Wno-format-nonliteral)`. | +| 13 | nanobind's git submodule + non-standard CMake config install dir | local builds | Both handled in the copied `build_nanobind.cmake`. | +| 14 | MSVC multi-config puts the extension in a per-config subdir | Windows `both` mode | Handled Python-side in the `both`-mode `__init__.py` by appending config names to `__path__`. | +| 15 | pybind11 3.x adds a `_pybind11_conduit_v1_` member to every bound class (its cross-extension interop hook); nanobind has no equivalent | every bound class | **The only public-surface difference between the two modules.** It is a framework-internal artifact, not part of OSL's API, and nothing can or should be done about it. SC-003 excludes it explicitly. | +| 15b | `make_iterator` is overloaded on (first, last) *and* on a container (`Type& value, Extra&&...`) in both frameworks. pybind11 **2.10 only** took the pair overload by forwarding reference, so lvalue iterators deduce to `It&` and tie exactly with `Type&` -> ambiguous | Only shows up when the call is wrapped in a helper that names its parameters, as `osl_py::make_iterator` does; direct `.begin()`/`.end()` calls pass prvalues, which can't bind `Type&`. Fixed by `std::move`-ing into the call. Verified by compiling `py_osl.cpp` against pybind11 2.7.0, 2.9.0, 2.10.0, 2.11.1, 3.0.1 and master. | +| 16 | `from .X import *` in a package `__init__.py` skips underscore-prefixed names | `__version__` | Not a backend difference at all - a **pre-existing OSL bug** the equivalence harness surfaced. `oslquery.__version__` worked when importing the extension module straight out of the build tree, but was missing from every *installed* OSL, because the installed package wraps it in an `__init__.py` doing `import *`. Both `__init__.py` files now re-export it explicitly. | + +### 4.2 Not applicable to OSL + +`export_values()` on enums (OSL binds no enums); buffer protocol and the absence of +`nb::buffer` (no buffers); numpy array construction (no numpy); `half` having no +nanobind dtype (no `half` in any bound signature - `PyTypeForCType` exists but is +never instantiated); assigning `None` to a property (no nullable properties); +`py::object` needing `py::borrow` when returned from a lambda (OSL's `py::object` +returns come from `C_to_val_or_tuple`, which constructs fresh objects); `gil_scoped_release` +(none used); `py::implicitly_convertible` (none used - and OIIO confirms nanobind +supports it anyway, so it was never a blocker). + +--- + +## 5. Decisions + +| Decision | Rationale | Alternatives rejected | +|---|---|---| +| Default backend `pybind11` | Zero disruption for existing builders and packagers; nanobind opt-in; mirrors OIIO | `both` - forces a nanobind dependency on everyone and doubles python build time. `nanobind` - makes any nanobind-only bug an immediate build break on day one. | +| Keep `Parameter.type` unchanged in both backends, no `#if` | The failure mode (needs OIIO's Python module, matching backend, compatible internals version) is pre-existing and identical under pybind11 today. Both frameworks resolve unregistered types at runtime, not compile time, so it compiles either way. | Removing it - a gratuitous breaking change. Guarding it pybind11-only - creates a real consumer-visible API difference between backends, violating the invariant this whole design rests on. | +| Skip OIIO's duplicate-then-unify detour | OIIO ended up deleting every duplicated file; on 350 lines the intermediate state has no value | Copying `src/python-nanobind/` wholesale as OIIO first did. | +| Call `add_one_testsuite()` directly for the python test instead of extending `TESTSUITE()` | `TESTSUITE()` has no `ENV` pass-through, and it currently also generates a `python-oslquery.rs_bitcode` variant of a test that executes no shader - pointless work that `both` mode would double | Adding an `ENV` multi-value argument to `TESTSUITE()` and threading it through its ~10 `add_one_testsuite` calls. | +| Set `PYTHONPATH` from CMake | It is set today only in `Makefile:293`, so bare `ctest` cannot run the python test at all, and per-backend selection is impossible without it | Continuing to rely on the Makefile - cannot express two different paths for two test variants. | +| Install `both`-mode nanobind package under a `nanobind/` subdir of the site dir | Avoids the `__init__.py` collision present in OIIO's `both` install | Copying OIIO exactly. | +| Use the existing `BUILD_LOCAL` machinery for nanobind | Already present in `src/cmake/dependency_utils.cmake` (lines 278-473, 605-800) with the same `${PROJECT_NAME}_LOCAL_DEPS_ROOT` variable OIIO uses; `build_nanobind.cmake` drops in unchanged | A `build_nanobind.bash` alongside `build_pybind11.bash` - kept as the fallback if `BUILD_LOCAL`, which OSL has never exercised, misbehaves. | + +--- + +## 6. Open risks + +1. **nanobind + `std::vector` through the list caster.** Mitigated by removing + the `reference_internal` policies first. Fallback: convert to a list explicitly in + the lambda, or bind the vector as an opaque type. +2. **`make_iterator` scope handle.** Must be a bound type. Fallback: implement `__iter__` + as iteration over the already-converted list. +3. **First use of `BUILD_LOCAL` in OSL.** Fallback: a `build_nanobind.bash` script. +4. **Re-enabling the `for p in q:` loop** in `test_oslquery.py`, disabled since ~2020 by + a `FIXME(pybind11)` about a macOS crash with pybind11 2.6 + Python 3.8/3.9. If it + still crashes, leave it disabled and record it - it is a pre-existing condition, and + the nanobind variant can still exercise iteration. +5. **Windows `both` mode** - two extension modules plus multi-config output directories. diff --git a/docs/dev/specs/003-nanobind-python-bindings/spec.md b/docs/dev/specs/003-nanobind-python-bindings/spec.md new file mode 100644 index 0000000000..4949086235 --- /dev/null +++ b/docs/dev/specs/003-nanobind-python-bindings/spec.md @@ -0,0 +1,363 @@ +# Feature Specification: nanobind Python bindings (dual-backend) + +**Feature Branch**: `lg-nanobind` + +**Created**: 2026-08-02 + +**Status**: Draft + +**Input**: User description: "For OSL's python bindings, I want to switch from pybind11 +to nanobind. Look at OpenImageIO (~/code/oiio/oiio.lg), which recently added nanobind +bindings. Salient features: (1) for now both bindings are built, controlled by a +build-time switch that selects pybind11, nanobind, or both; (2) all tests run for either +binding (including both, when 'both' is selected); (3) minimize the amount of code +duplicated for the two bindings (sometimes by using macros that are defined differently +for both). Devise a plan for a similar binding conversion for OSL. It should be a much +smaller task (since it's only the OSLQuery class that we make python bindings for), but +I want it in an analogous style and using a similar approach." + +## Overview + +OSL exposes exactly one class family to Python: `OSLQuery` and `OSLQuery::Parameter`, +bound with pybind11 in `src/liboslquery/py_osl.{h,cpp}` (~350 lines, ~120 of them live +code). OpenImageIO has migrated to nanobind while keeping pybind11 available behind a +build-time selector, so that both modules can be built from a single set of sources and +both can be tested. OSL should adopt the same structure. + +This matters beyond OSL's own modernization: OSL's `Parameter.type` returns an OIIO +`TypeDesc`, whose Python binding is registered by *OIIO's* module in a process-wide +type registry that is shared only among extension modules using the same binding +framework and a compatible internals/ABI version. Once OIIO's Python module is built +with nanobind, an OSL module built with pybind11 can no longer see that registration. +Supporting both backends in OSL lets packagers match whichever OIIO they ship. + +Commit 642ab36f already removed the hard dependency: `Parameter.type_name` returns a +plain string covering every use `type` was needed for, and the forced +`import OpenImageIO` at module init is gone. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Build OSL exactly as before (Priority: P1) + +An existing OSL builder, packager, or CI job configures and builds OSL with no new +flags. Everything about the Python module - its name, contents, behavior, build-tree +location, install location, and test name - is unchanged from today. + +**Why this priority**: Non-negotiable. This change must be invisible by default. If the +default path regresses, nothing else in the feature matters. + +**Independent Test**: Configure and build with no new options; run the `python-oslquery` +test. It must pass against the unchanged reference output, and the built module must +land in the same paths with the same file names as before the change. + +**Acceptance Scenarios**: + +1. **Given** a checkout with no new CMake variables set, **When** OSL is configured and + built, **Then** exactly one Python module is produced, at the same build-tree and + install paths as before, and no new dependency is required. +2. **Given** that default build, **When** the test suite runs, **Then** a single test + named `python-oslquery` runs and diffs clean against the existing reference output. +3. **Given** that default build, **When** a user imports the module and exercises + `OSLQuery`, **Then** every attribute and method available before the change is still + available with identical behavior. + +--- + +### User Story 2 - Build the nanobind module instead (Priority: P1) + +A builder who has moved to a nanobind-based OpenImageIO selects the nanobind backend. +They get a drop-in replacement module: same import name, same public API, same install +location, same test name and reference output. + +**Why this priority**: This is the actual goal of the feature. Without it the work +delivers nothing. + +**Independent Test**: Configure with the backend set to nanobind, build, and run the +test suite. The same single test name appears and passes against the same reference +output; pybind11 is not required for the build. + +**Acceptance Scenarios**: + +1. **Given** the nanobind backend is selected, **When** OSL is configured, **Then** + pybind11 is not searched for and is not required. +2. **Given** the nanobind backend is selected, **When** OSL is built and installed, + **Then** the Python module has the same import name and occupies the same install + location as the pybind11 module would have. +3. **Given** a nanobind build, **When** the same test script that runs against the + pybind11 module is run, **Then** its output is identical. +4. **Given** a nanobind build, **When** a user inspects the module's public surface, + **Then** it matches the pybind11 module's public surface exactly. + +--- + +### User Story 3 - Build and test both backends at once (Priority: P2) + +A maintainer or CI job selects "both". Two Python modules are produced, they do not +collide in the build tree or on install, and the full Python test suite runs twice - +once against each - from a single copy of the test sources and a single reference +output. + +**Why this priority**: This is how the equivalence claimed by Stories 1 and 2 is +actually enforced. It is P2 only because Stories 1 and 2 are individually shippable +without it. + +**Independent Test**: Configure with "both", build, and list the registered tests. Two +Python test entries appear, distinguished by a suffix, and both pass. + +**Acceptance Scenarios**: + +1. **Given** the "both" backend selection, **When** OSL is built, **Then** two Python + extension modules are produced in locations that do not overwrite each other. +2. **Given** the "both" backend selection, **When** the test suite runs, **Then** the + Python test appears twice - once under its original name and once under a + nanobind-marked name - and both diff clean against the same single reference output. +3. **Given** the "both" backend selection, **When** each module is imported in + isolation, **Then** each is importable under the project's normal Python import name + without the other being present. +4. **Given** the "both" backend selection, **When** OSL is installed, **Then** neither + backend's package files overwrite the other's. + +--- + +### User Story 4 - Maintain the bindings without writing them twice (Priority: P2) + +A maintainer adds or changes a binding. They edit one place, and both backends pick up +the change. + +**Why this priority**: This is the sustainability requirement that makes carrying two +backends acceptable at all. Without it, the dual-backend period becomes a permanent +tax. + +**Independent Test**: Count the binding source files and the backend-conditional +compilation sites. There must be exactly one set of binding sources, and the number of +backend-conditional sites must be countable on one hand and each justified by a genuine +framework difference. + +**Acceptance Scenarios**: + +1. **Given** the completed feature, **When** the binding sources are inspected, **Then** + there is exactly one copy of the binding code, compiled once per selected backend. +2. **Given** a new attribute added to the bound class in the single source, **When** + "both" is built, **Then** the attribute appears in both modules with no + backend-specific code. +3. **Given** the completed feature, **When** backend-conditional regions are counted, + **Then** there are at most three, each with a comment naming the framework + difference that forces it. + +--- + +### User Story 5 - Understand the OIIO interoperability constraint (Priority: P3) + +A user hits an error accessing `Parameter.type` and finds documentation explaining why, +and what to use instead. + +**Why this priority**: Documentation-only, and the failure mode is pre-existing rather +than introduced here - but the feature adds one more way to trigger it, so it should be +written down now. + +**Independent Test**: Read the documentation and confirm it states the constraint and +names the alternative. + +**Acceptance Scenarios**: + +1. **Given** the documentation, **When** a user searches for the type-related attribute, + **Then** they find a statement that it requires OIIO's Python module, built with the + same binding backend, to have been imported first. +2. **Given** that same documentation, **When** the user looks for an alternative, + **Then** the string-valued attribute is named as the coupling-free option. + +--- + +### Edge Cases + +- **Invalid backend selection**: an unrecognized value must fail at configure time with + a message listing the accepted values, not fail later at compile or link time. +- **Case variation**: `Both`, `NANOBIND`, etc. must be accepted, matching how the rest + of the project's string-valued options behave. +- **nanobind not installed**: when the nanobind backend is requested and nanobind is not + present, the build must either locate it automatically (including when it was + installed as a Python package rather than a system package) or build it locally, and + must say clearly which it did. +- **Python bindings disabled entirely**: with Python support turned off, the backend + selection must have no effect and must not cause a dependency search. +- **Sanitizer builds**: the Python test is already skipped under sanitizers; that must + remain true for every backend selection. +- **Both backends installed into the same prefix**: package initialization files for the + two backends must not overwrite one another. +- **Accessing the OIIO-typed attribute with no OIIO Python module imported**: must + behave the same way under both backends - a Python-level error, not a crash. +- **Multi-configuration builds on Windows**: the "both" mode's second module must remain + importable even though its binary lands in a per-configuration subdirectory. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Backend selection** + +- **FR-001**: The build MUST provide a single user-facing option that selects the Python + binding backend, accepting exactly three values: pybind11, nanobind, or both. +- **FR-002**: When the option is unset, the build MUST auto-select nanobind when + OpenImageIO is >= 3.2 (which defaults its own Python bindings to nanobind - matching + it keeps `Parameter.type` working) and Python is >= 3.10 (nanobind's minimum), and + pybind11 otherwise. A missing nanobind is built locally by the normal dependency + machinery, so the auto-selection does not depend on nanobind being pre-installed. + Any backend produces a Python module with the same public surface, return values, + and exception types; only the binding framework differs. (Updated 2026-09-09: the + feature originally shipped with a fixed pybind11 default; after soak time this + became the conditional auto-select.) +- **FR-003**: The option MUST be settable from the environment as well as from the + command line, matching the convention used by the project's other cached string + options, so continuous integration can drive it without editing build scripts. +- **FR-004**: The option MUST be case-insensitive and MUST reject any other value at + configure time with an error naming the accepted values. +- **FR-005**: The build MUST search for pybind11 only when pybind11 is among the + selected backends, and for nanobind only when nanobind is among the selected backends. +- **FR-006**: When nanobind is selected and is not already discoverable, the build MUST + attempt discovery via the active Python environment before falling back to building + it locally, and MUST report which path it took. +- **FR-007**: When Python bindings are disabled, the backend option MUST have no effect + and MUST trigger no dependency search. + +**Module identity and layout** + +- **FR-008**: When exactly one backend is selected, the resulting module MUST use the + project's established Python import name and MUST occupy the same build-tree and + install locations that the pybind11 module occupies today. +- **FR-009**: When both backends are selected, the two modules MUST be placed so that + neither overwrites the other in the build tree or on install, including their package + initialization files. +- **FR-010**: When both backends are selected, each module MUST still be importable + under the project's established import name, by putting the appropriate directory on + the Python module search path. + +**API equivalence** + +- **FR-011**: The nanobind module MUST expose the identical public surface as the + pybind11 module: the same module-level attributes, the same classes, and the same + attributes and methods on each class, with the same names. Members injected by the + binding framework itself, which are not part of OSL's API, are excluded. +- **FR-012**: Every bound attribute and method MUST return values of the same Python + types, with the same values, under both backends. +- **FR-013**: Both modules MUST support iteration over a query object's parameters, + index-based and name-based item lookup, and length. +- **FR-014**: Out-of-range index lookup and unknown-name lookup MUST raise the same + Python exception types under both backends. +- **FR-015**: The attribute that exposes an OpenImageIO type object MUST be present in + both backends' modules and MUST fail in the same manner - a Python-level error, not a + crash - when OIIO's Python module of the matching backend has not been imported. + +**Single-source maintenance** + +- **FR-016**: There MUST be exactly one copy of the binding source; the same sources MUST + be compiled once per selected backend rather than duplicated per backend. +- **FR-017**: Framework differences MUST be absorbed by a single compatibility header + that defines the same names differently per backend. +- **FR-018**: Backend-conditional compilation outside that compatibility header MUST be + limited to at most three sites, each accompanied by a comment naming the framework + difference that requires it. + +**Testing** + +- **FR-019**: The Python test MUST run against every selected backend, using one shared + copy of the test script and one shared reference output. +- **FR-020**: The test script MUST contain no backend-specific logic; backend selection + MUST be entirely a matter of which directory is on the Python module search path. +- **FR-021**: When both backends are selected, the two test runs MUST be registered + under distinct names and MUST execute in distinct working directories. +- **FR-022**: When only one backend is selected, its test MUST be registered under the + test's original name, regardless of which backend it is. +- **FR-023**: The build MUST configure the Python module search path for these tests + itself, so the tests pass when run directly by the test driver rather than only + through the project's convenience wrapper. +- **FR-024**: Iteration over a query object's parameters MUST be exercised by the test + suite under every selected backend. + +**Documentation and continuous integration** + +- **FR-025**: Installation documentation MUST list nanobind as a conditional dependency + and MUST document the backend option and its accepted values. +- **FR-026**: Documentation MUST state the OpenImageIO type-object interoperability + constraint and name the string-valued alternative. +- **FR-027**: Continuous integration MUST exercise the both-backends configuration on at + least one job per major platform, while the remaining jobs continue to exercise the + default. +- **FR-028**: A maintainer-facing note MUST record the conventions for keeping the two + backends in sync, including the principle that any consumer-visible difference between + the backends is a bug requiring a regression test. + +### Key Entities + +- **Binding backend selection**: a build-time choice among pybind11, nanobind, and both; + drives dependency discovery, which module targets are built, where they are placed, + and which test variants are registered. +- **Compatibility layer**: the single header that maps one set of binding spellings onto + either framework, so the binding source itself is backend-neutral. +- **Python module**: the extension module exposing the shader-query API; identified by + its import name, its location on the module search path, and its public surface. +- **Test variant**: one registered execution of the shared Python test script against + one backend, distinguished by name, working directory, and module search path. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: With the default configuration, the Python module's public surface, its + file names, and its build-tree and install locations are identical to those produced + before this change - verified by direct comparison, with zero differences. +- **SC-002**: The Python test's reference output is unchanged, and all backend + configurations diff clean against that same single reference file. +- **SC-003**: The full public surface of the two modules - module attributes, class + names, and per-class attributes and methods - compares equal, with zero differences, + excluding members the binding framework injects on its own behalf (pybind11 3.x adds + `_pybind11_conduit_v1_` to every class it binds; nanobind adds nothing comparable). +- **SC-004**: There is exactly one copy of the binding source, and it is compiled once + per selected backend. +- **SC-005**: Backend-conditional compilation appears at no more than three sites + outside the compatibility header. +- **SC-006**: Selecting a single backend yields exactly one Python test entry; selecting + both yields exactly two; all of them pass. +- **SC-007**: Configuring with an unrecognized backend value fails immediately with a + message naming the three accepted values. +- **SC-008**: A build with the nanobind backend selected completes on a machine with no + pybind11 installed. +- **SC-009**: A build with the pybind11 backend selected completes on a machine with no + nanobind installed. +- **SC-010**: Installing a both-backends build leaves both backends' package files + intact, with neither having overwritten the other. +- **SC-011**: Continuous integration runs the both-backends configuration on at least + one Linux, one macOS, and one Windows job, and those jobs pass. +- **SC-012**: Iteration over a query object's parameters is covered by the test suite + under every selected backend. + +## Assumptions + +- The public Python API is exactly what `OSLQuery` and `OSLQuery::Parameter` expose + today. No new API is added, and nothing is removed, by this feature. +- `Parameter.type` is retained unchanged in both backends. Its dependence on OIIO's + Python module having been imported, with a matching binding framework, is a + pre-existing condition (it predates this feature and predates commit 642ab36f's + removal of the forced OIIO import) and is addressed by documentation rather than by + code. `Parameter.type_name` is the supported coupling-free alternative. +- The default backend is auto-selected (as of 2026-09-09): nanobind when OIIO >= 3.2 + and Python >= 3.10, pybind11 otherwise. Removing pybind11 support entirely is a + separate future change, still gated on soak time. +- OpenImageIO's dual-backend implementation is the reference for structure, naming, and + known framework differences. Deviating from it requires a stated reason. +- The project's existing local-dependency-build machinery is available for nanobind; if + it proves unsuitable, a build script in the style of the existing pybind11 one is an + acceptable fallback. +- Python and C++ language-level minimums (Python 3.9, C++17) already satisfy nanobind's + requirements, so no minimum-version changes are needed. +- Type stub files are out of scope; OSL ships none today. +- Python packaging (wheels) is out of scope; OSL has no wheel build. +- Bindings for `ShadingSystem` or any other OSL class remain out of scope, as they are + today. + +## Out of Scope + +- Removing pybind11 support. +- Adding, removing, or changing any Python-visible API. +- Binding any OSL class other than `OSLQuery` and `OSLQuery::Parameter`. +- Generating type stubs or building Python wheels. +- Changing how OpenImageIO builds its own bindings. diff --git a/docs/dev/specs/003-nanobind-python-bindings/tasks.md b/docs/dev/specs/003-nanobind-python-bindings/tasks.md new file mode 100644 index 0000000000..5f6c507528 --- /dev/null +++ b/docs/dev/specs/003-nanobind-python-bindings/tasks.md @@ -0,0 +1,215 @@ +# Tasks: nanobind Python bindings (dual-backend) + +**Input**: Design documents from `docs/dev/specs/003-nanobind-python-bindings/` + +**Branch**: `lg-nanobind` + +## Format: `[ID] [P?] [Story?] Description with file path` + +- **[P]**: Parallelizable - touches different files or non-overlapping sections +- **[Story]**: Which user story (US1-US5) from spec.md +- Each task is one logical change, reviewable as a single small diff + +**Testing strategy**: There is exactly one Python test, `testsuite/python-oslquery`, and +it is shared verbatim by every backend. It is not modified after Phase 1. Every +subsequent phase is validated by that same test producing byte-identical output against +the same unmodified `ref/out.txt`. That identity *is* the API-equivalence proof +(SC-002, SC-003). Phases 1-4 keep the default backend at pybind11, so nothing can break +for anyone until it is explicitly asked for. + +**Reference**: `~/code/oiio/oiio.lg` is the working implementation. When a task says +"mirror OIIO", read the cited file there first. + +--- + +## Phase 1: Prep under pybind11 (no behavior change) + +**Purpose**: Delete dead code, remove constructs that have no nanobind equivalent, and +turn on the iteration coverage - all while pybind11 is still the only backend, so each +step is independently verifiable. + +**Rationale for each deletion**: research.md §1. + +- [X] T001 [P] Delete the five `py::return_value_policy::reference_internal` arguments in `src/liboslquery/py_osl.cpp` (`Parameter.metadata` line 86, `OSLQuery.parameters` line 132, `OSLQuery.metadata` line 136, both `__getitem__` overloads lines 147 and 157). Every decorated lambda returns by value, so all five are already no-ops. Commit message must say so. +- [X] T002 Replace the value-returning factory constructor at `src/liboslquery/py_osl.cpp:98-102` with `.def(py::init(), "shadername"_a, "searchpath"_a = "")`. `OSLQuery(string_view, string_view = {})` exists at `src/include/OSL/oslquery.h:118`; `std::string` converts implicitly. nanobind has no equivalent of pybind11's value-returning `py::init(lambda)`. +- [X] T003 [P] Delete the unused `#include ` at `src/liboslquery/py_osl.cpp:7` +- [X] T004 [P] Delete dead declarations `python_array_code()` and `typedesc_from_python_array_code()` (`src/liboslquery/py_osl.h:54-55`) - declared, defined nowhere in the repo, called nowhere +- [X] T005 [P] Delete unused `object_classname()` (`src/liboslquery/py_osl.h:58-62`) - also uses the `.cast()` member syntax nanobind lacks +- [X] T006 [P] Delete the never-instantiated `C_to_tuple` specialization (`src/liboslquery/py_osl.h:107-116`) and the unused `` / `` includes (`src/liboslquery/py_osl.h:32-33`) +- [X] T007 [US1] Fix the stale comment in `testsuite/python-oslquery/src/test_oslquery.py` that describes the printed type as "an OpenImageIO::TypeDesc but it can print like a string" - since 642ab36f the test prints `type_name`, a `str` +- [X] T008 [US1] Re-enable the `for p in q:` loop in `testsuite/python-oslquery/src/test_oslquery.py` (commented out at ~lines 65-72 with a 2020-era `FIXME(pybind11)` about a macOS crash under pybind11 2.6) and delete the `for i in range(len(q))` workaround. This is the only coverage of `__iter__` / `make_iterator` / `keep_alive<0,1>` (FR-024). `ref/out.txt` must not change. If the macOS crash recurs, revert to the disabled form and record it in plan.md's risk table - do not paper over it. + +**Checkpoint**: `make test TEST=python-oslquery` passes against an unmodified +`testsuite/python-oslquery/ref/out.txt`. `git diff` shows no change to `ref/`. + +--- + +## Phase 2: The compatibility shim (still pybind11-only) + +**Purpose**: Introduce `py_backend.h` and route the binding source through it, while +`OSL_PY_BACKEND_NANOBIND` is never defined. Nothing changes behaviorally; this proves +the shim's pybind11 arm is correct before the nanobind arm is ever compiled. + +- [X] T009 Create `src/liboslquery/py_backend.h` with **both** arms, modeled on `~/code/oiio/oiio.lg/src/python/py_backend.h`: `namespace py` alias, `py_module` typedef, `OSL_PY_RW` / `OSL_PY_PROP_RO` / `OSL_PY_PROP_RW`, `PY_STR`, and `namespace osl_py` with `make_tuple`, `make_iterator`, `throw_key_error`. Full mapping table in plan.md Phase 1. The nanobind arm is written now but not yet compiled. +- [X] T010 In `src/liboslquery/py_backend.h`, get the nanobind include set right: ``, ``, ``, ``. `stl/vector.h` is load-bearing - it is what converts `std::vector` for `.parameters` and `.metadata`, and omitting it fails at runtime, not compile time. Comment it as such. +- [X] T011 In `src/liboslquery/py_backend.h`, make `osl_py::make_iterator` take the scope as a template parameter and pass `py::type()` under nanobind. It must be a **bound** type; `py::type>()` would be null. Comment why. +- [X] T012 Rewrite `src/liboslquery/py_osl.h` to `#include "py_backend.h"` in place of the pybind11 include block, the `namespace py` alias, and the `PY_STR` define. Switch the two `C_to_tuple` bodies to `osl_py::make_tuple`. Change `declare_oslquery()` to take `py_module&`. +- [X] T013 Apply the macro substitution in `src/liboslquery/py_osl.cpp`: `def_readwrite`→`OSL_PY_RW`, `def_property_readonly`→`OSL_PY_PROP_RO`, `def_property`→`OSL_PY_PROP_RW`; `declare_oslqueryparam` / `declare_oslquery` take `py_module&`; `py::make_iterator(...)` → `osl_py::make_iterator(...)`; `throw py::key_error(...)` → `osl_py::throw_key_error(...)` +- [X] T014 Factor the eight `m.attr(...)` assignments at `src/liboslquery/py_osl.cpp:179-186` into `void declare_module_attributes(py_module& m)`, so each arm of the forthcoming module-entry `#if` is three lines +- [X] T015 Add the dual module entry point at the end of `src/liboslquery/py_osl.cpp` per plan.md Phase 1: `NB_MODULE` at global scope (namespace closed first - unlike `PYBIND11_MODULE`), `_oslquery` vs `oslquery` on `OSL_PY_NANOBIND_ISOLATED_PACKAGE`, and `py::set_leak_warnings(false)` guarded on `PY_VERSION_HEX < 0x030a0000` citing wjakob/nanobind#1405 +- [X] T016 Verify the `#if` budget: **one** `OSL_PY_BACKEND_NANOBIND` site in `py_osl.cpp` (the module macro; the Python-version guard nested inside it is not backend-conditional) and zero in `py_osl.h` (FR-018, SC-005) - better than the budgeted two. The six conditionals in `py_backend.h` each carry a comment naming the framework difference that forces them. Equivalence checked by diffing a full public-surface + values + exception-type dump before and after: zero differences. + +**Checkpoint**: default build unchanged; `make test TEST=python-oslquery` still passes +against the same `ref/out.txt`. `nm` or equivalent shows the same exported symbols. + +--- + +## Phase 3: Build system - backend selection + +**Purpose**: Add the option and dependency plumbing. Default stays `pybind11`, so a +build with no new flags is untouched. + +- [X] T017 [US1] Add `OSL_PYTHON_BINDINGS_BACKEND` to `src/cmake/pythonutils.cmake`: `set_cache` (which routes through `set_utils.cmake:80 super_set` → `set_from_env`, making the same-named environment variable work - CI depends on that), `set_property(CACHE ... STRINGS pybind11 nanobind both)`, `string(TOLOWER ...)`, and a `FATAL_ERROR` naming the three accepted values (FR-001, FR-004) +- [X] T018 [US1] Derive `OSL_BUILD_PYTHON_PYBIND11` and `OSL_BUILD_PYTHON_NANOBIND` in `src/cmake/pythonutils.cmake`; these are what the rest of the build tests. Confirm `CMakeLists.txt:209 include (pythonutils)` still precedes `include (externalpackages)`. +- [X] T019 [US2] Add `discover_nanobind_cmake_dir()` to `src/cmake/pythonutils.cmake`, copied from `~/code/oiio/oiio.lg/src/cmake/pythonutils.cmake:112-145`. It runs `python -m nanobind --cmake_dir` to find nanobind installed as a pip/brew Python package (FR-006). Keep it a `function`, not a `macro` - carry over OIIO's comment explaining that a macro's `return()` would abort the including file. +- [X] T020 [US2] In `find_python()` (`src/cmake/pythonutils.cmake:17-55`), add a second `find_package (Python . EXACT REQUIRED COMPONENTS ...)` when `OSL_BUILD_PYTHON_NANOBIND` - nanobind's CMake package wants the unversioned `Python::Module` target, not OSL's `Python3::*` +- [X] T021 [US1] Guard the existing pybind11 find on `OSL_BUILD_PYTHON_PYBIND11` and add the nanobind find in `src/cmake/externalpackages.cmake:89-93`: `checked_find_package (nanobind CONFIG REQUIRED VERSION_MIN 2.8.0 BUILD_LOCAL missing)` (FR-005) +- [X] T022 [US2] Create `src/cmake/build_nanobind.cmake`, copied from OIIO. Preserve both workarounds *with their comments*: the pre-clone plus `git submodule update --init --depth 1 -- ext/robin_map` (nanobind's CMakeLists hard-errors without it, and `build_dependency_with_cmake()`'s plain clone does not init submodules), and the `nanobind_DIR ... FORCE` pointing at `/nanobind/cmake`. This is OSL's first use of `BUILD_LOCAL`. **Verified working** with `-DOSL_BUILD_LOCAL_DEPS=nanobind`: clone + submodule + configure + install + refind, about 1 second. The `build_nanobind.bash` fallback was not needed. + +**Checkpoint**: `-DOSL_PYTHON_BINDINGS_BACKEND=garbage` fails at configure with a message +naming the three values (SC-007). Default configure/build/test is unchanged. Configuring +with `nanobind` finds or builds nanobind and reports which (FR-006). + +--- + +## Phase 4: Build system - module targets + +**Purpose**: Actually build the nanobind module. + +- [X] T023 [US2] Add `setup_python_module_nanobind()` to `src/cmake/pythonutils.cmake`, beside the existing `setup_python_module()`. Differences: `nanobind_add_module()` takes no `${PYLIB_LIB_TYPE}` argument, and `target_compile_options (nanobind-static PUBLIC -Wno-format-nonliteral)` is needed for Clang/AppleClang/IntelLLVM because nanobind's own sources warn. +- [X] T024 [US2] Implement the single-backend output and install layout in `setup_python_module_nanobind()`: build tree `${CMAKE_BINARY_DIR}/lib/python/site-packages`, install `${PYTHON_SITE_DIR}/oslquery/` - byte-identical to what pybind11 produces today, so nanobind is a drop-in (FR-008, SC-001) +- [X] T025 [US3] Implement the `both`-mode layout in `setup_python_module_nanobind()`: build tree `${CMAKE_BINARY_DIR}/lib/python/nanobind/oslquery/`, install `${PYTHON_SITE_DIR}/nanobind/oslquery/`. **This deliberately deviates from OIIO**, which installs its `both`-mode `__init__.py` into the same site dir as pybind11's, where the two collide (FR-009, SC-010). Note the deviation in a comment. +- [X] T026 [US2] Add the nanobind block to `src/liboslquery/CMakeLists.txt`, over the *same* `file(GLOB py_*.cpp)` list as the pybind11 block. (The `if (OSL_BUILD_PYTHON_PYBIND11)` guard on the existing block landed in Phase 3, since searching for pybind11 conditionally while building against it unconditionally made `nanobind`-only fail at `pybind11_add_module`.) The nanobind block adds `-DOSL_PY_BACKEND_NANOBIND`, and `-DOSL_PY_NANOBIND_ISOLATED_PACKAGE` when the backend is `both`. One source list, two targets (`pyoslquery`, `pyoslquery_nanobind`) - FR-016. +- [X] T027 [US3] Create `src/liboslquery/nanobind/__init__.py`: `from ._oslquery import *`, the same Windows `add_dll_directory` block as `src/liboslquery/__init__.py`, and OIIO's trick of appending `Release`/`Debug`/`RelWithDebInfo`/`MinSizeRel` to `__path__` so the per-config `.pyd` resolves on MSVC multi-config builds +- [X] T028 [US3] `configure_file(nanobind/__init__.py ... COPYONLY)` into `${CMAKE_BINARY_DIR}/lib/python/nanobind/oslquery/` from `src/liboslquery/CMakeLists.txt`, and install it, in `both` mode only + +**Checkpoint**: all three backend settings configure and build. Inspect the build tree +against the layout table in plan.md Phase 2. Tests are not yet wired up for nanobind - +that is Phase 5. + +--- + +## Phase 5: Test registration + +**Purpose**: Run the one existing test against every selected backend. This is where +US2 and US3 become verifiable. + +- [X] T029 Add `osl_tests_pythonpath_env_entry(out_var prefix_dir)` to `src/cmake/testing.cmake`, mirroring `~/code/oiio/oiio.lg/src/cmake/testing.cmake:28`. Emits one `PYTHONPATH=:$ENV{PYTHONPATH}` string; on Windows uses `` alone, because semicolons get split by CMake list processing in a CTest `ENVIRONMENT` entry. Comment that. +- [X] T030 [US1] Replace the registration at `src/cmake/testing.cmake:474` with a direct `add_one_testsuite()` call for the pybind11 variant carrying `ENV TESTSHADE_OPT=0 "${_pybind_pypath}"`. This drops the pointless `python-oslquery.rs_bitcode` variant that `TESTSUITE()` was generating for a test that executes no shader, and fixes FR-023 (bare `ctest` previously could not run this test at all, since `PYTHONPATH` was set only in `Makefile:293`). +- [X] T031 [P] [US1] Remove the now-vestigial `NOOPTIMIZE` and `NOOPTIX` marker files from `testsuite/python-oslquery/`, and say in the commit that the `.rs_bitcode` variant is gone deliberately. (An existing build tree keeps a stale `build/testsuite/python-oslquery.rs_bitcode/` directory until it is cleaned; harmless, no longer a registered test.) +- [X] T032 [US2] [US3] Add the nanobind variant registration in `src/cmake/testing.cmake` with the empty-suffix trick from plan.md Phase 3: suffix is `.nanobind` only when the pybind11 variant was also registered, otherwise empty so the test keeps its historical name (FR-021, FR-022). Distinct names give distinct sandbox dirs automatically via `add_one_testsuite`'s `testdir` (`testing.cmake:37`), while `OSL_TESTSUITE_SRC` still points at the one source dir. +- [X] T033 [US1] Note in `Makefile` near line 293 that the `PYTHONPATH` prefix is retained for interactive use but that the CTest `ENVIRONMENT` property now takes precedence for the python tests + +**Added in this phase (not originally planned)**: `testsuite/python-oslquery/src/test_oslquery.py` +gained an "Empty string properties" section, and `ref/out.txt` was regenerated. `printparam` +only ever reaches `structname` for parameters that *are* structs, so the empty-`ustring` +path had no coverage at all - which is why the `ustring::c_str()` bug in Phase 2 passed +the whole suite. Confirmed the new coverage fails before that fix and passes after, by +temporarily reintroducing the bug. + +**Checkpoint**: this is the feature's main gate. + +```bash +make OSL_PYTHON_BINDINGS_BACKEND=pybind11 && make test TEST=python-oslquery # 1 test +make OSL_PYTHON_BINDINGS_BACKEND=nanobind && make test TEST=python-oslquery # 1 test, same name +make OSL_PYTHON_BINDINGS_BACKEND=both && make test TEST=python-oslquery # 2 tests +``` + +All must diff clean against the single unmodified `ref/out.txt` (SC-002, SC-006). + +--- + +## Phase 6: Equivalence verification + +**Purpose**: Prove the claims the spec makes, rather than assuming them. Findings here +are bugs to fix, not results to record. + +- [X] T034 [US2] Against a `both` build, compare `sorted(dir(oslquery.OSLQuery))`, `sorted(dir(oslquery.Parameter))`, and the sorted module attribute list between the two `PYTHONPATH`s. Zero differences required (FR-011, SC-003). If any appear, fix the binding - do not document the difference away. **Done via a 1279-observation dump** (surface + every property's type *and* value, via iteration, integer indexing, name indexing and the `parameters` property, plus metadata recursion, property setters and exception types). Zero differences, build tree and installed. Excluded: `_pybind11_conduit_v1_` and package-vs-bare-module artifacts, both documented in the harness. +- [X] T035 [US2] Verify every bound attribute returns the same Python type and value under both backends - spot-check `value` for the int, float, string, tuple, and `None` cases the test shader already covers (FR-012). Types are compared, not just values, so a `str`-vs-`bytes` style divergence would show. Found and fixed a real gap here: `__version__` was lost from *installed* packages (see research.md #16). +- [X] T036 [US2] Verify `__getitem__` raises `IndexError` for an out-of-range index and `KeyError` for an unknown name under both backends (FR-014) +- [X] T037 [US5] Verify `p.type` fails the same way under both backends when `OpenImageIO` has not been imported: a Python-level error, not a crash (FR-015) +- [X] T038 [US3] `cmake --build build --target install` for each backend; confirm the installed layout matches plan.md Phase 2's table and that the `both` install leaves both `__init__.py` files intact (SC-010) +- [X] T039 [P] Confirm a pybind11-backend build succeeds on a machine with no nanobind installed, and a nanobind-backend build succeeds with no pybind11 installed (SC-008, SC-009). Verified by poisoning the other package's `_DIR`/`_ROOT`: both configure and build their module, and the only log mentions of the absent package are CMake's unused-variable warnings. + +--- + +## Phase 7: CI + +- [X] T040 [P] Add a `python_bindings_backend` input (default `''`) to `.github/workflows/build-steps.yml`, exported as env `OSL_PYTHON_BINDINGS_BACKEND` next to `PYBIND11_VERSION` / `PYTHON_VERSION` at lines 135-136 +- [X] T041 Turn that env var into a `-D` flag in `src/build-scripts/ci-build.bash`, after the `USE_SIMD` block +- [X] T042 Forward the input in `.github/workflows/ci.yml` at the ~5 job-level sites that already forward `pybind11_ver` (lines 66, 150, 435, 515, 585). Do not touch the ~18 matrix entries' `pybind11_ver` values. +- [X] T043 Set `python_bindings_backend: both` on exactly three `.github/workflows/ci.yml` matrix entries - one recent Linux, one macOS-arm, one Windows (SC-011, Constitution IV) +- [X] T044 [P] Add a conditional `brew install nanobind` to `src/build-scripts/install_homebrew_deps.bash` when the backend is `nanobind` or `both` +- [X] T045 [P] Add a pinned nanobind `pip install` to `src/build-scripts/gh-installdeps.bash` and `gh-win-installdeps.bash`, following OIIO's `ci-requirements-nanobind.txt` sha256-pinning pattern. Pinned to match `nanobind_BUILD_VERSION` (2.13.0 originally; bumped to 3.0.1 in T052); the hash was taken from PyPI and the pin verified by installing it into a throwaway venv and running `python -m nanobind --cmake_dir`. + - **Caveat**: `gh-win-installdeps.bash` has a pre-existing bash syntax error at the `elif [[ "$LLVM_GOOGLE_DRIVE_ID" != "" ]] then` line (missing `;`), introduced by #2011 on 2025-07-28. `build-steps.yml:229` *executes* rather than sources that script, so bash fails to parse it and the whole step dies -- meaning the line added here can never run until that is fixed. Not fixed as part of this feature: it is unrelated, and unbreaking it may surface other Windows CI failures that would muddy this PR. Windows `both` still works regardless, because `BUILD_LOCAL missing` builds nanobind from source when the pip install hasn't happened. +- [X] T046 Separate commit: verify that `python-oslquery` now passes in the two ASWF-container jobs and remove the stale `CTEST_EXCLUSIONS` entries at `.github/workflows/ci.yml:199-201, 212-214`. Their comment says the exclusion is needed "until the ASWF container properly includes OIIO's python bindings"; since 642ab36f the test no longer imports OpenImageIO. This is an independent fix - if it turns out the exclusion is still needed for another reason, drop the task and say why. **Removed on the strength of reading, not running**: the ASWF containers can't be exercised locally, so the first CI run on this branch is what actually confirms it. Left alone: the optix-gpu job also excludes `python-oslquery`, but bundled with GPU-specific exclusions and without the OIIO rationale, so it is a separate question. + +--- + +## Phase 8: Documentation + +- [X] T047 [P] [US2] Add nanobind to `INSTALL.md:69-73` as a conditional dependency with its minimum version (Constitution IV requires documented dependency minimums), and document `OSL_PYTHON_BINDINGS_BACKEND` (FR-025). INSTALL.md turned out to have no build-options section, so the option is documented in a new "Python binding backends" section instead, which also carries the `Parameter.type` caveat (T048). +- [X] T048 [P] [US5] Document the `Parameter.type` interoperability constraint (FR-026): it returns an OIIO `TypeDesc`, so it needs OIIO's Python module - built with the same binding backend and a compatible internals/ABI version - to have been imported; `type_name` is the coupling-free alternative. Put it in `INSTALL.md` and as a comment at the binding site in `src/liboslquery/py_osl.cpp`. +- [~] T049 [P] ~~Add a `CHANGES.md` entry~~ **Deferred to release preparation** at the maintainer's direction -- CHANGES.md is written up as part of the release process, not per-PR. +- [X] T050 [P] Create `src/liboslquery/MIGRATION_STATUS.md` (FR-028), a short version of `~/code/oiio/oiio.lg/src/python/MIGRATION_STATUS.md`: which macros to use, `py_module&` rather than `py::module&`, keep `#if` sites minimal and commented, and the invariant - "consumer-visible differences: none intended; if you find one, it is a bug, add a regression test" + +--- + +## Phase 9: Conditional default backend (2026-09-09) + +Soak-time follow-on. The dual-backend machinery has been in place and green; make the +default select nanobind only when the build can actually use it, and refresh the +auto-built nanobind version. + +- [X] T051 Make the `OSL_PYTHON_BINDINGS_BACKEND` default conditional. Cache default is now `""`; the normalize/validate/derive logic moved from file scope in `src/cmake/pythonutils.cmake` into a new `osl_resolve_python_bindings_backend()` macro. `find_python()` calls it right after `Python3` is found (and after OIIO, found earlier in `externalpackages.cmake`), before the nanobind-specific `find_package(Python)` and the pybind11/nanobind package finds. An empty selector resolves to `nanobind` when `OpenImageIO_VERSION >= 3.2` (OIIO defaults its own bindings to nanobind there) **and** `Python3_VERSION >= 3.10` (nanobind's minimum), else `pybind11`. It does not check whether nanobind is installed - the real `checked_find_package(nanobind ... BUILD_LOCAL missing)` builds it locally when absent (it is a ~1s header/CMake copy). Explicit `pybind11`/`nanobind`/`both` and the env var still honored. FR-002 updated in spec.md. + - Depends on the `NO_FP_RANGE_CHECK` on the real `checked_find_package(nanobind ...)`: nanobind's config-version file is `SameMajorVersion`, so `VERSION_MIN 2.8.0` alone rejects an installed nanobind 3.x and forces a local build. `NO_FP_RANGE_CHECK` lets the real find accept the installed 3.x. +- [X] T052 Bump `nanobind_BUILD_VERSION` in `src/cmake/build_nanobind.cmake` from 2.13.0 to 3.0.1 (latest), with `nanobind_GIT_COMMIT` = `db4827f06f6f1680e5d4004c95fc8d69299dba8b` (peeled `v3.0.1` tag). Sync `src/build-scripts/ci-requirements-nanobind.txt` to `nanobind==3.0.1` with the PyPI wheel sha256. Verified locally: `BUILD_LOCAL` clone + configure + install of 3.0.1 succeeds, `pyoslquery_nanobind` builds clean, module imports. Homebrew nanobind is already 3.0.1. +- [X] T053 [P] Docs: INSTALL.md "tested through" bump to 3.0, the two-condition auto-select rule (OIIO >= 3.2 and Python >= 3.10) in the dependency list and the `Python binding backends` section, plan.md/spec.md consistency. +- [X] T054 CI: `src/build-scripts/ci-startup.bash` unconditionally exported `OSL_PYTHON_BINDINGS_BACKEND=both`, which forced nanobind (and its Python >= 3.10 requirement) onto the Python 3.9 jobs - `pip install nanobind==3.0.1` fails Requires-Python and the `BUILD_LOCAL` fallback then hits `nanobind-config.cmake` "requires Python 3.10 or newer". Fix: just stop setting it. ci.yml no longer forces a value either, so an unset backend lets `pythonutils.cmake` auto-select per job (pybind11 for old Python/OIIO, nanobind for modern). A ci.yml matrix entry can still set `python_bindings_backend` to force `both` (or either single backend) for a specific job. + +--- + +## Dependencies + +``` +Phase 1 (T001-T008) ── independent, lands first, verifiable with today's build + ↓ +Phase 2 (T009-T016) ── needs T002 (no factory init) and T001 (no rv policies) + ↓ +Phase 3 (T017-T022) ── option + deps; T021 needs T018's derived booleans + ↓ +Phase 4 (T023-T028) ── needs Phase 2 (compilable nanobind arm) and Phase 3 (nanobind found) + ↓ +Phase 5 (T029-T033) ── needs Phase 4 (modules exist to point PYTHONPATH at) + ↓ +Phase 6 (T034-T039) ── verification; needs Phase 5 + ↓ +Phase 7 (T040-T046) ── CI; needs Phase 5 green locally. T046 is independent of everything else. +Phase 8 (T047-T050) ── docs; can start any time after Phase 3 fixes the option's name +``` + +Phases 7 and 8 are mutually independent and can proceed in parallel. + +## Incremental delivery + +- **After Phase 1**: dead code gone, iteration covered. Shippable on its own. +- **After Phase 2**: bindings are backend-neutral source; still pybind11-only in + practice. Shippable on its own. +- **After Phase 5**: US1, US2, and US3 all satisfied - this is the feature's MVP. +- **After Phase 8**: complete. + +## Out of scope (restated from spec.md) + +Removing pybind11 support; any Python API change; binding any other OSL class; type +stubs; wheels. (The default backend became conditional - nanobind when the build can +use it, else pybind11 - on 2026-09-09; see Phase 9.) diff --git a/src/build-scripts/ci-build.bash b/src/build-scripts/ci-build.bash index 5b3acac2b9..f089e60cd6 100755 --- a/src/build-scripts/ci-build.bash +++ b/src/build-scripts/ci-build.bash @@ -18,6 +18,10 @@ if [[ "$USE_SIMD" != "" ]] ; then OSL_CMAKE_FLAGS="$OSL_CMAKE_FLAGS -DUSE_SIMD=$USE_SIMD" fi +if [[ -n "${OSL_PYTHON_BINDINGS_BACKEND:-}" ]] ; then + OSL_CMAKE_FLAGS="$OSL_CMAKE_FLAGS -DOSL_PYTHON_BINDINGS_BACKEND=${OSL_PYTHON_BINDINGS_BACKEND}" +fi + if [[ -n "$CODECOV" ]] ; then OSL_CMAKE_FLAGS="$OSL_CMAKE_FLAGS -DCODECOV=${CODECOV}" fi diff --git a/src/build-scripts/ci-requirements-nanobind.txt b/src/build-scripts/ci-requirements-nanobind.txt new file mode 100644 index 0000000000..3fc41ac3b1 --- /dev/null +++ b/src/build-scripts/ci-requirements-nanobind.txt @@ -0,0 +1,13 @@ +# CI-only: nanobind for CMake (`python -m nanobind --cmake_dir`). Used on Linux +# (gh-installdeps) and Windows (gh-win-installdeps) with pip --require-hashes. +# macOS CI uses `brew install nanobind` instead (install_homebrew_deps.bash). +# +# Only needed when OSL_PYTHON_BINDINGS_BACKEND is nanobind or both. If this +# install fails, src/cmake/build_nanobind.cmake builds nanobind locally +# instead, so CI still works -- it's just slower. +# +# Keep this version in sync with nanobind_BUILD_VERSION in +# src/cmake/build_nanobind.cmake. When bumping the pin, get the wheel sha256 +# from https://pypi.org/pypi/nanobind//json +nanobind==3.0.1 \ + --hash=sha256:4b49491fe8bf483a5e0342125b8fe56237e490ba642c7e296170b8cd256a7764 diff --git a/src/build-scripts/gh-installdeps.bash b/src/build-scripts/gh-installdeps.bash index 7ea57c5f7c..b0c5013194 100755 --- a/src/build-scripts/gh-installdeps.bash +++ b/src/build-scripts/gh-installdeps.bash @@ -174,6 +174,12 @@ if [[ "$PYBIND11_VERSION" != "0" ]] ; then source src/build-scripts/build_pybind11.bash fi +# nanobind is only needed for the non-default python binding backends. Install +# the pinned wheel if we can; CMake builds it locally if this doesn't work out. +if [[ "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "nanobind" || "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "both" ]] ; then + pip3 install --require-hashes -r src/build-scripts/ci-requirements-nanobind.txt || true +fi + if [[ "$OPENEXR_VERSION" != "" ]] ; then source src/build-scripts/build_openexr.bash fi diff --git a/src/build-scripts/gh-win-installdeps.bash b/src/build-scripts/gh-win-installdeps.bash index 085f9ff9c7..520c72df24 100755 --- a/src/build-scripts/gh-win-installdeps.bash +++ b/src/build-scripts/gh-win-installdeps.bash @@ -78,6 +78,12 @@ export PNG_ROOT=$PWD/ext/dist source src/build-scripts/build_pybind11.bash export pybind11_ROOT=$PWD/ext/dist +# nanobind is only needed for the non-default python binding backends. Install +# the pinned wheel if we can; CMake builds it locally if this doesn't work out. +if [[ "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "nanobind" || "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "both" ]] ; then + pip install --require-hashes -r src/build-scripts/ci-requirements-nanobind.txt || true +fi + if [[ "$OPENEXR_VERSION" != "" ]] ; then source src/build-scripts/build_openexr.bash diff --git a/src/build-scripts/install_homebrew_deps.bash b/src/build-scripts/install_homebrew_deps.bash index 573035cc2a..b3399592ec 100755 --- a/src/build-scripts/install_homebrew_deps.bash +++ b/src/build-scripts/install_homebrew_deps.bash @@ -67,6 +67,12 @@ brew list --versions # Needed on some systems pip${PYTHON_VERSION} install numpy || true +# nanobind is only needed for the non-default python binding backends. If it +# can't be installed, the build falls back to CMake building it locally. +if [[ "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "nanobind" || "${OSL_PYTHON_BINDINGS_BACKEND:-}" == "both" ]] ; then + brew install nanobind || true +fi + # Set up paths. These will only affect the caller if this script is # run with 'source' rather than in a separate shell. export PATH=${HOMEBREW_PREFIX}/opt/qt5/bin:$PATH diff --git a/src/cmake/build_nanobind.cmake b/src/cmake/build_nanobind.cmake new file mode 100644 index 0000000000..dac54bf91f --- /dev/null +++ b/src/cmake/build_nanobind.cmake @@ -0,0 +1,71 @@ +# Copyright Contributors to the Open Shading Language project. +# SPDX-License-Identifier: BSD-3-Clause +# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +###################################################################### +# nanobind by hand! +# +# Unlike most of our other bundled dependencies, nanobind isn't a library +# to compile and install -- it's a source tree plus a set of CMake helper +# functions (nanobind_add_module(), etc.) that get pulled into whichever +# project actually builds Python extension modules. Its own top-level +# CMakeLists.txt, configured standalone with NB_TEST=OFF, does nothing +# but copy its headers/sources/cmake helpers to an install prefix and +# generate a version file -- there's nothing to actually compile. That's +# the (unusually light) local build performed below. +# +# This mirrors OpenImageIO's src/cmake/build_nanobind.cmake; keep the two +# in sync when bumping versions. +###################################################################### + +set_cache (nanobind_BUILD_VERSION 3.0.1 "nanobind version for local builds") +set (nanobind_GIT_REPOSITORY "https://github.com/wjakob/nanobind") +set_cache (nanobind_GIT_TAG "v${nanobind_BUILD_VERSION}" + "nanobind git tag to checkout") +set_cache (nanobind_GIT_COMMIT "db4827f06f6f1680e5d4004c95fc8d69299dba8b" + "nanobind commit hash to verify tag against") + +set (nanobind_LOCAL_SOURCE_DIR "${${PROJECT_NAME}_LOCAL_DEPS_ROOT}/nanobind") + +# nanobind vendors tsl::robin_map as a git submodule (ext/robin_map), and +# its CMakeLists.txt hard-errors if that submodule isn't checked out. +# build_dependency_with_cmake()'s plain `git clone` doesn't init submodules, +# so fetch the source and its submodule ourselves before handing off to the +# shared helper below (which will find the directory already present and +# just checkout/verify the pinned tag against it). +if (NOT IS_DIRECTORY "${nanobind_LOCAL_SOURCE_DIR}") + find_package (Git REQUIRED) + message (STATUS "Cloning ${nanobind_GIT_REPOSITORY} @ ${nanobind_GIT_TAG}") + execute_process (COMMAND ${GIT_EXECUTABLE} clone -q + -b ${nanobind_GIT_TAG} --depth 1 + ${nanobind_GIT_REPOSITORY} ${nanobind_LOCAL_SOURCE_DIR}) + execute_process (COMMAND ${GIT_EXECUTABLE} submodule update --init --depth 1 + -- ext/robin_map + WORKING_DIRECTORY ${nanobind_LOCAL_SOURCE_DIR}) +endif () + +build_dependency_with_cmake(nanobind + VERSION ${nanobind_BUILD_VERSION} + GIT_REPOSITORY ${nanobind_GIT_REPOSITORY} + GIT_TAG ${nanobind_GIT_TAG} + GIT_COMMIT ${nanobind_GIT_COMMIT} + CMAKE_ARGS + # Skip nanobind's own test suite -- we only want its install rules + # (headers, sources, and CMake helper functions), not a build of its + # tests, which would otherwise need Python at configure time and a + # lot of unnecessary compilation. + -D NB_TEST=OFF + ) + +# nanobind installs its CMake package config to /nanobind/cmake, a +# layout that generic find_package() prefix search doesn't check, so point +# straight at it. (This is the same trick pythonutils.cmake's +# discover_nanobind_cmake_dir() uses for a pip or Homebrew install, via +# `python -m nanobind --cmake_dir`.) +set (nanobind_DIR "${nanobind_LOCAL_INSTALL_DIR}/nanobind/cmake" CACHE PATH + "Path to the nanobind CMake package" FORCE) + +# Signal to caller that we need to find again at the installed location +set (nanobind_REFIND TRUE) +set (nanobind_REFIND_ARGS CONFIG) +set (nanobind_REFIND_VERSION ${nanobind_BUILD_VERSION}) diff --git a/src/cmake/externalpackages.cmake b/src/cmake/externalpackages.cmake index 62a6f21ad8..9cbbee1d6a 100644 --- a/src/cmake/externalpackages.cmake +++ b/src/cmake/externalpackages.cmake @@ -85,11 +85,20 @@ endif () checked_find_package (partio) -# From pythonutils.cmake +# From pythonutils.cmake. Which binding framework(s) we need is selected by +# OSL_PYTHON_BINDINGS_BACKEND; only look for the ones actually asked for, so +# that e.g. a nanobind-only build doesn't require pybind11 to be installed. find_python () -if (USE_PYTHON) +if (USE_PYTHON AND OSL_BUILD_PYTHON_PYBIND11) checked_find_package (pybind11 REQUIRED VERSION_MIN 2.7) endif () +if (USE_PYTHON AND OSL_BUILD_PYTHON_NANOBIND) + discover_nanobind_cmake_dir () + checked_find_package (nanobind CONFIG REQUIRED + VERSION_MIN 2.8.0 + NO_FP_RANGE_CHECK + BUILD_LOCAL missing) +endif () # Qt -- used for osltoy diff --git a/src/cmake/pythonutils.cmake b/src/cmake/pythonutils.cmake index 2fd050fde7..705faf7d10 100644 --- a/src/cmake/pythonutils.cmake +++ b/src/cmake/pythonutils.cmake @@ -5,6 +5,56 @@ # Python-related options. set_option (USE_PYTHON "Build the Python bindings" ON) set (PYTHON_VERSION "" CACHE STRING "Target version of python to try to find") +set_cache (OSL_PYTHON_BINDINGS_BACKEND "" + "Which Python binding backend(s) to build: pybind11, nanobind, both, or empty to auto-select (nanobind when OIIO >= 3.2 and Python >= 3.10, else pybind11)" VERBOSE) +set_property (CACHE OSL_PYTHON_BINDINGS_BACKEND PROPERTY STRINGS + "" pybind11 nanobind both) + +# The user-facing backend selector is normalized, auto-resolved, and turned +# into booleans by osl_resolve_python_bindings_backend(), below. It must run +# *after* both OpenImageIO and Python have been found (find_python() calls it +# at that point): an empty selector picks nanobind when OpenImageIO is >= 3.2 +# (older OIIO ships pybind11 python bindings, and mixing binding frameworks +# between the OIIO and OSL python modules does not work -- see INSTALL.md) and +# Python is >= 3.10 (nanobind's minimum), otherwise pybind11. nanobind itself +# need not be installed: the checked_find_package below builds it locally when +# it is missing. An explicit pybind11 / nanobind / both is honored as-is. +macro (osl_resolve_python_bindings_backend) + # set_cache means this can also be set by an environment variable of the + # same name, which is how CI drives it. + string (TOLOWER "${OSL_PYTHON_BINDINGS_BACKEND}" OSL_PYTHON_BINDINGS_BACKEND) + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "") + if (OpenImageIO_VERSION AND OpenImageIO_VERSION VERSION_GREATER_EQUAL 3.2 + AND Python3_VERSION AND Python3_VERSION VERSION_GREATER_EQUAL 3.10) + set (OSL_PYTHON_BINDINGS_BACKEND "nanobind") + message (STATUS "OSL_PYTHON_BINDINGS_BACKEND not set; auto-selected " + "'nanobind' (OpenImageIO ${OpenImageIO_VERSION}, Python ${Python3_VERSION})") + else () + set (OSL_PYTHON_BINDINGS_BACKEND "pybind11") + message (STATUS "OSL_PYTHON_BINDINGS_BACKEND not set; auto-selected " + "'pybind11' (nanobind needs OpenImageIO >= 3.2 and Python >= 3.10; " + "have ${OpenImageIO_VERSION} and ${Python3_VERSION})") + endif () + endif () + if (NOT OSL_PYTHON_BINDINGS_BACKEND MATCHES "^(pybind11|nanobind|both)$") + message (FATAL_ERROR + "OSL_PYTHON_BINDINGS_BACKEND must be one of: pybind11, nanobind, both") + endif () + + # Derive internal switches used by externalpackages.cmake, testing.cmake, + # and the Python helper macros below. + set (OSL_BUILD_PYTHON_PYBIND11 OFF) + set (OSL_BUILD_PYTHON_NANOBIND OFF) + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "pybind11" + OR OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + set (OSL_BUILD_PYTHON_PYBIND11 ON) + endif () + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "nanobind" + OR OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + set (OSL_BUILD_PYTHON_NANOBIND ON) + endif () +endmacro () + if (WIN32) set (PYLIB_LIB_TYPE SHARED CACHE STRING "Type of library to build for python module (MODULE or SHARED)") else () @@ -38,6 +88,23 @@ macro (find_python) Python3_Development_FOUND Python3_Interpreter_FOUND ) + # OpenImageIO was found earlier in externalpackages.cmake and Python3 just + # above -- both are needed to auto-select the Python binding backend from + # an empty OSL_PYTHON_BINDINGS_BACKEND, so resolve it here, before the + # nanobind-specific Python find and the pybind11/nanobind package finds + # that follow (all of which key off OSL_BUILD_PYTHON_{PYBIND11,NANOBIND}). + osl_resolve_python_bindings_backend () + + if (OSL_BUILD_PYTHON_NANOBIND) + # nanobind's CMake package expects the generic FindPython targets and + # variables (Python::Module, Python_EXECUTABLE, etc.), not the + # versioned Python3::* targets that the rest of OSL uses. Ask for the + # exact version we just found so the two can't disagree. + find_package (Python ${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR} + EXACT REQUIRED + COMPONENTS Interpreter Development) + endif () + # The version that was found may not be the default or user # defined one. set (PYTHON_VERSION_FOUND ${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}) @@ -55,6 +122,45 @@ macro (find_python) endmacro() +# Help CMake locate nanobind when it was installed as a Python package (pip +# or Homebrew), which is the common case -- nanobind ships its CMake package +# config inside the Python package rather than in a place find_package()'s +# prefix search would look. +# +# This is a function (not a macro) deliberately: its early return must not +# escape into whatever file happens to include pythonutils.cmake and call +# this at file scope (a macro's return() would abort that entire caller file, +# silently skipping everything after it). +function (discover_nanobind_cmake_dir) + # Cached from a previous configure. Trust it only if it still points to a + # real nanobind install -- it may be stale if nanobind was uninstalled or + # upgraded since the cache was written. + if (nanobind_DIR AND EXISTS "${nanobind_DIR}/nanobind-config.cmake") + return () + endif () + # Don't second-guess an explicit user hint. + if (nanobind_ROOT OR "$ENV{nanobind_DIR}" OR "$ENV{nanobind_ROOT}") + return () + endif () + if (NOT Python3_Interpreter_FOUND) + return () + endif () + + execute_process ( + COMMAND ${Python3_EXECUTABLE} -m nanobind --cmake_dir + RESULT_VARIABLE _osl_nanobind_result + OUTPUT_VARIABLE _osl_nanobind_cmake_dir + OUTPUT_STRIP_TRAILING_WHITESPACE + ERROR_QUIET) + if (_osl_nanobind_result EQUAL 0 + AND EXISTS "${_osl_nanobind_cmake_dir}/nanobind-config.cmake") + message (VERBOSE " Found nanobind CMake package via Python at ${_osl_nanobind_cmake_dir}") + set (nanobind_DIR "${_osl_nanobind_cmake_dir}" CACHE PATH + "Path to the nanobind CMake package" FORCE) + endif () +endfunction () + + ########################################################################### # pybind11 @@ -111,7 +217,109 @@ macro (setup_python_module) RUNTIME DESTINATION ${PYTHON_SITE_DIR}/${lib_MODULE} COMPONENT user LIBRARY DESTINATION ${PYTHON_SITE_DIR}/${lib_MODULE} COMPONENT user) - install(FILES __init__.py DESTINATION ${PYTHON_SITE_DIR}/${lib_MODULE}) + # COMPONENT user to match the TARGETS install above -- without it, a + # component-filtered install produces a package directory holding the + # extension module but no __init__.py, which can't be imported. + install(FILES __init__.py DESTINATION ${PYTHON_SITE_DIR}/${lib_MODULE} + COMPONENT user) + +endmacro () + + + +########################################################################### +# nanobind +# +# Same job as setup_python_module() above, but building the module with +# nanobind instead of pybind11. Arguments are the same, with MODULE naming +# the *package* (as for pybind11); the extension module inside it is named +# by this macro, because that depends on whether we're the only backend: +# +# backend=nanobind : /lib/python/site-packages/oslquery.so +# installed to ${PYTHON_SITE_DIR}/oslquery/ +# -- exactly where and what pybind11 would have put +# there, i.e. a drop-in replacement. +# +# backend=both : /lib/python/nanobind/oslquery/_oslquery.so +# installed to ${PYTHON_SITE_DIR}/nanobind/oslquery/ +# -- kept out of the way of the pybind11 module, which +# owns the ordinary location. `import oslquery` picks +# whichever one is on sys.path; a small __init__.py +# re-exports _oslquery so the import name is the same +# either way. +# +macro (setup_python_module_nanobind) + cmake_parse_arguments (lib "" "TARGET;MODULE" "SOURCES;LIBS;PACKAGE_FILES" ${ARGN}) + + set (target_name ${lib_TARGET}) + + if (NOT COMMAND nanobind_add_module) + discover_nanobind_cmake_dir () + endif () + + if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux" AND NOT ${CMAKE_COMPILER_ID} STREQUAL "Intel") + # Seems to be a problem on some systems, with the python headers + set_property (SOURCE ${lib_SOURCES} APPEND_STRING PROPERTY COMPILE_FLAGS " -Wno-macro-redefined ") + endif () + + # Note: unlike pybind11_add_module, this takes no MODULE/SHARED argument. + nanobind_add_module (${target_name} ${lib_SOURCES}) + + # nanobind's own sources trip -Wformat-nonliteral on clang. + if (TARGET nanobind-static AND (CMAKE_CXX_COMPILER_ID MATCHES "Clang" + OR CMAKE_CXX_COMPILER_ID MATCHES "Apple" + OR CMAKE_CXX_COMPILER_ID MATCHES "IntelLLVM")) + target_compile_options (nanobind-static PUBLIC -Wno-format-nonliteral) + endif () + + target_link_libraries (${target_name} PRIVATE ${lib_LIBS}) + target_compile_definitions (${target_name} PRIVATE OSL_PY_BACKEND_NANOBIND) + + set (_module_LINK_FLAGS "${VISIBILITY_MAP_COMMAND} ${EXTRA_DSO_LINK_ARGS}") + if (UNIX AND NOT APPLE) + # Hide symbols from any static dependent libraries embedded here. + set (_module_LINK_FLAGS "${_module_LINK_FLAGS} -Wl,--exclude-libs,ALL") + endif () + set_target_properties (${target_name} PROPERTIES + LINK_FLAGS ${_module_LINK_FLAGS} + DEBUG_POSTFIX "") + + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + set (_nanobind_build_dir ${CMAKE_BINARY_DIR}/lib/python/nanobind/${lib_MODULE}) + set (_nanobind_install_dir ${PYTHON_SITE_DIR}/nanobind/${lib_MODULE}) + target_compile_definitions (${target_name} + PRIVATE OSL_PY_NANOBIND_ISOLATED_PACKAGE) + set_target_properties (${target_name} PROPERTIES OUTPUT_NAME _${lib_MODULE}) + else () + set (_nanobind_build_dir ${CMAKE_BINARY_DIR}/lib/python/site-packages) + set (_nanobind_install_dir ${PYTHON_SITE_DIR}/${lib_MODULE}) + set_target_properties (${target_name} PROPERTIES OUTPUT_NAME ${lib_MODULE}) + endif () + + set_target_properties (${target_name} PROPERTIES + LIBRARY_OUTPUT_DIRECTORY ${_nanobind_build_dir} + ARCHIVE_OUTPUT_DIRECTORY ${_nanobind_build_dir} + ) + + install (TARGETS ${target_name} + RUNTIME DESTINATION ${_nanobind_install_dir} COMPONENT user + LIBRARY DESTINATION ${_nanobind_install_dir} COMPONENT user) + + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + # The isolated package needs its own __init__.py (re-exporting + # _${lib_MODULE}), and needs it in the build tree too, since that's + # what the testsuite imports. + configure_file (${lib_PACKAGE_FILES} ${_nanobind_build_dir}/__init__.py + COPYONLY) + install (FILES ${lib_PACKAGE_FILES} DESTINATION ${_nanobind_install_dir} + COMPONENT user RENAME __init__.py) + else () + # Drop-in replacement: same __init__.py the pybind11 module installs, + # and like it, nothing extra in the build tree (the testsuite imports + # the bare extension module from site-packages). + install (FILES __init__.py DESTINATION ${_nanobind_install_dir} + COMPONENT user) + endif () endmacro () diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake index eaf39e27e4..2daf573e50 100644 --- a/src/cmake/testing.cmake +++ b/src/cmake/testing.cmake @@ -20,6 +20,29 @@ add_custom_target ( CopyFiles ALL DEPENDS "${CMAKE_BINARY_DIR}/testsuite/runtest set (OSL_TEST_BIG_TIMEOUT 800 CACHE STRING "Timeout for tests that take a long time") +# Build a single "PYTHONPATH=..." entry suitable for a CTest ENVIRONMENT +# property, putting prefix_dir first. +# +# This is how the python tests find the module: which binding backend a given +# test run exercises is decided entirely by which directory this points at. +# (It also means `ctest` run directly in the build tree works -- previously +# PYTHONPATH was only ever set by the `make test` wrapper.) +# +# On Windows, deliberately don't append the inherited PYTHONPATH: entries are +# separated by ';' there, which CMake would then split as a list separator +# when this is used as a test ENVIRONMENT entry. +function (osl_tests_pythonpath_env_entry out_var prefix_dir) + if (WIN32) + set (_pythonpath "${prefix_dir}") + elseif (DEFINED ENV{PYTHONPATH} AND NOT "$ENV{PYTHONPATH}" STREQUAL "") + set (_pythonpath "${prefix_dir}:$ENV{PYTHONPATH}") + else () + set (_pythonpath "${prefix_dir}") + endif () + set (${out_var} "PYTHONPATH=${_pythonpath}" PARENT_SCOPE) +endfunction () + + # add_one_testsuite() - set up one testsuite entry # # Usage: @@ -481,8 +504,42 @@ macro (osl_add_all_tests) # We also exclude these tests if this is a sanitizer build, because the # Python interpreter itself won't be linked with the right asan # libraries to run correctly. + # + # These go through add_one_testsuite directly rather than TESTSUITE(), + # because they need a per-variant PYTHONPATH (which is the *only* thing + # that selects which binding backend a run exercises), and because none of + # the variants TESTSUITE() generates -- optimized, batched, rs_bitcode, + # optix -- mean anything for a test that never executes a shader. if (USE_PYTHON AND Python3_Development_FOUND AND NOT SANITIZE) - TESTSUITE ( python-oslquery ) + set (_py_testsrc "${CMAKE_SOURCE_DIR}/testsuite/python-oslquery") + osl_tests_pythonpath_env_entry (_pybind_pypath + "${CMAKE_BINARY_DIR}/lib/python/site-packages") + if (OSL_PYTHON_BINDINGS_BACKEND STREQUAL "both") + # In "both" mode the nanobind module is kept in its own build-tree + # package so it doesn't shadow the pybind11 one, which owns + # lib/python/site-packages. + osl_tests_pythonpath_env_entry (_nb_pypath + "${CMAKE_BINARY_DIR}/lib/python/nanobind") + else () + # nanobind-only builds put the module exactly where pybind11 + # would have, so the same path serves. + set (_nb_pypath "${_pybind_pypath}") + endif () + + set (_nb_suffix ".nanobind") + if (OSL_BUILD_PYTHON_PYBIND11) + add_one_testsuite ("python-oslquery" "${_py_testsrc}" + ENV TESTSHADE_OPT=0 "${_pybind_pypath}") + else () + # Whichever backend is the only one built gets the plain test + # name; the suffix exists to disambiguate, so with nothing to + # disambiguate from it would just be noise. + set (_nb_suffix "") + endif () + if (OSL_BUILD_PYTHON_NANOBIND) + add_one_testsuite ("python-oslquery${_nb_suffix}" "${_py_testsrc}" + ENV TESTSHADE_OPT=0 "${_nb_pypath}") + endif () endif () # Only run openvdb-related tests if the local OIIO has openvdb support. diff --git a/src/liboslquery/CMakeLists.txt b/src/liboslquery/CMakeLists.txt index 70a252595e..934de7c7ba 100644 --- a/src/liboslquery/CMakeLists.txt +++ b/src/liboslquery/CMakeLists.txt @@ -40,11 +40,27 @@ install_targets (${local_lib}) # from pythonutils.cmake +# +# Both backends compile the very same sources; the only difference is the +# OSL_PY_BACKEND_NANOBIND define (set by setup_python_module_nanobind), which +# py_backend.h keys off. When OSL_PYTHON_BINDINGS_BACKEND is "both", we get +# two targets from one source list. if (USE_PYTHON AND Python3_Development_FOUND) file (GLOB python_srcs py_*.cpp) - setup_python_module (TARGET pyoslquery - MODULE oslquery - SOURCES ${python_srcs} - LIBS oslquery - ) + if (OSL_BUILD_PYTHON_PYBIND11) + setup_python_module (TARGET pyoslquery + MODULE oslquery + SOURCES ${python_srcs} + LIBS oslquery + ) + endif () + if (OSL_BUILD_PYTHON_NANOBIND) + setup_python_module_nanobind ( + TARGET pyoslquery_nanobind + MODULE oslquery + SOURCES ${python_srcs} + LIBS oslquery + PACKAGE_FILES nanobind/__init__.py + ) + endif () endif () diff --git a/src/liboslquery/MIGRATION_STATUS.md b/src/liboslquery/MIGRATION_STATUS.md new file mode 100644 index 0000000000..d491455d20 --- /dev/null +++ b/src/liboslquery/MIGRATION_STATUS.md @@ -0,0 +1,101 @@ + + +Python bindings: pybind11 / nanobind dual-backend status +======================================================== + +OSL's Python bindings can be compiled against either +[pybind11](https://github.com/pybind/pybind11) or +[nanobind](https://github.com/wjakob/nanobind), selected at build time by +`OSL_PYTHON_BINDINGS_BACKEND` (`pybind11` | `nanobind` | `both`, default +`pybind11`). Both are built from **one** set of sources; the only difference +is the `OSL_PY_BACKEND_NANOBIND` define. + +This mirrors what OpenImageIO did, and for the same reason: the two projects' +Python modules share `TypeDesc` through the binding framework's process-wide +type registry, so they need to be able to agree on which framework that is. +See `INSTALL.md`, section "Python binding backends", for the user-facing view. + +The end state is nanobind only. pybind11 support exists so that the switch +can happen without a flag day, and so that any behavioral difference shows up +as a test failure rather than as a bug report. + +Layout +------ + +| File | Role | +|---|---| +| `py_backend.h` | The compatibility shim. Everything the two frameworks spell differently lives here. | +| `py_osl.h` | Conversion helpers. Backend-neutral; no `#if` at all. | +| `py_osl.cpp` | The bindings. Backend-neutral except for the module entry point. | +| `__init__.py` | Package init for the single-backend case. | +| `nanobind/__init__.py` | Package init for the second module in `both` mode. | + +Conventions for maintainers +--------------------------- + +- **Use the shim macros**, not the framework's own spellings: + `OSL_PY_RW`, `OSL_PY_PROP_RO`, `OSL_PY_PROP_RW`. +- **Use the `osl_py::` helpers**: `str()`, `make_tuple()`, `make_iterator()`, + `throw_key_error()`. Each exists because a direct call differs between the + frameworks; the reason is commented at each definition. +- **`declare_*()` functions take `py_module&`**, never `py::module&`. +- **Adding a `#if defined(OSL_PY_BACKEND_NANOBIND)` outside `py_backend.h` is + a last resort.** There is currently exactly one, for the module entry point + (`NB_MODULE` must be at global scope; `PYBIND11_MODULE` need not be), and + none at all in `py_osl.h`. If you need another, first try to absorb the + difference into the shim, and if you can't, comment it with the specific + framework difference that forces it. +- **Consumer-visible differences between the backends: none intended.** If + you find one, that is a bug, not a documented quirk -- fix it and add a + regression test. The one exception, which cannot be helped, is that + pybind11 3.x injects a `_pybind11_conduit_v1_` member into every class it + binds; that is a framework interop hook, not part of OSL's API. + +Testing +------- + +`testsuite/python-oslquery` runs once per selected backend, against a single +copy of the test script and a single `ref/out.txt`. The script contains no +backend awareness whatsoever -- it just does `import oslquery`, and +`PYTHONPATH` (set by `src/cmake/testing.cmake`) decides which module that +resolves to. In `both` mode you get `python-oslquery` and +`python-oslquery.nanobind`; with a single backend selected, whichever one it +is takes the plain name. + +Byte-identical output from that one reference file is the equivalence +guarantee. If you add a binding, add coverage for it, and remember that the +interesting cases are often the empty ones -- a `ustring` that is empty or +default-constructed has a null `c_str()`, and that exact hazard once produced +a segfault that the entire testsuite failed to notice. + +Known framework differences encountered so far +---------------------------------------------- + +All of these are already handled; the list is here so the next person doesn't +have to rediscover them. + +| Difference | Handled by | +|---|---| +| `NB_MODULE` must be at global scope; `PYBIND11_MODULE` can be inside a namespace | The one `#if` in `py_osl.cpp` | +| nanobind has no value-returning `py::init(lambda)` | Use the templated `py::init()`, which both accept | +| nanobind's `py::str` has no `std::string` constructor | `osl_py::str()` | +| nanobind's `py::tuple` can't be sized up front and assigned into | `osl_py::make_tuple()` | +| nanobind's `make_iterator` wants a scope type object and a name, and the scope must be a *bound* type | `osl_py::make_iterator()` | +| Both frameworks overload `make_iterator` on (first, last) *and* on a whole container; passing lvalue iterators can make both viable. pybind11 2.10 alone took the pair by forwarding reference, making it genuinely ambiguous | `osl_py::make_iterator()` moves its arguments; see the comment there | +| nanobind's `key_error` takes `const char*`, pybind11's takes `std::string` | `osl_py::throw_key_error()` | +| nanobind's STL casters are opt-in, one header per type | Explicit includes in `py_backend.h`; note that a missing one fails at *runtime*, not compile time | +| nanobind has no `.cast()` member function, only free `py::cast()` | No longer relevant -- the only user was dead code | +| Python 3.9 provokes spurious nanobind leak warnings at shutdown | `py::set_leak_warnings(false)`, version-guarded (wjakob/nanobind#1405) | +| nanobind's CMake wants unversioned `Python::` targets, not `Python3::` | Second `find_package(Python ...)` in `find_python()` | +| nanobind's CMake package config installs somewhere `find_package` won't look | `discover_nanobind_cmake_dir()` and `build_nanobind.cmake` | + +Remaining work +-------------- + +- Flip the default to `nanobind` once it has soaked in CI. +- Remove pybind11 support, and with it `py_backend.h`'s second arm, most of + the `osl_py::` helpers, and the `both` mode plumbing. diff --git a/src/liboslquery/__init__.py b/src/liboslquery/__init__.py index 569b321d64..3104ac290c 100644 --- a/src/liboslquery/__init__.py +++ b/src/liboslquery/__init__.py @@ -15,3 +15,9 @@ from .oslquery import * +# `import *` skips names beginning with an underscore, so the module's dunder +# attributes have to be brought over by hand. Without this, `oslquery.__version__` +# works when importing the extension module straight out of the build tree but +# is missing from an installed OSL, where this package wraps it. +from .oslquery import __version__ + diff --git a/src/liboslquery/nanobind/__init__.py b/src/liboslquery/nanobind/__init__.py new file mode 100644 index 0000000000..c8b5e9e8c6 --- /dev/null +++ b/src/liboslquery/nanobind/__init__.py @@ -0,0 +1,43 @@ +# Copyright Contributors to the Open Shading Language project. +# SPDX-License-Identifier: BSD-3-Clause +# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +# Package init for the nanobind build of the oslquery module, used only when +# OSL_PYTHON_BINDINGS_BACKEND=both and we therefore need two modules to +# coexist. The pybind11 module keeps the plain `oslquery` name, so this one is +# built as `_oslquery` inside a package that re-exports it -- meaning +# `import oslquery` works the same either way, and which one you get depends +# only on what's on sys.path. +# +# When nanobind is the only backend, the module is named `oslquery` outright +# and the ordinary ../__init__.py is used instead of this file. + +import os, sys, platform + +# This works around the python 3.8 change to stop loading DLLs from PATH on Windows. +# We reproduce the old behaviour by manually tokenizing PATH, checking that the directories exist and are not ".", +# then add them to the DLL load path. +# This behaviour can be disabled by setting the environment variable "OSL_LOAD_DLLS_FROM_PATH" to "0" +if sys.version_info >= (3, 8) and platform.system() == "Windows" and os.getenv("OSL_LOAD_DLLS_FROM_PATH", "1") == "1": + for path in os.getenv("PATH", "").split(os.pathsep): + if os.path.exists(path) and path != ".": + os.add_dll_directory(path) + +# MSVC multi-config builds put _oslquery.pyd in a per-configuration subdirectory +# (oslquery/Release/, oslquery/Debug/, ...) while this file stays in oslquery/. +# Extending the package search path here is simpler and less fragile than +# trying to make CMake flatten the output layout. +if platform.system() == "Windows": + _here = os.path.abspath(os.path.dirname(__file__)) + for _cfg in ("Release", "Debug", "RelWithDebInfo", "MinSizeRel"): + _subdir = os.path.join(_here, _cfg) + if os.path.isdir(_subdir) and _subdir not in __path__: + __path__.append(_subdir) + +from ._oslquery import * + +# `import *` skips names beginning with an underscore, so bring the module's +# dunder attributes over by hand -- otherwise `oslquery.__version__` would +# exist in a nanobind-only build (where the extension module is imported +# directly) but not here. See the same note in ../__init__.py. +from ._oslquery import __version__ diff --git a/src/liboslquery/py_backend.h b/src/liboslquery/py_backend.h new file mode 100644 index 0000000000..05412bdc24 --- /dev/null +++ b/src/liboslquery/py_backend.h @@ -0,0 +1,154 @@ +// Copyright Contributors to the Open Shading Language project. +// SPDX-License-Identifier: BSD-3-Clause +// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +// Compatibility shim that lets the OSLQuery Python bindings be compiled +// against either pybind11 or nanobind from a single set of sources. Which +// one you get is selected by the build (OSL_PYTHON_BINDINGS_BACKEND), which +// defines OSL_PY_BACKEND_NANOBIND for the nanobind variant. +// +// Everything the two frameworks spell differently is absorbed here, so that +// py_osl.h and py_osl.cpp stay backend-neutral. Conditional compilation +// elsewhere should be a last resort -- see MIGRATION_STATUS.md. +// +// Do not include this directly; include py_osl.h, which arranges for +// Python.h to come first. + +#pragma once + +#include +#include + +#if defined(OSL_PY_BACKEND_NANOBIND) + +# include +# include +// nanobind's STL type casters are opt-in, one header per type -- unlike +// pybind11, where brings them all in at once. Omitting one +// is not a compile error at the point of use; it fails at runtime when the +// conversion is attempted. stl/vector.h is what turns the +// std::vector returned by `.parameters` and `.metadata` +// into a Python list, and stl/string.h covers std::string arguments and +// returns (e.g. geterror()). +# include +# include + +namespace py = nanobind; +using py_module = nanobind::module_; +using namespace py::literals; + +# define OSL_PY_RW def_rw +# define OSL_PY_PROP_RO def_prop_ro +# define OSL_PY_PROP_RW def_prop_rw + +#else // pybind11 + +# include +# include + +namespace py = pybind11; +using py_module = pybind11::module; +using namespace py::literals; + +# define OSL_PY_RW def_readwrite +# define OSL_PY_PROP_RO def_property_readonly +# define OSL_PY_PROP_RW def_property + +#endif + + +namespace PyOSL { +namespace osl_py { + +// Make a Python str. Python 3 strings are always unicode, so py::str is the +// real thing in both backends. +// +// Use these rather than py::str directly: pybind11's str has a std::string +// constructor and nanobind's does not, and the obvious workaround of passing +// ustring::c_str() is a trap -- that returns nullptr for an empty or +// default-constructed ustring, which crashes inside CPython. ustring::string() +// is null-safe, and so is the const char* overload below. +inline py::str +str(const std::string& s) +{ +#if defined(OSL_PY_BACKEND_NANOBIND) + return py::str(s.c_str(), s.size()); +#else + return py::str(s); +#endif +} + + +inline py::str +str(const char* s) +{ + return py::str(s ? s : ""); +} + + +// Build a tuple of `size` elements, where element i is fill(i). +// +// pybind11 lets you size a tuple up front and then assign into it; nanobind's +// tuple is immutable from C++, so there we accumulate into a list and convert. +template +inline py::tuple +make_tuple(size_t size, F&& fill) +{ +#if defined(OSL_PY_BACKEND_NANOBIND) + py::list list; + for (size_t i = 0; i < size; ++i) + list.append(fill(i)); + return py::steal(PyList_AsTuple(list.ptr())); +#else + py::tuple result(size); + for (size_t i = 0; i < size; ++i) + result[i] = fill(i); + return result; +#endif +} + + +// Make a Python iterator over [first, last). +// +// nanobind additionally wants the type object that the iterator type should +// be scoped to, plus a name for it. Scope must be a *bound* class: passing +// something never registered (std::vector, say) yields a null +// handle. pybind11 derives all of that itself and ignores Scope. +// +// The std::move calls are load-bearing, not an optimization. Both frameworks +// offer two make_iterator overloads: a (first, last) pair, and a whole +// container spelled `make_iterator(Type& value, Extra&&... extra)`. Hand the +// latter an lvalue iterator and it becomes viable too, swallowing `last` into +// `Extra`. Whether that is ambiguous depends on how the pair overload takes +// its arguments: by value (every pybind11 except 2.10, and nanobind) is fine, +// but pybind11 2.10 alone used forwarding references, which deduce to `It&` +// for an lvalue and so tie exactly with `Type&`. Callers passing .begin() and +// .end() directly never see this, because a prvalue cannot bind `Type&`; +// naming them as parameters here is what makes them lvalues, and moving +// restores the value category those overloads were written to expect. +template +inline auto +make_iterator(It first, It last) +{ +#if defined(OSL_PY_BACKEND_NANOBIND) + return py::make_iterator(py::type(), "Iterator", std::move(first), + std::move(last)); +#else + return py::make_iterator(std::move(first), std::move(last)); +#endif +} + + +// nanobind's key_error takes a const char*, pybind11's takes a std::string. +[[noreturn]] inline void +throw_key_error(const std::string& msg) +{ +#if defined(OSL_PY_BACKEND_NANOBIND) + throw py::key_error(msg.c_str()); +#else + throw py::key_error(msg); +#endif +} + +} // namespace osl_py +} // namespace PyOSL diff --git a/src/liboslquery/py_osl.cpp b/src/liboslquery/py_osl.cpp index a4b904a913..ea05c1d15c 100644 --- a/src/liboslquery/py_osl.cpp +++ b/src/liboslquery/py_osl.cpp @@ -4,8 +4,6 @@ #include "py_osl.h" -#include - namespace PyOSL { using namespace OSL; @@ -13,28 +11,36 @@ using namespace OSL; void -declare_oslqueryparam(py::module& m) +declare_oslqueryparam(py_module& m) { - using namespace pybind11::literals; using Parameter = OSLQuery::Parameter; py::class_(m, "Parameter") .def(py::init<>()) .def(py::init()) - .def_property_readonly("name", - [](const Parameter& p) { - return PY_STR(p.name.string()); - }) - .def_readwrite("type", &Parameter::type) - .def_property( + .OSL_PY_PROP_RO("name", + [](const Parameter& p) { + return osl_py::str(p.name.string()); + }) + // NOTE: this exposes an OIIO TypeDesc, and OSL's module never + // registers that type -- it relies on OpenImageIO's Python module + // having registered it. So reading `type` only works if OIIO's module + // has been imported AND was built with the same binding framework as + // this one (each framework has its own type registry and they can't + // see each other's); otherwise it raises TypeError. Both backends + // behave identically here, and this predates the nanobind work. + // `type_name` gives the same information as a plain string with no + // such coupling, and is what callers should prefer. + .OSL_PY_RW("type", &Parameter::type) + .OSL_PY_PROP_RW( "type_name", - [](const Parameter& p) { return PY_STR(p.type_name()); }, + [](const Parameter& p) { return osl_py::str(p.type_name()); }, [](Parameter& p, const std::string& t) { p.type_name(t); }) - .def_readwrite("isoutput", &Parameter::isoutput) - .def_readwrite("varlenarray", &Parameter::varlenarray) - .def_readwrite("isstruct", &Parameter::isstruct) - .def_readwrite("isclosure", &Parameter::isclosure) - .def_property_readonly( + .OSL_PY_RW("isoutput", &Parameter::isoutput) + .OSL_PY_RW("varlenarray", &Parameter::varlenarray) + .OSL_PY_RW("isstruct", &Parameter::isstruct) + .OSL_PY_RW("isclosure", &Parameter::isclosure) + .OSL_PY_PROP_RO( "value", [](const Parameter& p) { py::object result; @@ -50,7 +56,7 @@ declare_oslqueryparam(py::module& m) result = py::none(); return result; }) - .def_property_readonly( + .OSL_PY_PROP_RO( "spacename", [](const Parameter& p) { py::object result; @@ -65,41 +71,36 @@ declare_oslqueryparam(py::module& m) } return result; }) - .def_property_readonly( - "fields", - [](const Parameter& p) { - py::object result; - if (p.isstruct) { - TypeDesc t(TypeDesc::STRING, p.fields.size()); - result = C_to_val_or_tuple(cspan(p.fields), t); - } else { - result = py::none(); - } - return result; - }) - .def_property_readonly("structname", - [](const Parameter& p) { - return PY_STR(p.structname.string()); - }) - .def_property_readonly( - "metadata", [](const Parameter& p) { return p.metadata; }, - py::return_value_policy::reference_internal); + .OSL_PY_PROP_RO("fields", + [](const Parameter& p) { + py::object result; + if (p.isstruct) { + TypeDesc t(TypeDesc::STRING, p.fields.size()); + result = C_to_val_or_tuple(cspan( + p.fields), + t); + } else { + result = py::none(); + } + return result; + }) + .OSL_PY_PROP_RO("structname", + [](const Parameter& p) { + return osl_py::str(p.structname.string()); + }) + .OSL_PY_PROP_RO("metadata", + [](const Parameter& p) { return p.metadata; }); } void -declare_oslquery(py::module& m) +declare_oslquery(py_module& m) { - using namespace pybind11::literals; - py::class_(m, "OSLQuery") .def(py::init<>()) - .def(py::init([](const std::string& shadername, - const std::string& searchpath) { - return OSLQuery(shadername, searchpath); - }), - "shadername"_a, "searchpath"_a = "") + .def(py::init(), "shadername"_a, + "searchpath"_a = "") // OSLQuery (const ShaderGroup *group, int layernum) @@ -124,42 +125,35 @@ declare_oslquery(py::module& m) .def("shadername", [](const OSLQuery& self) { return self.shadername().string(); }) - .def_property_readonly("nparams", - [](const OSLQuery& p) { return p.nparams(); }) - .def_property_readonly( - "parameters", - [](const OSLQuery& self) { return self.parameters(); }, - py::return_value_policy::reference_internal) + .OSL_PY_PROP_RO("nparams", + [](const OSLQuery& p) { return p.nparams(); }) + .OSL_PY_PROP_RO("parameters", + [](const OSLQuery& self) { return self.parameters(); }) - .def_property_readonly( - "metadata", [](const OSLQuery& self) { return self.metadata(); }, - py::return_value_policy::reference_internal) + .OSL_PY_PROP_RO("metadata", + [](const OSLQuery& self) { return self.metadata(); }) .def("__len__", [](const OSLQuery& p) { return p.nparams(); }) - .def( - "__getitem__", - [](const OSLQuery& self, size_t i) { - auto p = self.getparam(i); - if (!p) - throw py::index_error(); - return *p; - }, - py::return_value_policy::reference_internal) - .def( - "__getitem__", - [](const OSLQuery& self, const std::string& name) { - auto p = self.getparam(name); - if (!p) - throw py::key_error("parameter '" + name - + "' does not exist"); - return *p; - }, - py::return_value_policy::reference_internal) + .def("__getitem__", + [](const OSLQuery& self, size_t i) { + auto p = self.getparam(i); + if (!p) + throw py::index_error(); + return *p; + }) + .def("__getitem__", + [](const OSLQuery& self, const std::string& name) { + auto p = self.getparam(name); + if (!p) + osl_py::throw_key_error("parameter '" + name + + "' does not exist"); + return *p; + }) .def( "__iter__", [](const OSLQuery& self) { - return py::make_iterator(self.parameters().begin(), - self.parameters().end()); + return osl_py::make_iterator(self.parameters().begin(), + self.parameters().end()); }, py::keep_alive<0, 1>()) @@ -173,21 +167,59 @@ declare_oslquery(py::module& m) -PYBIND11_MODULE(oslquery, m) +// Global (OSL scope) symbols +void +declare_module_attributes(py_module& m) { - // Global (OSL scope) functions and symbols m.attr("osl_version") = OSL_VERSION; m.attr("VERSION") = OSL_VERSION; - m.attr("VERSION_STRING") = PY_STR(OSL_LIBRARY_VERSION_STRING); + m.attr("VERSION_STRING") = osl_py::str(OSL_LIBRARY_VERSION_STRING); m.attr("VERSION_MAJOR") = OSL_VERSION_MAJOR; m.attr("VERSION_MINOR") = OSL_VERSION_MINOR; m.attr("VERSION_PATCH") = OSL_VERSION_PATCH; - m.attr("INTRO_STRING") = PY_STR(OSL_INTRO_STRING); - m.attr("__version__") = PY_STR(OSL_LIBRARY_VERSION_STRING); + m.attr("INTRO_STRING") = osl_py::str(OSL_INTRO_STRING); + m.attr("__version__") = osl_py::str(OSL_LIBRARY_VERSION_STRING); +} + + - // Main OSL classes +#if defined(OSL_PY_BACKEND_NANOBIND) + +} // namespace PyOSL +// NB_MODULE, unlike PYBIND11_MODULE, must appear at global scope, so the +// namespace has to close before it rather than after. + +// When both backends are built, this module is the second one and lives +// inside a package whose __init__.py re-exports it, so it needs a distinct +// name. When nanobind is the only backend, it is a drop-in replacement for +// the pybind11 module and takes the plain name. +# if defined(OSL_PY_NANOBIND_ISOLATED_PACKAGE) +NB_MODULE(_oslquery, m) +# else +NB_MODULE(oslquery, m) +# endif +{ +# if PY_VERSION_HEX < 0x030a0000 + // Python 3.9 tears the interpreter down in an order that makes nanobind + // report leaks that aren't there. Not an issue on 3.10+. + // https://github.com/wjakob/nanobind/discussions/1405 + py::set_leak_warnings(false); +# endif + + PyOSL::declare_module_attributes(m); + PyOSL::declare_oslqueryparam(m); + PyOSL::declare_oslquery(m); +} + +#else + +PYBIND11_MODULE(oslquery, m) +{ + declare_module_attributes(m); declare_oslqueryparam(m); declare_oslquery(m); } } // namespace PyOSL + +#endif diff --git a/src/liboslquery/py_osl.h b/src/liboslquery/py_osl.h index 56a2802141..c93db5182d 100644 --- a/src/liboslquery/py_osl.h +++ b/src/liboslquery/py_osl.h @@ -18,8 +18,6 @@ #include // clang-format on -#include - // Avoid a compiler warning from a duplication in tiffconf.h/pyconfig.h #undef SIZEOF_LONG @@ -29,15 +27,7 @@ #include -#include -#include -#include -#include -namespace py = pybind11; - - -// Python3 is always unicode, so return a true str -#define PY_STR py::str +#include "py_backend.h" namespace PyOSL { @@ -46,21 +36,7 @@ using namespace OSL; // clang-format off -void declare_oslquery (py::module& m); - - -// bool PyProgressCallback(void*, float); -// object C_array_to_Python_array (const char *data, TypeDesc type, size_t size); -const char * python_array_code (TypeDesc format); -TypeDesc typedesc_from_python_array_code (char code); - - -inline std::string -object_classname(const py::object& obj) -{ - return obj.attr("__class__").attr("__name__").cast(); -} - +void declare_oslquery (py_module& m); template struct PyTypeForCType { }; @@ -72,9 +48,9 @@ template<> struct PyTypeForCType { typedef py::int_ type; }; template<> struct PyTypeForCType { typedef py::float_ type; }; template<> struct PyTypeForCType { typedef py::float_ type; }; template<> struct PyTypeForCType { typedef py::float_ type; }; -template<> struct PyTypeForCType { typedef PY_STR type; }; -template<> struct PyTypeForCType { typedef PY_STR type; }; -template<> struct PyTypeForCType { typedef PY_STR type; }; +template<> struct PyTypeForCType { typedef py::str type; }; +template<> struct PyTypeForCType { typedef py::str type; }; +template<> struct PyTypeForCType { typedef py::str type; }; // clang-format on @@ -84,47 +60,20 @@ template inline py::tuple C_to_tuple(cspan vals) { - size_t size = vals.size(); - py::tuple result(size); - for (size_t i = 0; i < size; ++i) - result[i] = typename PyTypeForCType::type(vals[i]); - return result; + return osl_py::make_tuple(vals.size(), [&](size_t i) { + return typename PyTypeForCType::type(vals[i]); + }); } -template -inline py::tuple -C_to_tuple(const T* vals, size_t size) -{ - py::tuple result(size); - for (size_t i = 0; i < size; ++i) - result[i] = typename PyTypeForCType::type(vals[i]); - return result; -} - - -// Special case for TypeDesc -template<> -inline py::tuple -C_to_tuple(cspan vals) -{ - size_t size = vals.size(); - py::tuple result(size); - for (size_t i = 0; i < size; ++i) - result[i] = py::cast(vals[i]); - return result; -} - // Special case for ustring template<> inline py::tuple C_to_tuple(cspan vals) { - size_t size = vals.size(); - py::tuple result(size); - for (size_t i = 0; i < size; ++i) - result[i] = PY_STR(vals[i].string()); - return result; + return osl_py::make_tuple(vals.size(), [&](size_t i) { + return osl_py::str(vals[i].string()); + }); } @@ -149,7 +98,7 @@ C_to_val_or_tuple(cspan vals, TypeDesc type) { size_t n = type.numelements() * type.aggregate * vals.size(); if (n == 1 && !type.arraylen) - return PY_STR(vals[0].string()); + return osl_py::str(vals[0].string()); else return C_to_tuple(vals); } diff --git a/testsuite/python-oslquery/NOOPTIMIZE b/testsuite/python-oslquery/NOOPTIMIZE deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/testsuite/python-oslquery/NOOPTIX b/testsuite/python-oslquery/NOOPTIX deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/testsuite/python-oslquery/ref/out.txt b/testsuite/python-oslquery/ref/out.txt index af5215e9cc..91ef1f712a 100644 --- a/testsuite/python-oslquery/ref/out.txt +++ b/testsuite/python-oslquery/ref/out.txt @@ -32,4 +32,12 @@ Shader: shader test meta: string s = 'I have "Escape" sequences ' + Empty string properties: + structname of a non-struct param: '' + default Parameter name: '' + default Parameter structname: '' + default Parameter type_name: 'unknown' + default Parameter value: None + default Parameter fields: None + default Parameter spacename: None Done. diff --git a/testsuite/python-oslquery/src/test_oslquery.py b/testsuite/python-oslquery/src/test_oslquery.py index 129155c05a..4101fb70c0 100755 --- a/testsuite/python-oslquery/src/test_oslquery.py +++ b/testsuite/python-oslquery/src/test_oslquery.py @@ -23,11 +23,10 @@ def printparam(p, indent=" ") : "output " if p.isoutput else "", p.name, p.value)) else : - # All other parameter types. Note how we check for output-ness, the - # type is an OpenImageIO::TypeDesc but it can print like a string, - # if the type is a string we surround it with single quotes to make - # it clear. Aggregate types will have their `value` print correctly - # as tuples. + # All other parameter types. Note how we check for output-ness. + # p.type_name is the type as a plain string; if the type is a string + # we surround the value with single quotes to make it clear. + # Aggregate types will have their `value` print correctly as tuples. print (indent, "{}{} {} = {}".format( "output " if p.isoutput else "", p.type_name, p.name, @@ -56,15 +55,24 @@ def printparam(p, indent=" ") : # Iterating over the query object itself is iterating over the # parameters to the shader: print (" Parameters:") - for i in range(len(q)) : - printparam(q[i]) - # FIXME(pybind11): The following way of looping over params should work. - # But on Mac, with a combination of python 3.8/3.9 and pybind11 2.6, it - # crashes. Works with older pybind11, so I think it's a pybind11 bug - # that will get fixed at some point. Try it again later. - # - # for p in q : - # printparam(p) + for p in q : + printparam(p) + + # Properties backed by a ustring that is empty or default-constructed. + # These are worth exercising explicitly because printparam above only + # reaches structname for parameters that are structs, so the empty case + # went untested for years. It matters: ustring::c_str() is NULL for an + # empty ustring, so any binding that converts via a raw char* instead of + # ustring::string() crashes here rather than producing ''. + print (" Empty string properties:") + print (" structname of a non-struct param:", repr(q[0].structname)) + empty = oslquery.Parameter() + print (" default Parameter name:", repr(empty.name)) + print (" default Parameter structname:", repr(empty.structname)) + print (" default Parameter type_name:", repr(empty.type_name)) + print (" default Parameter value:", repr(empty.value)) + print (" default Parameter fields:", repr(empty.fields)) + print (" default Parameter spacename:", repr(empty.spacename)) print ("Done.") except Exception as detail: