feat: allow passing ros2 types in send_message #741
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
==========================================
+ Coverage 65.34% 65.53% +0.18%
==========================================
Files 78 78
Lines 3388 3406 +18
==========================================
+ Hits 2214 2232 +18
Misses 1174 1174 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughChanges expand ROS2 message handling in the API and connectors to support passing pre-formed ROS2Message objects alongside dictionary payloads, making the msg_type parameter optional. Version bumped from 2.6.0 to 2.7.0. Added utility methods for message type classification and comprehensive test coverage for message publishing scenarios and error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rai_core/rai/communication/ros2/api/topic.py (1)
150-158: Update docstring to reflect new parameter type.The docstring for
msg_content(line 154) still describes it as "Dictionary containing the message content", but the parameter now also acceptsIROS2Messageinstances directly.🔎 Suggested docstring update
"""Publish a message to a ROS2 topic. Args: topic: Name of the topic to publish to - msg_content: Dictionary containing the message content + msg_content: ROS2 message instance or dictionary containing the message content msg_type: ROS2 message type as string (e.g. 'std_msgs/msg/String') auto_qos_matching: Whether to automatically match QoS with subscribers qos_profile: Optional custom QoS profile to use
🧹 Nitpick comments (1)
tests/communication/ros2/test_connectors.py (1)
90-96: Type hint formessage_contentis too restrictive.The parametrized cases include both
ROS2Messagewrappers and direct ROS2 message instances (e.g.,String(),Pose()). The type hint should reflect this union to avoid misleading developers and improve IDE/type-checker accuracy.🔎 Suggested fix
+from typing import Any ... def test_ros2_connector_send_message( ros_setup: None, request: pytest.FixtureRequest, - message_content: ROS2Message, + message_content: ROS2Message | Any, # ROS2Message or native ROS2 message instance msg_type: str | None, actual_type: type, ):Alternatively, if
IROS2Messageis the protocol/interface for native ROS2 messages, you could useROS2Message | IROS2Message.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/API_documentation/connectors/ROS_2_Connectors.mdsrc/rai_core/pyproject.tomlsrc/rai_core/rai/communication/ros2/api/base.pysrc/rai_core/rai/communication/ros2/api/topic.pysrc/rai_core/rai/communication/ros2/connectors/base.pytests/communication/ros2/test_api.pytests/communication/ros2/test_connectors.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 379
File: tests/communication/ros2/test_connectors.py:50-50
Timestamp: 2025-01-21T11:24:27.687Z
Learning: In ROS2 connector implementation, topics are typically static and well-discovered before running any method of the API. The discovery mechanism in tests is an edge case, as it's not representative of the typical usage pattern.
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 416
File: src/rai_core/rai/communication/ros2/connectors.py:241-267
Timestamp: 2025-02-10T21:35:25.819Z
Learning: When dealing with ROS2 message conversions, prefer using `rosidl_runtime_py.convert.message_to_ordereddict` directly instead of wrapping it in a utility function to avoid unnecessary abstractions and potential circular imports.
Learnt from: boczekbartek
Repo: RobotecAI/rai PR: 335
File: src/rai/rai/tools/ros/native_actions.py:0-0
Timestamp: 2025-01-08T13:36:27.469Z
Learning: In the RAI codebase, ROS2 messages are converted to string representation to make them more LLM-friendly, as seen in the `GetMsgFromTopic._run` method. This design decision helps in better processing and understanding of the messages by the language model.
📚 Learning: 2025-02-10T21:35:25.819Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 416
File: src/rai_core/rai/communication/ros2/connectors.py:241-267
Timestamp: 2025-02-10T21:35:25.819Z
Learning: Avoid circular imports in the ROS2 connector modules by placing utility functions in dedicated utility modules that don't import from the connector modules.
Applied to files:
docs/API_documentation/connectors/ROS_2_Connectors.mdsrc/rai_core/rai/communication/ros2/connectors/base.py
📚 Learning: 2025-01-21T11:24:27.687Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 379
File: tests/communication/ros2/test_connectors.py:50-50
Timestamp: 2025-01-21T11:24:27.687Z
Learning: In ROS2 connector implementation, topics are typically static and well-discovered before running any method of the API. The discovery mechanism in tests is an edge case, as it's not representative of the typical usage pattern.
Applied to files:
docs/API_documentation/connectors/ROS_2_Connectors.mdsrc/rai_core/rai/communication/ros2/connectors/base.pytests/communication/ros2/test_api.pytests/communication/ros2/test_connectors.py
📚 Learning: 2025-01-08T13:36:27.469Z
Learnt from: boczekbartek
Repo: RobotecAI/rai PR: 335
File: src/rai/rai/tools/ros/native_actions.py:0-0
Timestamp: 2025-01-08T13:36:27.469Z
Learning: In the RAI codebase, ROS2 messages are converted to string representation to make them more LLM-friendly, as seen in the `GetMsgFromTopic._run` method. This design decision helps in better processing and understanding of the messages by the language model.
Applied to files:
docs/API_documentation/connectors/ROS_2_Connectors.mdsrc/rai_core/rai/communication/ros2/api/topic.pysrc/rai_core/rai/communication/ros2/api/base.py
📚 Learning: 2025-01-16T16:40:58.528Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 367
File: src/rai/rai/communication/ros2/api.py:370-373
Timestamp: 2025-01-16T16:40:58.528Z
Learning: The ROS2 high-level API in `src/rai/rai/communication/ros2/api.py` is intentionally designed to be synchronous for simpler usage, without requiring callback handling.
Applied to files:
src/rai_core/rai/communication/ros2/connectors/base.pysrc/rai_core/rai/communication/ros2/api/topic.pytests/communication/ros2/test_api.py
📚 Learning: 2025-01-08T13:42:45.228Z
Learnt from: boczekbartek
Repo: RobotecAI/rai PR: 335
File: src/rai/rai/node.py:0-0
Timestamp: 2025-01-08T13:42:45.228Z
Learning: In the RAI codebase's ROS2 tools that interact with LLMs, all messages (including error messages) from `get_raw_message_from_topic` are intentionally returned as strings. This design pattern ensures consistent string-based communication with the LLM, where successful responses are converted using `str()` and error messages are returned directly as strings. This is demonstrated in the tool implementation where `return str(msg), {}` is used for regular messages and error messages are passed through unchanged.
Applied to files:
src/rai_core/rai/communication/ros2/api/topic.pysrc/rai_core/rai/communication/ros2/api/base.py
📚 Learning: 2025-02-10T21:35:25.819Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 416
File: src/rai_core/rai/communication/ros2/connectors.py:241-267
Timestamp: 2025-02-10T21:35:25.819Z
Learning: When dealing with ROS2 message conversions, prefer using `rosidl_runtime_py.convert.message_to_ordereddict` directly instead of wrapping it in a utility function to avoid unnecessary abstractions and potential circular imports.
Applied to files:
src/rai_core/rai/communication/ros2/api/topic.py
📚 Learning: 2025-04-07T16:16:36.242Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 508
File: src/rai_core/rai/tools/ros2/nav2.py:34-38
Timestamp: 2025-04-07T16:16:36.242Z
Learning: The tools in src/rai_core/rai/tools/ros2/nav2.py are intentionally simplified versions of the more generic ROS2 tools, with a simplified implementation approach that uses global variables.
Applied to files:
tests/communication/ros2/test_api.py
📚 Learning: 2025-01-24T13:42:58.814Z
Learnt from: maciejmajek
Repo: RobotecAI/rai PR: 383
File: tests/tools/ros2/test_topic_tools.py:84-105
Timestamp: 2025-01-24T13:42:58.814Z
Learning: The ROS2 node discovery in the test suite is asynchronous, and the current implementation doesn't wait for topic discovery. This is a known limitation that causes tests like `test_receive_message_tool` and `test_receive_image_tool` to be marked as `xfail`. A proper discovery mechanism with timeout will be implemented in the future to ensure reliable topic communication.
Applied to files:
tests/communication/ros2/test_api.pytests/communication/ros2/test_connectors.py
📚 Learning: 2025-01-30T16:48:57.540Z
Learnt from: rachwalk
Repo: RobotecAI/rai PR: 393
File: tests/communication/sounds_device/test_connector.py:68-77
Timestamp: 2025-01-30T16:48:57.540Z
Learning: When testing audio processing in Python, using mock audio data (e.g., b"mock_audio_data") is preferred over loading real audio files to ensure test reliability, speed, and isolation, unless there's a specific need to test file format compatibility or audio properties.
Applied to files:
tests/communication/ros2/test_connectors.py
🧬 Code graph analysis (2)
src/rai_core/rai/communication/ros2/connectors/base.py (3)
src/rai_core/rai/communication/ros2/api/base.py (1)
IROS2Message(50-53)src/rai_core/rai/communication/ros2/messages.py (1)
ROS2Message(41-42)src/rai_core/rai/communication/ros2/api/topic.py (1)
publish(141-175)
src/rai_core/rai/communication/ros2/api/topic.py (1)
src/rai_core/rai/communication/ros2/api/base.py (3)
IROS2Message(50-53)is_ros2_message(144-145)build_ros2_msg(108-118)
🪛 Ruff (0.14.10)
src/rai_core/rai/communication/ros2/api/topic.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
173-173: Avoid specifying long messages outside the exception class
(TRY003)
tests/communication/ros2/test_api.py
117-117: Unused function argument: ros_setup
(ARG001)
148-148: Unused function argument: ros_setup
(ARG001)
224-224: Unused function argument: ros_setup
(ARG001)
250-250: Unused function argument: ros_setup
(ARG001)
282-282: Unused function argument: ros_setup
(ARG001)
tests/communication/ros2/test_connectors.py
91-91: Unused function argument: ros_setup
(ARG001)
🔇 Additional comments (13)
src/rai_core/pyproject.toml (1)
7-7: LGTM!The minor version bump from 2.6.0 to 2.7.0 appropriately reflects the addition of new backward-compatible functionality (allowing ROS2 message types directly in
send_message).docs/API_documentation/connectors/ROS_2_Connectors.md (1)
36-52: LGTM!The documentation clearly demonstrates both usage patterns:
- Raw ROS2 message with inferred type (lines 41-45)
- Dictionary payload with explicit
msg_type(lines 47-52)This aligns well with the implementation changes and provides good guidance for users.
src/rai_core/rai/communication/ros2/api/base.py (1)
143-153: LGTM!Clean static helper methods that wrap
rosidl_runtime_pyutilities, providing a consistent API surface within the RAI framework. The delegation pattern keeps the implementation simple while centralizing ROS2 type detection logic.src/rai_core/rai/communication/ros2/api/topic.py (1)
166-175: LGTM!The type-aware branching logic correctly handles all cases:
- Direct ROS2 message passthrough
- Dictionary with explicit
msg_typefor building- Appropriate errors for missing type or invalid content
The implementation aligns well with the
IROS2Messageprotocol andbuild_ros2_msghelper.src/rai_core/rai/communication/ros2/connectors/base.py (1)
279-289: LGTM!The logic correctly distinguishes between:
ROS2Message(wrapper class withpayloaddict) → extracts the payload- Raw ROS2 messages (matching
IROS2Messageprotocol) → passed directlyThe delegation to
_topic_api.publishthen handles the type-aware branching consistently.tests/communication/ros2/test_connectors.py (2)
22-44: LGTM!The new imports align well with the parametrized test cases, bringing in the necessary ROS2 message types (
Time,Point,Pose,PoseWithCovariance, etc.) to exercise the expanded message-sending scenarios.
97-110: LGTM!The test body correctly validates the new message-sending behavior:
- Dynamic type handling via
actual_typeparameter- Optional
msg_typefor both wrapped and direct messages- Proper resource cleanup in the finally block
tests/communication/ros2/test_api.py (6)
25-47: LGTM!The new imports appropriately support the expanded test coverage for type classification (
BaseROS2API) and various message types (Point,Pose,PoseStamped,Header,String, etc.).
64-121: Excellent test coverage for type classification utilities.The parametrization thoroughly exercises
is_ros2_message,is_ros2_service, andis_ros2_actionwith a comprehensive set of inputs including edge cases (empty values, nested messages) and correct classification of service request/response instances as messages vs service classes as services.
124-171: LGTM!Well-structured parametrized test that validates both dictionary-based publishing (with explicit
msg_type) and direct ROS2 message publishing. TheAnytype hint formessage_contentcorrectly reflects the flexible input types.
216-246: LGTM!Good extension of the QoS error test to cover both dictionary-based and direct ROS2 message inputs, ensuring consistent error handling across message formats.
249-267: LGTM!Important test case that validates the API correctly requires
msg_typewhen publishing dictionary payloads, which aligns with the PR's design where direct ROS2 messages can infer their type but dictionaries cannot.
270-302: LGTM!Comprehensive edge case coverage for invalid inputs. The test correctly validates that the API rejects non-dict, non-ROS2-message types (tuples, None, plain strings) with appropriate
ValueErrorexceptions, ensuring robust input validation.
| message : T | IROS2Message | ||
| The message to send. Can be a subclass of ROS2Message (payload is a dict) or any ROS2 message. | ||
| target : str | ||
| The target topic name. | ||
| msg_type : str | ||
| The ROS2 message type. | ||
| msg_type : str | None, optional | ||
| The ROS2 message type. If None, the message type will be inferred from the message content. Must be provided if msg_content is a ROS2Message subclass. |
There was a problem hiding this comment.
Minor docstring inconsistency.
Line 271 references msg_content but the parameter is named message. Consider updating for consistency.
🔎 Suggested fix
msg_type : str | None, optional
- The ROS2 message type. If None, the message type will be inferred from the message content. Must be provided if msg_content is a ROS2Message subclass.
+ The ROS2 message type. If None, the message type will be inferred from the message content. Must be provided if message is a ROS2Message subclass (with dict payload).📝 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.
| message : T | IROS2Message | |
| The message to send. Can be a subclass of ROS2Message (payload is a dict) or any ROS2 message. | |
| target : str | |
| The target topic name. | |
| msg_type : str | |
| The ROS2 message type. | |
| msg_type : str | None, optional | |
| The ROS2 message type. If None, the message type will be inferred from the message content. Must be provided if msg_content is a ROS2Message subclass. | |
| message : T | IROS2Message | |
| The message to send. Can be a subclass of ROS2Message (payload is a dict) or any ROS2 message. | |
| target : str | |
| The target topic name. | |
| msg_type : str | None, optional | |
| The ROS2 message type. If None, the message type will be inferred from the message content. Must be provided if message is a ROS2Message subclass (with dict payload). |
🤖 Prompt for AI Agents
In src/rai_core/rai/communication/ros2/connectors/base.py around lines 266 to
271, the docstring uses `msg_content` but the parameter is named `message`;
update the docstring to reference `message` instead of `msg_content` and
rephrase the final sentence to read something like: "Must be provided if message
is a ROS2Message subclass." Ensure the parameter description consistently uses
`message` and matches the function signature.
|
Thanks for adding the context. While I really like that your PR to make RAI friendly to human developers, I think it ought not to make it harder for LLM agents.
If these are true, there's a bit of a tension here. And what should we do ? Looking at the RAI connector layer, I think we could implement a hybrid API (or dual support) to serve both audiences: developers + LLM agents. If you like the idea, find a reference implementation here, including the support to service/action. Basically,
Things you may not like, look for the TODO in code above. Some of the arguments is overloaded and no type safety check. Now, does this hybrid API design violate any principles? Hmmm ...
There are some open source projects like Pydantic which accepts both dicts and model instances in many methods, and LangChain which uses mixins for tool and agent composition. What makes RAI unique is that it combines a few patterns together that aren't often seen in other projects. We're using Protocol for type hints while using introspection to extract parent classes from nested types. We support both dicts and classes at API, connector, and tool layers. And we're working with generated classes that can't be modified, requiring structural typing. |
Purpose
Currently, sending ROS2 messages requires constructing a nested dictionary and passing the message type as a string. This approach has several drawbacks:
Switching to using ROS2 message classes directly eliminates these problems, providing clear, type-checked, and maintainable code. The original idea of passing dictionaries instead of real message objects was intended to make it easier for AI tools or LLMs to generate code without needing to import message definitions. However, with these changes (especially if call_service and start_action are updated to support the same simplifications) RAI will significantly lower the entry barrier for AI developers who have less familiarity with ROS 2, enabling them to work with messages, services, and actions in a straightforward way.
Reducing boilerplate:
Proposed Changes
Allow passing IROS2Message into send_message method. Msg cls validation by rosidl_runtime_py
Issues
TBD: Implementation for messages is easy, implementation for services and actions is hard. We are currently passing a dictionary and msg_type e.g. "std_srvs/srv/SetBool". Then we use the msg_type to create a Request cls and fill it with the passed dictionary.
A simpler api, coherent with new send_messages should allow:
But to create a service client we also need to know the underlying base class (SetBool, not a SetBool.Request). Since SetBool.Request is not linked to SetBool, we will either need to keep passing msg_type via a string/cls or hack our way out with some magic.
Testing
Extended test suite
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.