Skip to content

Feat: Add BaseHostNode #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 14, 2025
Merged

Feat: Add BaseHostNode #202

merged 10 commits into from
Apr 14, 2025

Conversation

jkbmrz
Copy link
Collaborator

@jkbmrz jkbmrz commented Apr 10, 2025

Purpose

Based on the comment in #192, we are missing an abstract class with platform extraction functionality. Adding it limits code repetition in host nodes and centralizes the control over the ImgFrame.Type settings for individual platforms.

Specification

Adding the BaseHostNode, an abstract class with platform extraction functionality, and using it as a parent in all of our host nodes.

Dependencies & Potential Impact

None

Deployment Plan

None

Testing & Validation

Tested by running the depthai-experiments/neural-networks/generic-example using the midas-v2-1:small-256x192 model both on RVC2 and RVC4.

@jkbmrz jkbmrz marked this pull request as ready for review April 10, 2025 13:11
@github-actions github-actions bot added the enhancement New feature or request label Apr 10, 2025
@jkbmrz jkbmrz requested a review from dominik737 April 10, 2025 13:12
@jkbmrz jkbmrz requested a review from klemen1999 April 10, 2025 13:35
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.

Project coverage is 51.96%. Comparing base (5c6073a) to head (2e978e3).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
depthai_nodes/node/base_host_node.py 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   51.80%   51.96%   +0.15%     
==========================================
  Files          81       82       +1     
  Lines        4799     4821      +22     
==========================================
+ Hits         2486     2505      +19     
- Misses       2313     2316       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkbmrz jkbmrz requested a review from dominik737 April 10, 2025 14:04
Copy link
Collaborator

@kkeroo kkeroo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just thinking if we should rename this base class ImageFrameSender to more general name BaseHostNode or something. Because its a general class and not a "sender" and it might come useful in other nodes aswell.

@klemen1999
Copy link
Collaborator

Maybe BaseImgFrameSender or BaseFrameSender? Because it is a base abstract class that all other nodes that send out frames could inherit from, but I wouldn't make it a BaseHostNode because that implies all host nodes inherit from it IMO

@kkeroo
Copy link
Collaborator

kkeroo commented Apr 11, 2025

Ok, but I dont see why other nodes couldnt inherit from this node. _platform attribute could be used in other nodes as well, similar for _frame_type?

@jkbmrz
Copy link
Collaborator Author

jkbmrz commented Apr 11, 2025

Ok, but I dont see why other nodes couldnt inherit from this node. _platform attribute could be used in other nodes as well, similar for _frame_type?

That's true. There is currently no direct usage of the platform parameter in other nodes but there might come more where this will come handy (and they might not output the ImgFrames). I'd thus agree we rename the node to BaseHostNode and use it as a parent class in all of our host nodes. This might also allow us to abstract more things in the future.

@klemen1999
Copy link
Collaborator

klemen1999 commented Apr 11, 2025

Actually I agree yeah, I think it makes sens to make it into an overall base node. Let's make sure every other inherits from it as
well

@dominik737
Copy link
Contributor

dominik737 commented Apr 11, 2025

This might also allow us to abstract more things in the future.

I think having custom base node is good to abstract some very very common things.

On the other hand I would not put too much logic that is not common/true for all our nodes. That would then over time lead to the node being very bulky, unclear and with many responsibilities as opposed to single responsibility.

@jkbmrz jkbmrz force-pushed the feat/img_frame_sender_node branch from 39d6a17 to 2e978e3 Compare April 11, 2025 10:28
@jkbmrz
Copy link
Collaborator Author

jkbmrz commented Apr 11, 2025

I've renamed the node in ab06069 to BaseHostNode. I think this works nicely as it also follows the structure we have for the parsers (inheriting from the BaseParser).

@jkbmrz jkbmrz changed the title Feat: add ImgFrameSender abstract node Feat: Add BaseHostNode Apr 11, 2025
@jkbmrz jkbmrz merged commit c58ed4f into main Apr 14, 2025
12 checks passed
@jkbmrz jkbmrz deleted the feat/img_frame_sender_node branch April 14, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants