feat: ros2 connector #379
feat: ros2 connector #379
Conversation
|
@maciejmajek i'll perform a comprehensive review of the ros2 connector implementation in the rai sdk. i'll analyze the code changes, architecture, and implementation details thoroughly. ✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
WalkthroughThis pull request introduces significant modifications to the ROS2 communication framework, focusing on the refactoring of message handling and the enhancement of API capabilities. Key changes include the removal of the Changes
Sequence DiagramsequenceDiagram
participant Connector as ROS2ARIConnector
participant TopicAPI as ROS2TopicAPI
participant ActionAPI as ROS2ActionAPI
Connector->>TopicAPI: send_message()
TopicAPI-->>Connector: Verify message metadata
Connector->>TopicAPI: receive_message()
TopicAPI-->>Connector: Return ROS2ARIMessage
Connector->>ActionAPI: start_action()
ActionAPI->>ActionAPI: Manage concurrent feedback
ActionAPI-->>Connector: Return action handle
Connector->>ActionAPI: terminate_action()
ActionAPI-->>Connector: Terminate goal
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: 9
🧹 Nitpick comments (11)
src/rai/rai/communication/ros2/connectors.py (1)
48-51: Consider implementing automatic message type detection for enhanced usability.The
send_messagemethod currently requires themsg_typein the message metadata. The TODO comment suggests allowingmsg_typeto beNoneand adding automatic topic type detection. Implementing this feature would improve the usability by removing the necessity for the user to specify the message type when it can be inferred.src/rai/rai/communication/ros2/api.py (5)
Line range hint
162-169: Clarify documentation forpublishmethod parameters.The
publishmethod's docstring mentions thatmsg_typeis required, but with the introduction of*, theauto_qos_matchingandqos_profileparameters are now keyword-only arguments. Update the docstring to reflect these changes and adjust the parameter descriptions accordingly.Apply this diff to update the docstring:
def publish( self, topic: str, msg_content: Dict[str, Any], msg_type: str, + *, auto_qos_matching: bool = True, qos_profile: Optional[QoSProfile] = None, ) -> None: """Publish a message to a ROS2 topic. Args: topic: Name of the topic to publish to msg_content: 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 Raises: ValueError: If neither auto_qos_matching is True nor qos_profile is provided """
186-193: Ensure proper argument validation in_verify_receive_args.The
_verify_receive_argsmethod correctly checks for incompatible arguments. However, consider refining the error messages to provide clearer guidance to the user.Apply this diff to improve error messages:
def _verify_receive_args( self, topic: str, auto_topic_type: bool, msg_type: Optional[str] ) -> None: if auto_topic_type and msg_type is not None: - raise ValueError("Cannot provide both auto_topic_type and msg_type") + raise ValueError("Provide either 'msg_type' or set 'auto_topic_type' to True, not both.") if not auto_topic_type and msg_type is None: - raise ValueError("msg_type must be provided if auto_topic_type is False") + raise ValueError("When 'auto_topic_type' is False, 'msg_type' must be provided.")
221-231: Redundantmsg_typecheck inreceivemethod.After calling
_verify_receive_args, there's an extra check formsg_typebeingNone, which is redundant since it's already handled. Consider removing the redundant check to simplify the code.Apply this diff to remove the redundant check:
self._verify_receive_args(topic, auto_topic_type, msg_type) topic_endpoints = self._verify_publisher_exists(topic) # TODO: Verify publishers topic type consistency if auto_topic_type: msg_type = topic_endpoints[0].topic_type - else: - if msg_type is None: - raise ValueError( - "msg_type must be provided if auto_topic_type is False" - )
297-306: Update_verify_publisher_existsdocumentation to reflect return type change.The
_verify_publisher_existsmethod now returns aList[TopicEndpointInfo], but the docstring still suggests it only raises an exception. Update the documentation to include information about the return value.Apply this diff to update the docstring:
def _verify_publisher_exists(self, topic: str) -> List[TopicEndpointInfo]: """Verify that at least one publisher exists for the given topic. + Returns: + A list of TopicEndpointInfo objects for the publishers. Raises: ValueError: If no publisher exists for the topic """
450-451: Handle exceptions indone_callbackto prevent unhandled errors.When adding the
done_callback, it's important to handle any exceptions that might occur within the callback to prevent them from propagating and causing unhandled errors.Apply this diff to wrap the
done_callback:get_result_future = cast(Future, goal_handle.get_result_async()) - get_result_future.add_done_callback(done_callback) + def wrapped_done_callback(future): + try: + done_callback(future) + except Exception as e: + self._logger.error(f"Exception in done_callback: {e}") + + get_result_future.add_done_callback(wrapped_done_callback)tests/communication/ros2/helpers.py (3)
56-56: Consider renaming the class to avoid trailing underscore.The class name
ActionServer_uses an unconventional naming pattern. Consider a more descriptive name likeTestActionServerorMockActionServer.
69-81: Consider increasing the sleep duration in execute callback.The sleep duration of 0.01s might be too short and could lead to excessive CPU usage in tests. Consider increasing it to 0.1s unless there's a specific requirement for rapid feedback.
- time.sleep(0.01) + time.sleep(0.1)
110-124: Consider using a single executor for multiple nodes.Creating separate executors for each node might be inefficient. Consider using a single
MultiThreadedExecutorfor all nodes unless there's a specific requirement for isolation.def multi_threaded_spinner( nodes: List[Node], ) -> Tuple[List[MultiThreadedExecutor], List[threading.Thread]]: - executors: List[MultiThreadedExecutor] = [] + executor = MultiThreadedExecutor() executor_threads: List[threading.Thread] = [] for node in nodes: - executor = MultiThreadedExecutor() executor.add_node(node) - executors.append(executor) - for executor in executors: - executor_thread = threading.Thread(target=executor.spin) - executor_thread.daemon = True - executor_thread.start() - executor_threads.append(executor_thread) + executor_thread = threading.Thread(target=executor.spin) + executor_thread.daemon = True + executor_thread.start() + executor_threads.append(executor_thread) - return executors, executor_threads + return [executor], executor_threadstests/communication/ros2/test_api.py (1)
378-380: Consider extracting common test setup code.Multiple test functions have similar setup code. Consider creating a fixture to reduce duplication:
@pytest.fixture def setup_action_test(request): action_name = f"{request.node.originalname}_action" node_name = f"{request.node.originalname}_node" action_server = ActionServer(action_name) node = Node(node_name) executors, threads = multi_threaded_spinner([action_server]) yield action_server, node, executors, threads shutdown_executors_and_threads(executors, threads)Also applies to: 406-408
src/rai/rai/communication/base_connector.py (1)
22-22: Consider restricting metadata value types.The current type annotation
Dict[str, Any]allows any value type in metadata, which could lead to type-related issues. Consider restricting value types to JSON-serializable types or defining a more specific type union.- metadata: Dict[str, Any] + metadata: Dict[str, Union[str, int, float, bool, None, List[Any], Dict[str, Any]]]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/rai/rai/communication/ari_connector.py(2 hunks)src/rai/rai/communication/base_connector.py(1 hunks)src/rai/rai/communication/ros2/api.py(9 hunks)src/rai/rai/communication/ros2/connectors.py(1 hunks)tests/communication/ros2/__init__.py(1 hunks)tests/communication/ros2/helpers.py(1 hunks)tests/communication/ros2/test_api.py(19 hunks)tests/communication/ros2/test_connectors.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/communication/ros2/test_api.py
36-36: Found useless expression. Either assign it to a variable or remove it.
(B018)
tests/communication/ros2/test_connectors.py
34-34: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (12)
src/rai/rai/communication/ros2/connectors.py (2)
25-28: Well-definedROS2ARIMessageclass extendingARIMessage.The
ROS2ARIMessageclass correctly extendsARIMessage, and the constructor properly initializes the payload and metadata.
31-42: Proper initialization and resource management inROS2ARIConnector.The
__init__method sets up the ROS2 node and APIs correctly. The use of aMultiThreadedExecutorand starting it on a separate thread is appropriate for handling incoming messages asynchronously. The shutdown mechanism is also in place via theshutdownmethod.src/rai/rai/communication/ari_connector.py (2)
27-34: Ensure consistency with updatedARIMessageconstructor.The addition of the
__init__method inARIMessageallows for payload and metadata initialization. This change enhances flexibility in message creation.
15-15: Confirm removal ofROS2RRIMessageand adjust dependent code accordingly.The
ROS2RRIMessageclass has been removed. Ensure that all references to this class in the codebase have been updated to useROS2ARIMessageor another appropriate class to prevent import errors or broken functionality.Run the following script to search for any remaining references:
✅ Verification successful
ROS2RRIMessage removal verified successfully
The codebase shows no remaining references to
ROS2RRIMessageand demonstrates consistent usage ofROS2ARIMessageacross both implementation and test files. All dependent code appears to be properly updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find references to the removed ROS2RRIMessage class. # Search the codebase for 'ROS2RRIMessage' rg 'ROS2RRIMessage'Length of output: 19
Script:
#!/bin/bash # Search with various patterns to ensure thorough verification echo "=== Searching for any RRI or ARI message patterns ===" rg -i "ros2\s*(rri|ari).*message" echo -e "\n=== Checking specifically for ROS2ARIMessage ===" rg "ROS2ARIMessage" echo -e "\n=== Looking for any message type definitions ===" ast-grep --pattern 'class $_Message'Length of output: 3824
tests/communication/ros2/helpers.py (3)
31-41: LGTM! Clean and focused test service implementation.The service server implementation is well-structured and appropriate for testing purposes.
44-53: LGTM! Well-structured message receiver for testing.The implementation properly stores received messages for verification in tests.
98-107: LGTM! Clean publisher implementation.The implementation uses appropriate timer frequency for testing purposes.
tests/communication/ros2/__init__.py (1)
15-29: LGTM! Clean module initialization.The implementation properly defines the public API and follows Python best practices.
src/rai/rai/communication/base_connector.py (4)
16-16: LGTM! Import changes are well-structured.The import statement properly includes all necessary types from the typing module to support the new metadata feature.
Line range hint
41-41: LGTM! BaseConnector remains compatible.The generic typing with
T = TypeVar("T", bound=BaseMessage)properly handles the BaseMessage changes, maintaining type safety in the connector interface.
Line range hint
1-73: Verify consistent metadata handling across connectors.Since BaseMessage now includes metadata support, ensure that all implementing connectors properly handle metadata propagation.
✅ Verification successful
Metadata handling is properly implemented across connectors ✓
The verification shows that metadata is consistently handled:
- Base implementation provides proper metadata support
- ROS2 connector correctly initializes and extends metadata with domain-specific information
- Test coverage validates the metadata handling for messages, services, and actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all connector implementations rg -t py "class.*\(.*BaseConnector\)" -C 5 # Search for metadata usage in message handling rg -t py "metadata.*=.*" -C 2Length of output: 5274
20-20: Verify impact of Protocol removal on type checking.The removal of
Protocolbase class might affect static type checking in code that relies on structural typing. Please ensure this change doesn't break existing type checks.✅ Verification successful
Protocol removal is safe for type checking
The removal of
ProtocolfromBaseMessageis safe as type checking in the codebase primarily relies onlangchain_core.messages.BaseMessagefor annotations. OurBaseMessageclass is only used as a concrete base class for message implementations (ARIMessage,HRIMessage,MultimodalMessage), not for structural typing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of BaseMessage in type annotations rg -t py "BaseMessage" -C 2Length of output: 13385
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/rai/rai/communication/ros2/api.py (1)
Line range hint
197-231: Improve error handling and avoid redundant checks inreceivemethod.The method
_verify_receive_argsalready handles the validation ofauto_topic_typeandmsg_type. The additional check at lines 228-231 is redundant.Apply this diff to remove the redundant check:
else: - if msg_type is None: - raise ValueError( - "msg_type must be provided if auto_topic_type is False" - )
♻️ Duplicate comments (5)
src/rai/rai/communication/ros2/connectors.py (3)
82-85:⚠️ Potential issueAdd validation for
'msg_type'in message metadata to preventKeyError.In the
service_callmethod,message.metadata["msg_type"]is accessed without checking if'msg_type'exists. This could raise aKeyErrorif'msg_type'is missing frommessage.metadata. It's important to validate the presence of'msg_type'before using it.Apply this diff to add the validation:
def service_call( self, message: ROS2ARIMessage, target: str, timeout_sec: float = 1.0 ): + if "msg_type" not in message.metadata: + raise ValueError("'msg_type' is required in message metadata for service calls") msg = self._service_api.call_service( service_name=target, service_type=message.metadata["msg_type"],
92-104:⚠️ Potential issueEnsure
action_datais notNoneto preventAttributeError.The
start_actionmethod acceptsaction_dataasOptional[ROS2ARIMessage], but ifaction_dataisNone, accessingaction_data.metadataandaction_data.payloadwill raise anAttributeError. It's advisable to check ifaction_dataisNonebefore proceeding or change the parameter type toROS2ARIMessage.Apply this diff to ensure
action_datais notNone:def start_action( self, - action_data: Optional[ROS2ARIMessage], + action_data: ROS2ARIMessage, target: str, on_feedback: Callable[[Any], None] = lambda _: None, on_done: Callable[[Any], None] = lambda _: None, timeout_sec: float = 1.0, ) -> str: - if not isinstance(action_data, ROS2ARIMessage): + if action_data is None or not isinstance(action_data, ROS2ARIMessage): raise ValueError("Action data must be a ROS2ARIMessage instance")
103-104:⚠️ Potential issueValidate presence of
'msg_type'inaction_datametadata.In
start_action,action_data.metadata["msg_type"]is accessed without checking if'msg_type'exists in the metadata. This could result in aKeyErrorif'msg_type'is missing. Please add a check to ensure'msg_type'is present.Apply this diff to add the validation:
if action_data is None or not isinstance(action_data, ROS2ARIMessage): raise ValueError("Action data must be a ROS2ARIMessage instance") + if "msg_type" not in action_data.metadata: + raise ValueError("'msg_type' is required in action_data metadata")src/rai/rai/communication/ros2/api.py (2)
363-398:⚠️ Potential issueEnsure proper shutdown of
ThreadPoolExecutor.The
_callback_executoris introduced for handling feedback callbacks. It is crucial to ensure it is properly shut down to avoid resource leaks.This issue is similar to a past review comment.
Apply this diff to add a
shutdownmethod and ensure it's called appropriately:class ROS2ActionAPI: def __init__(self, node: rclpy.node.Node) -> None: self.node = node self._logger = node.get_logger() self.actions: Dict[str, ROS2ActionData] = {} self._callback_executor = ThreadPoolExecutor(max_workers=10) + def shutdown(self) -> None: + """Shut down the ThreadPoolExecutor to release resources.""" + if self._callback_executor: + self._callback_executor.shutdown(wait=False) def _generate_handle(self): return str(uuid.uuid4())Ensure that
shutdownis called when the application is terminating or when theROS2ActionAPIinstance is no longer needed.
483-486: 🛠️ Refactor suggestionAvoid using
__del__for resource cleanup; use explicit shutdown instead.Using
__del__is not reliable for resource cleanup because object destruction timing is nondeterministic in Python.This issue is similar to a past review comment.
Apply this diff to remove the
__del__method:def get_result(self, handle: str) -> Any: if not self.is_goal_done(handle): raise ValueError(f"Goal {handle} is not done") if self.actions[handle]["result_future"] is None: raise ValueError(f"No result available for goal {handle}") return self.actions[handle]["result_future"].result() - def __del__(self) -> None: - """Cleanup thread pool when object is destroyed.""" - if hasattr(self, "_callback_executor"): - self._callback_executor.shutdown(wait=False)Ensure that the
shutdownmethod added earlier is called appropriately to manage resources.
🧹 Nitpick comments (14)
src/rai/rai/communication/ari_connector.py (2)
27-34: Remove unnecessary*argsand**kwargsfromARIMessage.__init__.The
__init__method includes*argsand**kwargs, but they are not used in the method or passed tosuper().__init__(). Including them can lead to unintended behavior if unexpected arguments are passed. It's recommended to remove them for clarity and to prevent potential issues.Apply this diff to remove
*argsand**kwargs:def __init__( self, payload: Any, metadata: Optional[Dict[str, Any]] = None, - *args: Any, - **kwargs: Any, ): - super().__init__(payload, metadata, *args, **kwargs) + super().__init__(payload, metadata)
Line range hint
35-42: Correct the docstring inARIConnectorto remove unrelated text.The docstring for
ARIConnectorcontains an unrelated sentence:"Base class for Robot-Robot Interaction (RRI) connectors."This appears to be a copy-paste error and may confuse readers about the purpose of the class. Please remove or correct this line to maintain clarity.Apply this diff to correct the docstring:
Usage: Inherit from this class to implement specific ARI connectors, such as - ROS2-based or custom protocol-based connectors. Base class for Robot-Robot Interaction (RRI) connectors. + ROS2-based or custom protocol-based connectors.src/rai/rai/communication/ros2/connectors.py (1)
48-51: Offer assistance for implementing auto topic type detection.The
TODOcomment indicates an intention to allowmsg_typeto beNoneand add auto topic type detection in thesend_messagemethod. If you need help implementing this feature, I can assist with high confidence.Would you like me to propose an implementation for auto-detecting the message type when
msg_typeis not provided?src/rai/rai/communication/ros2/api.py (3)
186-193: Add documentation for_verify_receive_argsmethod.The newly added
_verify_receive_argsmethod lacks a docstring. Providing a clear docstring enhances code readability and maintainability.Apply this diff to add a docstring:
def _verify_receive_args( self, topic: str, auto_topic_type: bool, msg_type: Optional[str] ) -> None: + """ + Verify the arguments provided to the receive method. + + Args: + topic: Name of the topic. + auto_topic_type: Flag to determine if topic type should be auto-detected. + msg_type: The message type, if not auto-detected. + + Raises: + ValueError: If both auto_topic_type is True and msg_type is provided, + or if auto_topic_type is False and msg_type is None. + """ if auto_topic_type and msg_type is not None: raise ValueError("Cannot provide both auto_topic_type and msg_type") if not auto_topic_type and msg_type is None: raise ValueError("msg_type must be provided if auto_topic_type is False")
224-226: Enhance topic type consistency verification.The TODO comment at line 224 suggests verifying publishers' topic type consistency. Implementing this check can prevent runtime errors due to mismatched message types.
Would you like assistance in implementing the topic type consistency verification?
303-306: Optimize publisher existence verification.The
_verify_publisher_existsmethod returnstopic_endpointsbut does not utilize it beyond checking for emptiness. If only the existence check is needed, returning the list might be unnecessary.If the
topic_endpointsare not used elsewhere, consider modifying the method to returnboolorNone.Alternatively, if
topic_endpointsare needed, ensure they are utilized appropriately.tests/communication/ros2/helpers.py (4)
104-108: Avoid hardcoded messages inpublish_messagemethod.The message content is hardcoded as "Hello, ROS2!". For flexibility, consider parameterizing the message.
Apply this diff to parameterize the message:
class MessagePublisher(Node): def __init__(self, topic: str, message: str = "Hello, ROS2!"): super().__init__("test_message_publisher") self.publisher = self.create_publisher(String, topic, 10) self.timer = self.create_timer(0.1, self.publish_message) + self.message_content = message def publish_message(self) -> None: msg = String() - msg.data = "Hello, ROS2!" + msg.data = self.message_content self.publisher.publish(msg)
110-124: Ensure proper handling of daemon threads inmulti_threaded_spinner.Setting threads as daemons can cause abrupt termination without cleanup. Consider not setting
daemon = Trueand ensuring proper shutdown.Apply this diff to remove the daemon setting:
for executor in executors: executor_thread = threading.Thread(target=executor.spin) - executor_thread.daemon = True executor_thread.start() executor_threads.append(executor_thread)Ensure that all threads are properly joined during shutdown.
126-138: Reduce sleep duration inshutdown_executors_and_threads.The sleep duration of 0.5 seconds may be unnecessary and could slow down tests.
Consider reducing or removing the sleep:
def shutdown_executors_and_threads( executors: List[MultiThreadedExecutor], threads: List[threading.Thread] ) -> None: # First shutdown executors for executor in executors: executor.shutdown() # Small delay to allow executors to finish pending operations - time.sleep(0.5) + time.sleep(0.1) # Then join threads with a timeout for thread in threads: thread.join(timeout=2.0)
140-144: Ensure proper cleanup inros_setupfixture.The
ros_setupfixture does not handle exceptions that might occur betweenyieldandrclpy.shutdown(). This could leave the ROS client library in an inconsistent state.Wrap the yield in a try-finally block to ensure
rclpy.shutdown()is always called:@pytest.fixture(scope="function") def ros_setup() -> Generator[None, None, None]: rclpy.init() + try: yield + finally: rclpy.shutdown()tests/communication/ros2/test_connectors.py (3)
90-93: Add timeout handling for service calls.The service call could potentially block indefinitely. Consider:
- Adding a timeout parameter to prevent hanging
- Implementing retry logic with exponential backoff
- Adding assertions for service availability before making the call
159-159: Improve error handling test case.Using
1 / 0for error testing is not ideal. Consider:
- Using a custom exception that better represents real-world scenarios
- Testing multiple error conditions
- Verifying error logging and recovery behavior
- on_feedback=lambda feedback: 1 / 0, + on_feedback=lambda feedback: raise RuntimeError("Simulated callback error"),
167-172: Design decision needed for callback lifecycle management.The skipped test highlights an important architectural decision regarding callback handling during shutdown. Consider:
- Implementing a callback timeout mechanism
- Adding explicit callback cancellation during shutdown
- Using asyncio or threading events instead of sleep
Would you like me to propose a design for proper callback lifecycle management?
tests/communication/ros2/__init__.py (1)
23-29: Update__all__to include all imported items.The
ServiceServerclass is imported via helpers but not included in__all__. Consider adding all imported items to maintain a complete public interface.__all__ = [ "shutdown_executors_and_threads", "multi_threaded_spinner", "MessagePublisher", "ActionServer", "MessageReceiver", + "ServiceServer", ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/rai/rai/communication/ari_connector.py(2 hunks)src/rai/rai/communication/base_connector.py(1 hunks)src/rai/rai/communication/ros2/api.py(9 hunks)src/rai/rai/communication/ros2/connectors.py(1 hunks)tests/communication/ros2/__init__.py(1 hunks)tests/communication/ros2/helpers.py(1 hunks)tests/communication/ros2/test_api.py(19 hunks)tests/communication/ros2/test_connectors.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/communication/ros2/test_api.py
36-36: Found useless expression. Either assign it to a variable or remove it.
(B018)
tests/communication/ros2/test_connectors.py
34-34: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (5)
tests/communication/ros2/test_api.py (1)
36-36: Fix the useless expression warning.The line
ros_setup # type: ignoreresults in a useless expression warning from the static analysis tool. Assigning the fixture to_explicitly uses it and prevents the warning.Apply this diff to fix the warning:
-ros_setup # type: ignore prevents pytest from complaining about unused fixture +_ = ros_setup # Explicitly use the fixture to prevent pytest warnings🧰 Tools
🪛 Ruff (0.8.2)
36-36: Found useless expression. Either assign it to a variable or remove it.
(B018)
🧰 Tools
🪛 Ruff (0.8.2)
36-36: Found useless expression. Either assign it to a variable or remove it.
(B018)
src/rai/rai/communication/base_connector.py (1)
20-35: Add input validation and documentation formetadatainBaseMessage.The
metadataattribute lacks input validation, and there's no documentation explaining its intended use.This is similar to a past review comment.
Apply this diff to add validation and a docstring:
class BaseMessage: payload: Any metadata: Dict[str, Any] + """ + Base class for messages with payload and optional metadata. + + Attributes: + payload: The main data of the message. + metadata: Additional information associated with the message. + """ def __init__( self, payload: Any, metadata: Optional[Dict[str, Any]] = None, *args: Any, **kwargs: Any, ): self.payload = payload + if metadata is None: + self.metadata = {} + else: + if not all(isinstance(k, str) for k in metadata.keys()): + raise ValueError("Metadata keys must be strings") + self.metadata = metadatatests/communication/ros2/helpers.py (1)
69-88: Handle potential infinite loops inhandle_test_actionmethod.The
handle_test_actionmethod uses a fixed range, but if the range is altered or conditions change, there could be a risk of an infinite loop.Ensure that the loop in
handle_test_actionhas a definite termination condition.tests/communication/ros2/test_connectors.py (2)
34-34: Fix the useless expression warning.The line
ros_setup # type: ignoreis flagged by static analysis. Consider using the fixture properly:-ros_setup # type: ignore prevents pytest from complaining about unused fixture +_ = ros_setup # Explicitly use the fixture to prevent pytest warnings🧰 Tools
🪛 Ruff (0.8.2)
34-34: Found useless expression. Either assign it to a variable or remove it.
(B018)
50-50: Consider implementing a proper discovery mechanism.The test relies on
time.sleep()for synchronization, which can be flaky. Consider implementing a proper discovery mechanism with retries and timeouts.
maciejmajek
force-pushed
the
feat/ros2-connectors
branch
from
f9be1c6 to
3274763
Compare
Purpose
New RAI sdk is based on Connector architecture which closely resembles ROS 2 features/api. This PR implements framework standard ROS2 connectors.
Proposed Changes
ROS2 ARI (Agent-Robot-Interface) connector based on out generic ROS 2 api.
Issues
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests