feat: migrating ros2 tools #385
Conversation
|
@coderabbitai full review |
WalkthroughThis pull request introduces significant enhancements to the ROS2 communication and tooling infrastructure. The changes primarily focus on expanding the Changes
Sequence DiagramsequenceDiagram
participant Connector as ROS2ARIConnector
participant TopicAPI as Topic API
participant TransformListener as Transform Listener
Connector->>TopicAPI: get_topics_names_and_types()
TopicAPI-->>Connector: Return list of topic names and types
Connector->>TransformListener: get_transform(target_frame, source_frame)
TransformListener-->>Connector: Return TransformStamped
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (8)
src/rai/rai/tools/ros2/utils.py (1)
22-23: Consider improving type hints and documenting type ignore.
- The
messageparameter type hint could be more specific thanAny. Consider using a union of common ROS2 message types or creating a custom type.- The
# type: ignorecomment should include a justification explaining why it's needed.Example improvement:
-def ros2_message_to_dict(message: Any) -> OrderedDict[str, Any]: +from rclpy.interfaces.msg import Message +def ros2_message_to_dict(message: Message) -> OrderedDict[str, Any]:- ) # type: ignore + ) # type: ignore[no-any-return] # rosidl_runtime_py.convert doesn't have type hintsAlso applies to: 34-37
tests/tools/test_tool_utils.py (1)
69-83: Enhance test assertions for ros2_message_to_dict.The test only verifies that the conversion succeeds but doesn't validate the contents of the returned dictionary. Consider adding assertions for expected field values.
Example improvement:
def test_ros2_message_to_dict(message): - assert ros2_message_to_dict(message) + result = ros2_message_to_dict(message) + assert isinstance(result, OrderedDict) + # Add assertions for expected fields based on message type + if isinstance(message, Point): + assert all(k in result for k in ['x', 'y', 'z']) + assert all(isinstance(v, (int, float)) for v in result.values())tests/tools/ros2/test_topic_tools.py (1)
146-152: Enhance transform test assertions.The test only verifies the presence of 'translation' and 'rotation' fields but doesn't validate their values or structure.
Example improvement:
response = tool._run( target_frame=publisher.frame_id, source_frame=publisher.child_frame_id, timeout_sec=1.0, ) # type: ignore assert "translation" in response assert "rotation" in response + # Verify translation structure + assert all(k in response["translation"] for k in ["x", "y", "z"]) + assert all(isinstance(v, (int, float)) for v in response["translation"].values()) + # Verify rotation structure + assert all(k in response["rotation"] for k in ["x", "y", "z", "w"]) + assert all(isinstance(v, (int, float)) for v in response["rotation"].values())tests/communication/ros2/helpers.py (2)
132-139: Add docstring and make transform values configurable.The class would benefit from:
- A docstring explaining its purpose and usage
- Configurable transform values through constructor parameters
class TransformPublisher(Node): + """Publishes transform data between two frames at regular intervals. + + This class is primarily used for testing transform-related functionality. + """ def __init__(self, topic: str): super().__init__("test_transform_publisher") self.tf_broadcaster = TransformBroadcaster(self) self.timer = self.create_timer(0.1, self.publish_transform) - self.frame_id = "base_link" - self.child_frame_id = "map" + self.frame_id: str = "base_link" + self.child_frame_id: str = "map" + self.translation = (1.0, 2.0, 3.0) + self.rotation = (0.0, 0.0, 0.0, 1.0) # quaternion (x, y, z, w)
140-152: Remove type ignore comments by using proper type hints.The type ignore comments can be avoided by properly typing the ROS2 message fields.
def publish_transform(self) -> None: msg = TransformStamped() - msg.header.stamp = self.get_clock().now().to_msg() # type: ignore - msg.header.frame_id = self.frame_id # type: ignore - msg.child_frame_id = self.child_frame_id # type: ignore - msg.transform.translation.x = 1.0 # type: ignore - msg.transform.translation.y = 2.0 # type: ignore - msg.transform.translation.z = 3.0 # type: ignore - msg.transform.rotation.x = 0.0 # type: ignore - msg.transform.rotation.y = 0.0 # type: ignore - msg.transform.rotation.z = 0.0 # type: ignore - msg.transform.rotation.w = 1.0 # type: ignore + msg.header.stamp = self.get_clock().now().to_msg() + msg.header.frame_id = self.frame_id + msg.child_frame_id = self.child_frame_id + msg.transform.translation.x, msg.transform.translation.y, msg.transform.translation.z = self.translation + msg.transform.rotation.x, msg.transform.rotation.y, msg.transform.rotation.z, msg.transform.rotation.w = self.rotation self.tf_broadcaster.sendTransform(msg)src/rai/rai/tools/ros2/topics.py (3)
Line range hint
82-105: Improve error handling in image conversion.The image conversion could fail for various reasons (e.g., encoding issues). Consider adding more specific error handling.
@wrap_tool_input def _run(self, tool_input: GetROS2ImageToolInput) -> Tuple[str, MultimodalArtifact]: message = self.connector.receive_message(tool_input.topic) msg_type = type(message.payload) - if msg_type == Image: - image = CvBridge().imgmsg_to_cv2( # type: ignore - message.payload, desired_encoding="rgb8" - ) - elif msg_type == CompressedImage: - image = CvBridge().compressed_imgmsg_to_cv2( # type: ignore - message.payload, desired_encoding="rgb8" - ) - else: - raise ValueError( - f"Unsupported message type: {message.metadata['msg_type']}" - ) + try: + if msg_type == Image: + image = CvBridge().imgmsg_to_cv2( + message.payload, desired_encoding="rgb8" + ) + elif msg_type == CompressedImage: + image = CvBridge().compressed_imgmsg_to_cv2( + message.payload, desired_encoding="rgb8" + ) + else: + raise ValueError( + f"Unsupported message type: {message.metadata['msg_type']}" + ) + except Exception as e: + raise ValueError(f"Failed to convert image: {str(e)}") return "Image received successfully", MultimodalArtifact(images=[preprocess_image(image)]) # type: ignore
136-162: Improve error handling and type safety in message interface retrieval.The error handling could be more informative, and the type safety could be improved.
class GetROS2MessageInterfaceTool(BaseTool): connector: ROS2ARIConnector name: str = "get_ros2_message_interface" description: str = "Get the interface of a ROS2 message" args_schema: Type[GetROS2MessageInterfaceToolInput] = ( GetROS2MessageInterfaceToolInput ) @wrap_tool_input def _run(self, tool_input: GetROS2MessageInterfaceToolInput) -> str: """Show ros2 message interface in json format.""" - msg_cls: Type[object] = rosidl_runtime_py.utilities.get_interface( - tool_input.msg_type - ) + try: + msg_cls: Type[object] = rosidl_runtime_py.utilities.get_interface( + tool_input.msg_type + ) + except (ImportError, AttributeError) as e: + raise ValueError(f"Invalid message type '{tool_input.msg_type}': {str(e)}") + try: msg_dict = ros2_message_to_dict(msg_cls()) # type: ignore return json.dumps(msg_dict) except NotImplementedError: # For action classes that can't be instantiated - goal_dict = ros2_message_to_dict(msg_cls.Goal()) # type: ignore - - result_dict = ros2_message_to_dict(msg_cls.Result()) # type: ignore - - feedback_dict = ros2_message_to_dict(msg_cls.Feedback()) # type: ignore + try: + goal_dict = ros2_message_to_dict(msg_cls.Goal()) # type: ignore + result_dict = ros2_message_to_dict(msg_cls.Result()) # type: ignore + feedback_dict = ros2_message_to_dict(msg_cls.Feedback()) # type: ignore + except AttributeError as e: + raise ValueError(f"Invalid action type '{tool_input.msg_type}': {str(e)}") return json.dumps( {"goal": goal_dict, "result": result_dict, "feedback": feedback_dict} )
171-184: Add error handling for transform retrieval.Consider catching and handling specific exceptions that might occur during transform retrieval.
@wrap_tool_input def _run(self, tool_input: GetROS2TransformToolInput) -> str: - transform = self.connector.get_transform( - target_frame=tool_input.target_frame, - source_frame=tool_input.source_frame, - timeout_sec=tool_input.timeout_sec, - ) - return stringify_dict(ros2_message_to_dict(transform)) + try: + transform = self.connector.get_transform( + target_frame=tool_input.target_frame, + source_frame=tool_input.source_frame, + timeout_sec=tool_input.timeout_sec, + ) + return stringify_dict(ros2_message_to_dict(transform)) + except LookupException as e: + raise ValueError(f"Transform lookup failed: {str(e)}") + except Exception as e: + raise ValueError(f"Unexpected error during transform retrieval: {str(e)}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/rai/rai/communication/ros2/connectors.py(3 hunks)src/rai/rai/tools/ros2/__init__.py(1 hunks)src/rai/rai/tools/ros2/topics.py(3 hunks)src/rai/rai/tools/ros2/utils.py(1 hunks)tests/communication/ros2/helpers.py(2 hunks)tests/tools/ros2/test_topic_tools.py(3 hunks)tests/tools/test_tool_utils.py(2 hunks)
🔇 Additional comments (4)
src/rai/rai/tools/ros2/__init__.py (1)
17-24: LGTM! Well-structured import changes and consistent naming.The changes follow a consistent naming convention with the ROS2 prefix and maintain good code organization using parentheses for imports.
Also applies to: 28-35
tests/tools/test_tool_utils.py (1)
68-68: Address the TODO comment about custom RAI messages.Please clarify if custom RAI messages will be needed and create a tracking issue if required.
Would you like me to help create an issue to track this TODO?
src/rai/rai/communication/ros2/connectors.py (2)
17-17: LGTM! Import statements are well-organized.The new imports for tf2_ros and rclpy modules are correctly organized and necessary for the added transformation functionality.
Also applies to: 19-26
52-53: LGTM! Clean delegation to the underlying API.The method maintains good separation of concerns by delegating to the _topic_api.
maciejmajek
force-pushed
the
feat/ros2-tools
branch
from
7eb8827 to
0697d45
Compare
Co-authored-by: Bartłomiej Boczek <[email protected]>
Co-authored-by: Bartłomiej Boczek <[email protected]>
maciejmajek
force-pushed
the
feat/ros2-tools
branch
from
00d1261 to
7271796
Compare
Purpose
Our new BaseConnector API requires changes to be made to the langchain tools.
Proposed Changes
Recreating existing tools using the new api
Testing
Summary by CodeRabbit
New Features
Improvements
Testing