Are DepthAI Core Contributions Welcome?

TheHiddenWaffle

Situation is this:
Because of some convoluted stuff that I'm doing(using pybind11 embedded module to allow C++ to call out to a python method that builds a pipeline) I find myself replicating functionality from depthai nodes in C++. With that I'm experiencing frustration that dai::node::NeuralNetwork is so tightly coupled with NNArchives and the Zoo, such that it's difficult to write a re-implementation of depthai_nodes.ParsingNeuralNetwork.build(…) here without fully replicating the logic of all the various signatures for dai::node::NeuralNetwork::build(…). This would not be a problem if NeuralNetwork provided something like const NNArchive& getNNArchive() to get an immutable reference to the owned archive, allowing you to delegate the creation/homogenization of the archive to the respective overload signatures of NeuralNetwork::build(….) and then simply reading off the archive created by NN::build() at this location's C++ parody. I guess I say all that to ask:

Does Luxonis accept any PR's from externally for DepthAI Core? I don't see any contributing guidelines.
If the answer to the above is yes, what is the scope of what would be accepted for DepthAI? Only very simple tweaks like the above or is there a possibility for greater contribution(possibly after a design discussion with a Luxonis dev)?

I realize I can just copy paste the code from core but that seems inelegant and not future proofed.

TheHiddenWaffle

Another midscope PR that I would propose is the ~15-20 lines of cmake it took to build a new target depthai::embed with the embedded module that allows for external projects to link to it and simply add [[maybe_unused]] ::pybind11::detail::embedded_module pybind11_module_depthai("depthai", pybind11_init_impl_depthai); . This allows them to not include python path or any other external python setup in order to run a script that imports DepthAI.

MatevzMorato

Hi @TheHiddenWaffle, great to hear you'd be interested in contributing!

We do generally accept contributions from the community, but as you mentioned, it's best to sync with the us first on for any larger efforts to make sure they are aligned with our plans. Best place to do so is through a Github issue.

To go into the specific proposal, adding const NNArchive& getNNArchive() is fine, though I would go with making the return value optional, since the NeuralNetwork is not guaranteed to have a NNArchive internally.

For the Pybind PR - does this mean you can build a C++ app, which would then be able to delegate the pipelline building to Python for example? Sounds like a niche, but quite interesting. As long as it doesn't break any existing usecases, we're also open for a PR.

TheHiddenWaffle

MatevzMorato That's wonderful to hear! I did end up just doing it locally after I posted this and ended up with a return of std::optional<std::reference_wrapper<const NNArchive>> after discovering the joy of C++ type assertions not being included in IDE type hints when my first try was std::optional<const NNArchive&> so I'll submit that momentarily.

For the Pybind PR - does this mean you can build a C++ app, which would then be able to delegate the pipelline building to Python for example? Sounds like a niche, but quite interesting. As long as it doesn't break any existing usecases, we're also open for a PR.

