Skip to content

Return the declared type from @pybind_lambda adapters - #214

Open
DLuminary wants to merge 1 commit into
borglab:masterfrom
DLuminary:fix/pybind-lambda-declared-return
Open

DLuminary wants to merge 1 commit into
borglab:masterfrom
DLuminary:fix/pybind-lambda-declared-return

Conversation

@DLuminary

Copy link
Copy Markdown

A @pybind_lambda adapter declares the signature its binding should expose, but the emitted lambda deduced its return type from the C++ callable. When that callable returns a non-owning view by value, such as an Eigen block, pybind11 casts it as a view without a parent. The NumPy array then dangles once a temporary owner is destroyed.

GTSAM relies on these adapters to do exactly that conversion. Its linear.i declares JacobianFactor.getA()/getb() as @pybind_lambda gtsam::Matrix/gtsam::Vector, with the comment "The C++ methods return Eigen views, and the wrapper materializes them at the language boundary." The wrapper never did. getA(), getb(), GaussianConditional.d() and HessianFactor.linearTerm() return arrays that don't own their data, and on a temporary they read freed memory: gtsam.JacobianFactor(0, A, b, model).getA() returns garbage. This reproduces with gtsam-develop 4.3.2.dev202610300138, 4.3.0 and 4.2.2.

No interface declaration can express the intended binding today:

  • A direct binding must match the C++ return type, which is the view.
  • @pybind_lambda is the documented way to declare a different signature, but it ignored the declared return type.

Change.

  • By-value @pybind_lambda adapters now state the declared return type, for example [](T* self) -> gtsam::Vector{return self->d();}. This applies to methods, static methods and global functions.
  • Adapters returning const T& keep reference_internal.
  • Void and reference returns, and unannotated lambdas, are unchanged.
  • MATLAB output is unaffected.

Tests. The adapter fixture gains a callable whose C++ returns a View<T>. The expected output and assertions are updated, and the existing test compiles the generated code.

Validation.

  • CI commands: I ran the same steps as CI (uv sync --locked --all-extras --dev, cmake ., uv run pytest tests) on a fresh clone of master with this patch, on Python 3.10, 3.11 and 3.13, plus 3.12 with plain pytest. 137 tests passed on each.
  • Compilers: the adapter test, which compiles the generated code, passes with both GCC and clang.
  • MATLAB: no MATLAB golden files change, and the MATLAB tests pass.
  • Skill guardrails: the change follows .agents/skills/annotate-pybind-adapters. It adds no symbol lists or GTSAM-specific heuristics, MATLAB output is unchanged, and Python names, defaults, policies, docstrings and overloads are preserved.
  • Downstream effect: if an adapter declares a type that cannot be built from what the C++ returns, it now fails to compile, instead of silently returning the C++ type.

Effect on GTSAM.

  • Bindings that change: 31 generated bindings in geometry, inference, linear and navigation, and all four modules compile. Four of them (getA, getb, d, linearTerm) now return owned arrays. The other 27 gain the same type that was deduced before (Key, char, Pose3, Vector6, std::shared_ptr<...>, ...).
  • Scan of every interface file: I wrapped every generated binding in a compile-time check for an Eigen view (Map, Ref, Block, also inside pair, tuple, vector or optional) returned by value. Across gtsam.i, its 22 submodules and gtsam_unstable.i, exactly six bindings hand out views without keeping their owner alive, all in linear: the four above plus GaussianConditional.R()/S(). With this change and the follow-up GTSAM change, none remain. The optional CUDA and MOSEK interfaces were not scanned.
  • Cost: the copy costs 0.1–0.2 µs per call on typical factor blocks (3×6, 6×12) and is proportional to size for large dense blocks; measurements are in the GTSAM PR. C++ callers keep their zero-copy views.

A `@pybind_lambda` adapter declares the signature the binding should expose,
but the emitted lambda deduced its return type from the C++ callable. When
that callable returns a non-owning view by value, such as an Eigen block,
pybind11 casts it as a view with no parent, so the array dangles once a
temporary owner is destroyed. In GTSAM, `GaussianConditional.d()`,
`JacobianFactor.getA()`/`getb()` and `HessianFactor.linearTerm()` are
declared as owning `Vector`/`Matrix` adapters, yet return such arrays.

Adapters that return by value now state the declared return type
explicitly, so the result is converted to it. Adapters that return
`const T&` keep `reference_internal`; void and reference returns and
unannotated lambdas are unchanged. The fixture gains an adapter whose C++
callable returns a view, and the existing test compiles it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant