Remove implicit node instantiation in Python#1763
Conversation
📝 WalkthroughWalkthroughPython bindings for node types are refactored to remove implicit pipeline creation from constructors. Constructors now either accept a device parameter, are parameterless, or no-argument variants, with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/python/src/pipeline/PipelineBindings.cpp (1)
292-304:⚠️ Potential issue | 🔴 CriticalNarrowed catch leaks thread-local pipeline state on non-
runtime_errorexceptions.Replacing
catch(...)withcatch(std::runtime_error&)means exceptions that don't inherit fromstd::runtime_errorskip the cleanup calls, leaking TLS state:
py::error_already_set(Python exceptions from__init__or subclass code) — does NOT inherit fromstd::runtime_error; it's a separate pybind11 exception hierarchy.py::cast_error(cast failures inpy::cast<std::shared_ptr<node::ThreadedHostNode>>(...)) — also separate fromstd::runtime_error.- Standard exceptions like
std::invalid_argument,std::bad_alloc, etc.Any of these will skip
delCreatingNodeFromPipelineCreate()anddelImplicitPipeline(), corrupting the implicit-pipeline TLS and "creating node" flag for all subsequentPipeline::create()calls in the same thread.Catch
std::exception(or reinstatecatch(...)with rethrow) to ensure cleanup always runs. Even better, wrap the two TLS flags in a small RAII guard to make this leak-proof on any future refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/python/src/pipeline/PipelineBindings.cpp` around lines 292 - 304, The catch block around py::cast and class_ currently only handles std::runtime_error, which allows other exceptions (py::error_already_set, py::cast_error, std::bad_alloc, etc.) to skip calling delCreatingNodeFromPipelineCreate() and delImplicitPipeline(), leaking TLS state; replace the narrow catch with either a catch(const std::exception&) (or reinstate catch(...) and rethrow) so cleanup always runs, or better yet introduce a small RAII guard object around setImplicitPipeline(&p) / setCreatingNodeFromPipelineCreate() that calls delCreatingNodeFromPipelineCreate() and delImplicitPipeline() in its destructor to guarantee cleanup on all exception paths (references: setImplicitPipeline, setCreatingNodeFromPipelineCreate, delCreatingNodeFromPipelineCreate, delImplicitPipeline, py::cast, class_).bindings/python/src/pipeline/node/StereoDepthBindings.cpp (1)
86-99: 🧹 Nitpick | 🔵 TrivialDocument that
StereoDepth.build(autoCreateCameras=True)requires the node to already belong to a pipeline.The C++ implementation correctly uses
getParentPipeline()(which throws"Pipeline is null"if the parent is null) to create Camera nodes. However, the Python bindings do not clearly document that callingbuild(autoCreateCameras=True)on an unattached node (e.g.,dai.node.StereoDepth()created directly instead of viapipeline.create(StereoDepth)) will raise a runtime exception. The C++ header mentions "this API is global" but should explicitly clarify the pipeline attachment requirement in both C++ and Python documentation to prevent user confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/python/src/pipeline/node/StereoDepthBindings.cpp` around lines 86 - 99, The Python binding for StereoDepth.build does not warn that calling build(autoCreateCameras=True) requires the StereoDepth node to already belong to a Pipeline and will raise a runtime error via getParentPipeline() if it is unattached; update the binding docstring/documentation for StereoDepth.build (both overloads exposed in StereoDepthBindings.cpp) to explicitly state that autoCreateCameras=True requires the node to be created via pipeline.create(...) or otherwise attached to a Pipeline (and that getParentPipeline() will throw "Pipeline is null" if not), so users know to attach the node before calling build with autoCreateCameras enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/pipeline/node/ColorCameraBindings.cpp`:
- Line 94: The binding for ColorCamera uses py::init() which is inconsistent
with the rest of the PR; update the constructor registration in
ColorCameraBindings.cpp by replacing the call to colorCamera.def(py::init())
with the angle-bracket form colorCamera.def(py::init<>()) so it matches other
bindings like VideoEncoderBindings.cpp and StereoDepthBindings.cpp.
In `@bindings/python/src/pipeline/node/VideoEncoderBindings.cpp`:
- Line 77: The binding uses py::init<>() but VideoEncoder inherits only
protected constructors from DeviceNodeCRTP so the default constructor is not
public; either add a public default constructor to VideoEncoder (make
VideoEncoder() public and call the base protected constructors as needed) or
replace py::init<>() with a factory-based initializer in the pybind binding
(wrap creation in a lambda/factory that returns a new VideoEncoder or
unique_ptr<VideoEncoder>), referencing VideoEncoder, DeviceNodeCRTP and
py::init<>() so the binding compiles without needing a public base constructor.
---
Outside diff comments:
In `@bindings/python/src/pipeline/node/StereoDepthBindings.cpp`:
- Around line 86-99: The Python binding for StereoDepth.build does not warn that
calling build(autoCreateCameras=True) requires the StereoDepth node to already
belong to a Pipeline and will raise a runtime error via getParentPipeline() if
it is unattached; update the binding docstring/documentation for
StereoDepth.build (both overloads exposed in StereoDepthBindings.cpp) to
explicitly state that autoCreateCameras=True requires the node to be created via
pipeline.create(...) or otherwise attached to a Pipeline (and that
getParentPipeline() will throw "Pipeline is null" if not), so users know to
attach the node before calling build with autoCreateCameras enabled.
In `@bindings/python/src/pipeline/PipelineBindings.cpp`:
- Around line 292-304: The catch block around py::cast and class_ currently only
handles std::runtime_error, which allows other exceptions
(py::error_already_set, py::cast_error, std::bad_alloc, etc.) to skip calling
delCreatingNodeFromPipelineCreate() and delImplicitPipeline(), leaking TLS
state; replace the narrow catch with either a catch(const std::exception&) (or
reinstate catch(...) and rethrow) so cleanup always runs, or better yet
introduce a small RAII guard object around setImplicitPipeline(&p) /
setCreatingNodeFromPipelineCreate() that calls
delCreatingNodeFromPipelineCreate() and delImplicitPipeline() in its destructor
to guarantee cleanup on all exception paths (references: setImplicitPipeline,
setCreatingNodeFromPipelineCreate, delCreatingNodeFromPipelineCreate,
delImplicitPipeline, py::cast, class_).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a24d33c8-af93-4407-b806-1a3998ce47d8
📒 Files selected for processing (6)
bindings/python/src/pipeline/PipelineBindings.cppbindings/python/src/pipeline/node/ColorCameraBindings.cppbindings/python/src/pipeline/node/DetectionNetworkBindings.cppbindings/python/src/pipeline/node/StereoDepthBindings.cppbindings/python/src/pipeline/node/VideoEncoderBindings.cppinclude/depthai/pipeline/node/DetectionNetwork.hpp
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-16T11:17:12.819Z
Learnt from: pheec
Repo: luxonis/depthai-core PR: 1715
File: include/depthai/pipeline/node/PointCloud.hpp:34-98
Timestamp: 2026-03-16T11:17:12.819Z
Learning: In `include/depthai/pipeline/node/PointCloud.hpp`, the nested `Impl` class inside `PointCloud` is intentionally public (not private/forward-declared) because the on-host unit tests in `tests/src/onhost_tests/point_cloud_test.cpp` directly access `Impl` methods such as `setIntrinsics`, `setExtrinsics`, `useCPU`, `useCPUMT`, `computePointCloudDense`, `applyTransformation`, and `filterValidPoints`. Do not suggest making `Impl` private or forward-declaring it.
Applied to files:
bindings/python/src/pipeline/PipelineBindings.cpp
📚 Learning: 2026-03-24T22:39:00.650Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:662-662
Timestamp: 2026-03-24T22:39:00.650Z
Learning: In `src/pipeline/Pipeline.cpp`, the `DEPTHAI_AUTOCALIBRATION` environment variable intentionally defaults to `"ON_START"` (not `""`). This means auto-calibration is initialized by default for stereo pipelines at pipeline build time. The resulting implicit EEPROM/calibration writes on startup are by design and should not be flagged as a backward-incompatible change or unintended side-effect in future reviews.
Applied to files:
bindings/python/src/pipeline/node/StereoDepthBindings.cpp
📚 Learning: 2026-04-16T01:04:34.569Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1753
File: src/pipeline/Pipeline.cpp:834-839
Timestamp: 2026-04-16T01:04:34.569Z
Learning: In `src/pipeline/Pipeline.cpp` (`PipelineImpl::build()`), when `autoCalibrationModeSetByApi` is false, `DEPTHAI_AUTOCALIBRATION` is read with an empty-string default (`""`). If the env var is unset or explicitly empty, `autoCalibrationMode` falls back to the pipeline-configured default (`ON_START`). This means an explicitly empty env var no longer disables auto-calibration — it simply defers to the API/default mode. This behavior is intentional and should not be flagged in future reviews.
Applied to files:
bindings/python/src/pipeline/node/StereoDepthBindings.cpp
🔇 Additional comments (2)
include/depthai/pipeline/node/DetectionNetwork.hpp (1)
23-23: LGTM —expliciton single-arg constructor.Good practice; prevents unintended implicit conversions from
std::shared_ptr<Device>toDetectionNetwork. Consistent with the PR intent to require explicit construction.bindings/python/src/pipeline/node/DetectionNetworkBindings.cpp (1)
129-129: Remove thepy::init<Device>binding from DetectionNetwork.The binding violates the PR's design principle. The commit message explicitly states implicit node creation should be removed except for HostNodes. DetectionNetwork is a DeviceNodeGroup, not a HostNode. Other DeviceNodeGroup nodes (ToF, SpatialDetectionNetwork) lack this binding, making DetectionNetwork an inconsistent exception. Direct construction via
dai.node.DetectionNetwork(device)would bypass pipeline registration, defeating the new design. Remove the binding at line 129 and rely solely onpipeline.create(DetectionNetwork).> Likely an incorrect or invalid review comment.
| auto camera = getImplicitPipeline()->create<ColorCamera>(); | ||
| return camera; | ||
| })) | ||
| colorCamera.def(py::init()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: style consistency — prefer py::init<>().
Other bindings in this PR (VideoEncoderBindings.cpp, StereoDepthBindings.cpp) use py::init<>() with empty angle brackets. Using py::init() here is functionally equivalent in pybind11 but inconsistent with the rest of the PR.
🎨 Proposed nit
- colorCamera.def(py::init())
+ colorCamera.def(py::init<>())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| colorCamera.def(py::init()) | |
| colorCamera.def(py::init<>()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/pipeline/node/ColorCameraBindings.cpp` at line 94, The
binding for ColorCamera uses py::init() which is inconsistent with the rest of
the PR; update the constructor registration in ColorCameraBindings.cpp by
replacing the call to colorCamera.def(py::init()) with the angle-bracket form
colorCamera.def(py::init<>()) so it matches other bindings like
VideoEncoderBindings.cpp and StereoDepthBindings.cpp.
| }), | ||
| VIDEO_ENCODER_BUILD_PYARGS, | ||
| VIDEO_ENCODER_PYARGS) | ||
| .def(py::init<>()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm VideoEncoder has an accessible default constructor.
ast-grep --pattern $'class VideoEncoder $_ {
$$$
VideoEncoder();
$$$
}'
rg -nP -C2 '\bVideoEncoder\s*\(\s*\)\s*(;|\{|=)' include/ src/Repository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Find VideoEncoder class definition
fd -type f -name "*.hpp" -o -name "*.h" | xargs rg -l "class VideoEncoder"
# Locate and read VideoEncoderBindings.cpp context
fd -type f -name "VideoEncoderBindings.cpp"Repository: luxonis/depthai-core
Length of output: 469
🏁 Script executed:
# Search more broadly for VideoEncoder in header files
rg "class VideoEncoder" -A 10 --max-count=3Repository: luxonis/depthai-core
Length of output: 1056
🏁 Script executed:
# Check the binding file to understand full context
wc -l bindings/python/src/pipeline/node/VideoEncoderBindings.cppRepository: luxonis/depthai-core
Length of output: 126
🏁 Script executed:
# Read the full VideoEncoderBindings.cpp file to see context
cat -n bindings/python/src/pipeline/node/VideoEncoderBindings.cppRepository: luxonis/depthai-core
Length of output: 8973
🏁 Script executed:
# Read the VideoEncoder class definition to understand constructors
cat -n include/depthai/pipeline/node/VideoEncoder.hppRepository: luxonis/depthai-core
Length of output: 5058
🏁 Script executed:
# Check the base class DeviceNodeCRTP to understand inherited constructors
rg "class DeviceNodeCRTP" -A 30 include/Repository: luxonis/depthai-core
Length of output: 2542
🏁 Script executed:
# Check the actual constructor definitions in DeviceNodeCRTP
rg "DeviceNodeCRTP\s*\(" include/depthai/pipeline/DeviceNode.hpp -A 3 -B 3Repository: luxonis/depthai-core
Length of output: 1237
🏁 Script executed:
# Also check what ADD_NODE macro does
rg "define ADD_NODE" bindings/python/src/ -A 5Repository: luxonis/depthai-core
Length of output: 1091
🏁 Script executed:
# Check if there's a default constructor template in DeviceNodeCRTP
cat -n include/depthai/pipeline/DeviceNode.hpp | head -80Repository: luxonis/depthai-core
Length of output: 2800
🏁 Script executed:
# Find the addNode template implementation
rg "template.*addNode" bindings/python/src/ -A 20Repository: luxonis/depthai-core
Length of output: 46
🏁 Script executed:
# Look at NodeBindings.hpp to see how ADD_NODE works
cat -n bindings/python/src/pipeline/node/NodeBindings.hpp | head -100Repository: luxonis/depthai-core
Length of output: 1125
🏁 Script executed:
# Look for the addNode function implementation
find bindings/python/src -name "*.hpp" -o -name "*.cpp" | xargs rg -l "addNode"Repository: luxonis/depthai-core
Length of output: 108
🏁 Script executed:
# Check the Common.hpp file which defines ADD_NODE
cat -n bindings/python/src/pipeline/node/Common.hpp | head -150Repository: luxonis/depthai-core
Length of output: 3066
Line 77 will fail to compile: py::init<>() cannot access the protected default constructor.
VideoEncoder inherits constructors from DeviceNodeCRTP via using DeviceNodeCRTP::DeviceNodeCRTP;, but all constructors in DeviceNodeCRTP are protected, not public. Pybind11's py::init<>() requires a public constructor to expose in Python. This binding will result in a C++ compilation error.
Either provide a public default constructor in VideoEncoder or use a custom factory-based initialization pattern (e.g., a lambda that wraps the pipeline creation logic).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/pipeline/node/VideoEncoderBindings.cpp` at line 77, The
binding uses py::init<>() but VideoEncoder inherits only protected constructors
from DeviceNodeCRTP so the default constructor is not public; either add a
public default constructor to VideoEncoder (make VideoEncoder() public and call
the base protected constructors as needed) or replace py::init<>() with a
factory-based initializer in the pybind binding (wrap creation in a
lambda/factory that returns a new VideoEncoder or unique_ptr<VideoEncoder>),
referencing VideoEncoder, DeviceNodeCRTP and py::init<>() so the binding
compiles without needing a public base constructor.
aljazkonec1
changed the title
aljazkonec1
changed the title
aljazkonec1
changed the title
|
The ImplicitPipeline mechanism was not removed due to change in behavior in certain depthai-nodes where the pipeline is needed at initializaiton. If we fully remove the mechanism we need to either compensate it with another mechanism that gets the pipeline or update depthai-nodes and oak-examples (+ there is a posibility this would break user nodes). |
|
@aljazkonec1 based on comment above, should we just close this PR? |
2 participants
Purpose
This PR removes the following deprecated feature:
The only exception is (Threaded)HostNodes as they might require implicit pipeline in their init() methods as in the case of ParsingNeuralNetwork:
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
Tested by running a couple of oak-examples.
Summary by CodeRabbit
Bug Fixes
API Changes