On this subject, yes that's exactly what this accomplishes, I actually ended up with a working solution for DepthAI V2 a long while back in this thread but I didn't take it far enough(because the embedded module wasn't included in the binary you had to include the whole DepthAI project inside your dependent project). When V3 was released I started pouring over the source code and various cmake settings and noticed that the team had already added a switch to build with DEPTHAI_PYTHON_EMBEDDED_MODULE, so I didn't have to change nearly as much this time around. What does the DepthAI team use the embedded module for? It feels worthless to anyone but me? Not complaining just very curious.

My thought would be to add a second switch after the first like the following:

cmake_dependent_option(DEPTHAI_PYTHON_EMBEDDED_MODULE_TARGET "Build an additional target which includes the embedded module" OFF DEPTHAI_PYTHON_EMBEDDED_MODULE OFF)

to disable it by default always, and make it dependent on DEPTHAI_PYTHON_EMBEDDED_MODULE.

One major caveat that I've found for building the pipeline in python: you can't use depthai_nodes in your pipeline unless you plan on keeping the Python interpreter alive during the entire lifetime of your C++ pipeline. Looking back on it this seems fairly obvious considering depthai_nodes is python not pybind and pure python objects/subclasses would be torn down as the interpreter comes down, but at first the very quiet SIGSEGV occurring when I called pipeline.start() was very confusing.

I find myself replicating functionality from depthai nodes in C++.

And that brings us full circle to why I'm doing this.

MatevzMorato

TheHiddenWaffle What does the DepthAI team use the embedded module for? It feels worthless to anyone but me? Not complaining just very curious.

Great question and glad it helped you!
We needed it make script node work inside the firmware in RVC4 as we had duplicated bindings for it on RVC2 and managed to avoid it by embedding DepthAI bindings directly on RVC4.

TheHiddenWaffle One major caveat that I've found for building the pipeline in python: you can't use depthai_nodes in your pipeline unless you plan on keeping the Python interpreter alive during the entire lifetime of your C++ pipeline. Looking back on it this seems fairly obvious considering depthai_nodes is python not pybind and pure python objects/subclasses would be torn down as the interpreter comes down, but at first the very quiet SIGSEGV occurring when I called pipeline.start() was very confusing.

I find myself replicating functionality from depthai nodes in C++.

And that brings us full circle to why I'm doing this.

Makes sense that it happens, interesting.
We do plan to start to port the depthai-nodes selectively over time, but we don't have any time-lines yet.

We plan to integrate the nodes that are the most requested first, so if you don't mind letting us know which parsers are you using that'd be helpful.

TheHiddenWaffle

MatevzMorato Great question and glad it helped you!
We needed it make script node work inside the firmware in RVC4 as we had duplicated bindings for it on RVC2 and managed to avoid it by embedding DepthAI bindings directly on RVC4.

Very interesting, thanks for the insight! If that's the case I think there are some tweaks I should do to my branch to ensure RVC4 doesn't perceive any changes, I'll work on that.

MatevzMorato We plan to integrate the nodes that are the most requested first, so if you don't mind letting us know which parsers are you using that'd be helpful.

I appreciate it, though unfortunately I already had to write my own parser: I'm using RTMPose3D which outputs SimCC maps for 133 keypoints in 3 dimensions so I basically just had to rewrite most of KeypointParser as SimCCKeypointParser.

Speaking of which, I don't know where this should go but I found this useful hack:

# Allows depthai_nodes to find our custom class in its global() call

depthai_nodes.parser_generator.__dict__["SimCCKeypointParser"] = SimCCKeypointParser

TheHiddenWaffle

MatevzMorato We plan to integrate the nodes that are the most requested first, so if you don't mind letting us know which parsers are you using that'd be helpful.

On this, I've replicated some of the base classes in depthai_nodes for my purposes(BaseParser, KeypointParser, ParsingNeuralNetwork, ParserGenerator). It wouldn't be a problem for me to tack on a pybind module in cmake that feeds into the existing depthai_nodes repo. Would this be helpful or just a pain for the team to maintain till they start converting them in earnest?

MatevzMorato

Out of curiosity - how come you went with depthai-nodes, considering you had to write your own parser anyways and did not write it in C++ directly?

TheHiddenWaffle

MatevzMorato Mostly just because I wanted to follow the direction that Luxonis was taking and reuse some chunks of code. May have been a bit idealistic considering my odd use case 😄

MatevzMorato

@TheHiddenWaffle do you mean you implemented those classes in C++?
We'd be very happy with a PR if that's the case, no need to add bindings for the initial feedback to see if it's aligned with our plans.

TheHiddenWaffle

MatevzMorato correct yes, implemented in C++. I tried to keep most of the logic and methodology the same but I did make some minor design decisions that suited my purposes/allowed me to better pretend that I was writing in Rust(I have dramatically more experience in Rust) so you're certainly right about needing feedback. Will submit a PR