Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
+ Coverage 66.31% 70.23% +3.91%
==========================================
Files 78 82 +4
Lines 3423 3534 +111
==========================================
+ Hits 2270 2482 +212
+ Misses 1153 1052 -101 ☔ View full report in Codecov by Sentry. |
Juliaj
changed the title
Juliaj
force-pushed
the
jj/feat/3dpipe_and_usability
branch
from
7aca530 to
09674aa
Compare
Juliaj
force-pushed
the
jj/feat/3dpipe_and_usability
branch
from
32d8802 to
4af6d1b
Compare
|
Regarding rai_sim and rai_bench, I don't think a lot of changes will be required. Mainly naming changes, switching to new tools or changing paths. For example in |
|
I will start with coderabbit to speed things up. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
WalkthroughMajor architectural refactoring and expansion of the RAI perception module introducing new algorithms, services, and components alongside deprecation of legacy agents. Updates include new ROS2 communication utilities, timeout decorators, comprehensive gripping-point extraction pipeline, model registries, and extensive test coverage. Multiple packages reorganized with service-oriented architecture replacing direct agent instantiation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…to run both versions
@jmatejcz thanks for help look into this. The current code changes are back-compat by default. Once we have this merged, I will look into migrate existing code to the new service names. |
src/rai_extensions/rai_perception/rai_perception/components/exceptions.py
Outdated
Show resolved
Hide resolved
@jmatejcz, thanks for looking into this. The testing I have done is listed in the PR description. A few additional things perhaps
|
|
@Juliaj I tested V1 and V2 manipulaiton demo with same prompts. They both work and results are the same. |
There was a problem hiding this comment.
I did a deeper dive into the changes. Thank you for the documents you wrote on the design - they are great source of knowlegde and also give an idea about your approach to this refactor.
In my opition the layered approach is a very good idea. The tools/ dir is very easy to use as it supposed to be.
Also the components/ are designed and self explenatory. I like the idea with presets, especially with the comments which is best for what.
These changes for sure reduce friction for new users and I love the abstraction level that they introduce. I tried myself using some of this code and I didn't have to even look at algorithms/ or implementations of components to use the module.
Now i can see the issue that rest of rai modules are not organized in such a way, which might confuse user, but I guess we can refactor them in the future too ;p
| """Abstract method - must be implemented by subclasses.""" | ||
| raise NotImplementedError("Subclasses must implement _run method") | ||
|
|
||
| def _get_detection_service_name(self) -> str: |
There was a problem hiding this comment.
I started my dive into this refactor with the tools/ dir as it is highest tier layer in this API.
If I understand correctly from the documents you wrote and the code, now switching detection models is possible by setting ros param.
In such case i would rename the base tool and its varaibles, as sticking with "GroundingDino..." names suggest to user that this work only with Grounding dino model. I think renaming to DetectionBaseTool etc. would increase role expressiveness
There was a problem hiding this comment.
The same applies to other fragments in code when "dino..." naming is used , for example "dino"node"
There was a problem hiding this comment.
@jmatejcz, thanks a bunch for the feedback. This is a good catch.
In addtion to this module, a few other modules (components/gripping_points.py, tools/segmentation_tools.py) are also tightly coupled to GroundingDINO specifics, e.g. dardcoded RAIGroundingDino service type, and model-specific parameters (box_threshold, text_threshold). Overall, the refactoring change needs a careful evaluation.
Given the current PR scope, I've added a note for this in MIGRATION.md under "Generic Detection Tools Abstraction" so that we may address in a follow-up PR after merge. Hope you are okay with deferring this.
There was a problem hiding this comment.
okay, so the main problem is that interface of grounding dino differs from other models?
And now we can't actually use any model besides GD, but if rai interfaces are added for other models, this refactor provides a fundamentals for next refactor?
There was a problem hiding this comment.
because now, parts of code enable model switching, but it does not actually work yet yes?
There was a problem hiding this comment.
Correct. This PR laid out the foundation for model switching. For example, the DetectionService has infrastructure for model switching. In base_vision_service.py, following method reads the parameter and initialize the model,
But the service interface is still GroundingDINO-specific, so it cannot actually switch to other models yet. In addition, tools remain tightly coupled to GroundingDINO's interface.
There was a problem hiding this comment.
okay, that you for clarification. Do you think we should leave some note , that only GD is currently supported? in this method above for example
There was a problem hiding this comment.
@Juliaj thank you for this PR and clarification. I approve it, you could add the note or something add we are done ;p
There was a problem hiding this comment.
Thanks @jmatejcz! I've updated module comments across relevant files to clarify that model switching is not yet fully implemented and to document the current coupling to GroundingDINO/Grounded SAM interfaces, along with future directions.
I evaluated the effort required for true model-agnostic refactoring (generic service interfaces). It's medium to high complexity due to parameter abstraction, backward compatibility, and refactoring across services, tools, and components.
As an alternative, we can use model-specific services and tools per model, which may be simpler and faster to implement but may result in some code duplication. I've updated MIGRATION.md with the trade-offs between these approaches.
| ) | ||
|
|
||
|
|
||
| def _publish_gripping_point_debug_data( |
There was a problem hiding this comment.
it shouldn't be "private" method I believe
There was a problem hiding this comment.
Thanks for the suggestion.
I'm neutral on this. Currently, developers can access this via GetObjectGrippingPointsTool after setting debug=True in the tool's input, which publishes the debug data automatically.
Making it public would allow standalone reuse of the visualization logic, but since it's a debug-only utility with performance overhead, keeping it private help avoid committing to a debug-only public API. What do you think ?
There was a problem hiding this comment.
If the intention is to use it only in GetObjectGrippingPointsTool then it should be moved to get_gripping_points_tool.py. i think it breaks encapsulation and is a bit weird to have a private method in file that is not even used in the same module.
But it can be made "public" if the intention is for user to use it. What do you think?
3 participants
Purpose
Proposed Changes
Code refactor based on Rethinking Usability and API Usability Design Considerations.
There are breaking changes introduced, see section below for details. To reduce the impact on applications using the old service names, a legacy service names flag (
enable_legacy_service_names) has been introduced and the default behavior is backward compatible (defaults totrue). For new applications using only the new service names (/detection,/segmentation), you can disable legacy names by setting the flag tofalse:Via launch file argument:
Via environment variable:
Via ROS2 parameter (in code):
Migration Guide
Issues
Testing
To start manipulation-streamlit with both versions
Summary by CodeRabbit
New Features
Documentation
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.