refactor: rai the package #386
Conversation
|
@coderabbitai full review |
WalkthroughThis pull request encompasses a comprehensive set of changes across multiple files in the RAI project, focusing on reintroducing and reorganizing import statements, updating package configurations, and making minor enhancements to various components. The changes span example demos, core library files, extensions, and test suites, with a particular emphasis on restoring ROS 2 related functionality and refining the project's structure. Changes
Possibly related PRs
✨ 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: 4
🔭 Outside diff range comments (4)
src/rai/rai/tools/ros/deprecated.py (1)
Line range hint
106-107: Replace deprecatedtostring()withtobytes().The
tostring()method is deprecated in favor oftobytes(). Update the code to use the newer method.- image_data = cv2.imencode(".png", cv_image)[1].tostring() # type: ignore + image_data = cv2.imencode(".png", cv_image)[1].tobytes() # type: ignoresrc/rai/rai/tools/utils.py (2)
Line range hint
1-300: This file retains ROS2 dependencies, contradicting the PR objective.The PR aims to convert this into a pure Python package, but the file still heavily depends on ROS2-specific imports and functionality (rclpy, cv_bridge, sensor_msgs, tf2_ros). This contradicts the goal of removing the ROS2 package dependency.
Consider the following approaches:
- Move ROS2-specific functionality to a separate package that can be optionally installed
- Create an abstraction layer to decouple ROS2 dependencies from core functionality
- Use dependency injection to allow different implementations (ROS2 vs pure Python)
Would you like me to help create a design proposal for any of these approaches?
Line range hint
109-196: Consider removing or updating deprecated functions.These functions are marked as deprecated with messages indicating a transition to langchain.agent:
images_to_vendor_formatrun_tool_callrun_requested_toolsSince we're refactoring the package, this would be a good time to remove deprecated code or update it to use the new langchain.agent-based approach.
Would you like me to help implement the langchain.agent-based versions of these functions?
src/rai_hmi/rai_hmi/agent.py (1)
Line range hint
46-61: Consider adding error handling for task submission.The
submit_mission_to_the_robottool should handle potential errors during task execution.@tool def submit_mission_to_the_robot(task: TaskInput): """Use this tool submit the task to the robot. The task will be handled by the executor part of your system.""" - uid = uuid.uuid4() - hmi_node.execute_mission( - Task( - name=task.name, - description=task.description, - priority=task.priority, - uid=str(uid), - ) - ) - return f"UID={uid} | Task added to the queue: {task.model_dump_json()}" + try: + hmi_node.execute_mission( + Task( + name=task.name, + description=task.description, + priority=task.priority, + uid=str(uid), + ) + ) + return f"UID={uid} | Task added to the queue: {task.model_dump_json()}" + except Exception as e: + return f"Failed to submit task: {str(e)}"
🧹 Nitpick comments (24)
pyproject.toml (1)
Line range hint
16-81: Consider pinning versions for critical dependencies.Several observations about the dependency specifications:
- Using wildcards (*) for critical packages like
langchain,langgraph, etc., could lead to breaking changes- Git dependencies (
openwakeword,sam2,visualnav_transformer) might cause reproducibility issuesConsider:
- Pinning versions for critical dependencies instead of using wildcards
- Using released versions instead of git repositories where possible
- Adding version constraints for git dependencies using the
revortagparametersExample:
-langchain = "*" -langgraph = "*" +langchain = "^0.1.0" # Replace with appropriate version +langgraph = "^0.1.0" # Replace with appropriate versionsrc/rai/rai/tools/ros/deprecated.py (1)
Line range hint
52-67: Consider extracting QoS profile configuration.The QoS profile configuration for the
/maptopic appears to be hardcoded with a TODO comment. Since this is marked as "overfitting to husarion", consider extracting this configuration to a separate configuration file or making it configurable through parameters.- if ( - self.topic == "/map" - ): # overfitting to husarion TODO(maciejmajek): find a better way - qos_profile = QoSProfile( - reliability=QoSReliabilityPolicy.RELIABLE, - history=QoSHistoryPolicy.KEEP_ALL, - durability=QoSDurabilityPolicy.TRANSIENT_LOCAL, - lifespan=Duration(seconds=0), - deadline=Duration(seconds=0), - liveliness=QoSLivelinessPolicy.AUTOMATIC, - liveliness_lease_duration=Duration(seconds=0), - ) + qos_profile = self._get_qos_profile_for_topic(self.topic)Then add a new method:
def _get_qos_profile_for_topic(self, topic: str) -> QoSProfile: if topic == "/map": # TODO: Move to configuration return QoSProfile( reliability=QoSReliabilityPolicy.RELIABLE, history=QoSHistoryPolicy.KEEP_ALL, durability=QoSDurabilityPolicy.TRANSIENT_LOCAL, lifespan=Duration(seconds=0), deadline=Duration(seconds=0), liveliness=QoSLivelinessPolicy.AUTOMATIC, liveliness_lease_duration=Duration(seconds=0), ) return rclpy.qos.qos_profile_sensor_datasrc/rai/rai/tools/ros2/topics.py (1)
Line range hint
89-101: Add error handling for image conversion failures.While the type checking is good, consider these improvements:
- Add try-catch blocks around CvBridge conversions as they can fail
- Make error messages consistent by using the actual type name instead of metadata
if msg_type == Image: - image = CvBridge().imgmsg_to_cv2( # type: ignore - message.payload, desired_encoding="rgb8" - ) + try: + image = CvBridge().imgmsg_to_cv2( # type: ignore + message.payload, desired_encoding="rgb8" + ) + except Exception as e: + raise ValueError(f"Failed to convert Image message: {e}") elif msg_type == CompressedImage: - image = CvBridge().compressed_imgmsg_to_cv2( # type: ignore - message.payload, desired_encoding="rgb8" - ) + try: + image = CvBridge().compressed_imgmsg_to_cv2( # type: ignore + message.payload, desired_encoding="rgb8" + ) + except Exception as e: + raise ValueError(f"Failed to convert CompressedImage message: {e}") else: raise ValueError( - f"Unsupported message type: {message.metadata['msg_type']}" + f"Unsupported message type: {msg_type.__name__}" )src/rai/rai/utils/ros_logs.py (1)
Line range hint
1-156: Consider moving ROS2-specific code to ros2 directory.Since we're transitioning to a pure Python package, consider moving this ROS2-specific logging functionality to the
ros2directory to better isolate ROS2 dependencies. This would make the package structure clearer and make it easier to manage ROS2 dependencies.Suggested structure:
src/rai/rai/ ├── tools/ │ └── ros2/ │ ├── topics.py │ └── logs.py # moved from utils/ros_logs.py └── utils/ └── common_utils.py # non-ROS2 utilitiessrc/rai_whoami/rai_whoami/rai_whoami_node.py (1)
Line range hint
1-24: File structure contradicts PR objectives.This file still heavily relies on ROS2-specific imports and functionality, which contradicts the PR objective of converting to a pure Python package managed by Poetry. The file needs significant refactoring to remove ROS2 dependencies.
Consider the following refactoring steps:
- Extract the core functionality into a pure Python class
- Create an adapter class that implements the ROS2 interface
- Move ROS2-specific code to the adapter class
- Update the imports to reflect the new structure
Example structure:
# core.py (Pure Python) class WhoAmICore: def __init__(self, robot_description_path): self.robot_description_path = robot_description_path # Core functionality without ROS2 dependencies # ros_adapter.py (ROS2 specific) class WhoAmINode(Node): def __init__(self): self.core = WhoAmICore(...) # ROS2 specific initializationsrc/rai_hmi/rai_hmi/base.py (1)
Line range hint
1-29: File structure contradicts PR objectives.This file also heavily relies on ROS2-specific functionality, which contradicts the PR objective of converting to a pure Python package managed by Poetry.
Consider the following refactoring steps:
- Extract the core HMI functionality into a pure Python base class
- Create a ROS2-specific adapter class
- Move ROS2-specific code (publishers, action clients, services) to the adapter
- Update the imports to reflect the new structure
Example structure:
# base.py (Pure Python) class BaseHMI: def __init__(self, robot_description_path): self.robot_description_path = robot_description_path # Core functionality without ROS2 dependencies # ros_adapter.py (ROS2 specific) class BaseHMINode(Node): def __init__(self): self.core = BaseHMI(...) # ROS2 specific initializationsrc/rai/rai/communication/sound_device_connector.py (2)
Line range hint
23-157: Great implementation with room for minor improvements.The implementation demonstrates good practices with comprehensive error handling, type hints, and documentation. Consider these enhancements:
- Add return type hints for class methods
- Consider using dataclasses for
ConfiguredAudioInputDevice- Add logging for error conditions and device state changes
Example implementation for point 2:
-class ConfiguredAudioInputDevice: +from dataclasses import dataclass + +@dataclass +class ConfiguredAudioInputDevice: """ A class to store the configuration of an audio device - - Attributes - ---------- - sample_rate (int): Device sample rate - consumer_sampling_rate (int): The sampling rate of the consumer - window_size_samples (int): The size of the window in samples - dtype (str): The data type of the audio samples """ + sample_rate: float + consumer_sampling_rate: int + window_size_samples: int + dtype: str
Line range hint
23-27: Enhance error handling structure.While the error handling is comprehensive, consider these improvements:
- Create specific exception subclasses for different error types (configuration, streaming, device)
- Add error codes to make programmatic error handling easier
- Include more context in error messages (e.g., current device state)
Example implementation:
class SoundDeviceError(Exception): """Base exception for sound device errors.""" def __init__(self, msg: str, error_code: int): self.error_code = error_code super().__init__(f"[{error_code}] {msg}") class DeviceConfigurationError(SoundDeviceError): """Raised when device configuration fails.""" def __init__(self, msg: str, device_id: str): super().__init__(f"Configuration error for device {device_id}: {msg}", 1001) class StreamingError(SoundDeviceError): """Raised when streaming operations fail.""" def __init__(self, msg: str, device_id: str): super().__init__(f"Streaming error for device {device_id}: {msg}", 1002)Also applies to: 89-157
tests/tools/ros2/test_action_tools.py (1)
Line range hint
28-33: Consider removing the alias for ActionServer.The import
ActionServer_ as ActionServersuggests a potential naming conflict. Consider renaming the original class to avoid using an alias.-from tests.communication.ros2.helpers import ActionServer_ as ActionServer +from tests.communication.ros2.helpers import ActionServersrc/rai/rai/agents/conversational_agent.py (1)
Line range hint
54-61: Consider simplifying logger initialization.The logger initialization logic can be simplified using the null coalescing pattern.
- _logger = None - if isinstance(logger, RcutilsLogger): - _logger = logger - else: - _logger = logging.getLogger(__name__) + _logger = logger if isinstance(logger, RcutilsLogger) else logging.getLogger(__name__)tests/tools/ros2/test_topic_tools.py (1)
Line range hint
63-76: Consider implementing a proper discovery mechanism.Both test cases are marked with
@pytest.mark.xfaildue to ROS2 node discovery being asynchronous. This is a known limitation that should be addressed.Consider implementing a proper discovery mechanism with timeout using ROS2's built-in discovery features:
- Use
rclpy.wait_for_message()with timeout- Implement a subscription count check before publishing
- Use ROS2's QoS settings for reliable communication
Also applies to: 89-102
examples/taxi-demo.py (1)
Line range hint
67-95: Consider extracting mock navigation to a separate module.The mock navigation implementation, while well-documented, could be moved to a separate module to improve maintainability and make it easier to replace with a real implementation later.
Consider creating a separate module for navigation-related functionality:
- @tool - def navigate(street: str, number: int, city: str, country: str) -> str: - """ - Send the destination to the Autoware system. - This is a mock example, so the goal pose is hardcoded. - In a real implementation, the goal pose should be retrieved from maps vendor (latitude and longitude) - and then converted to a pose in the map frame. - """ - autoware_navigate_tool = Ros2PubMessageTool(node=self) - autoware_mode_tool = Ros2ServiceTool() - autoware_navigate_tool.run(...) - autoware_mode_tool.run(...) - return f"Navigating to {street} {number}, {city}, {country}"# navigation.py from rai.tools.ros.native import Ros2PubMessageTool, Ros2ServiceTool from langchain_core.tools import tool @tool def navigate(street: str, number: int, city: str, country: str) -> str: """ Send the destination to the Autoware system. This is a mock example, so the goal pose is hardcoded. In a real implementation, the goal pose should be retrieved from maps vendor (latitude and longitude) and then converted to a pose in the map frame. """ autoware_navigate_tool = Ros2PubMessageTool(node=self) autoware_mode_tool = Ros2ServiceTool() autoware_navigate_tool.run(...) autoware_mode_tool.run(...) return f"Navigating to {street} {number}, {city}, {country}"examples/rosbot-xl-demo.py (1)
Line range hint
108-127: Consider moving environment coordinates to a configuration file.The hardcoded environment coordinates should be moved to a configuration file for better maintainability and reusability.
Consider creating a YAML configuration file:
# config/environment.yaml corners: - [-2.76, 9.04, 0.0] - [4.62, 9.07, 0.0] - [-2.79, -3.83, 0.0] - [4.59, -3.81, 0.0] locations: kitchen: - [2.06, -0.23, 0.0] - [2.07, -1.43, 0.0] - [-2.44, -0.38, 0.0] - [-2.56, -1.47, 0.0] living_room: - [-2.49, 1.87, 0.0] - [-2.50, 5.49, 0.0] - [0.79, 5.73, 0.0] - [0.92, 1.01, 0.0]src/rai/rai/agents/voice_agent.py (2)
Line range hint
41-42: Consider implementing name-based device identification.The TODO comment indicates that the current device identification method using IDs should be replaced with a name-based approach.
Would you like me to help implement name-based device identification? This could involve:
- Creating a device discovery mechanism
- Mapping device names to IDs
- Providing a more user-friendly interface for device selection
Line range hint
156-157: Remove redundant TODO comment.The TODO comment about the transcription lock being only necessary for the local model should be addressed or removed.
- with self.transcription_lock: # this is only necessary for the local model... TODO: fix this somehow + with self.transcription_lock: # Lock required for thread-safe transcriptiontests/core/test_ros2_tools.py (1)
22-23: Consider test isolation for ROS2 dependencies.Since the package is transitioning to a pure Python package, consider marking ROS2-dependent tests with a custom marker to allow running non-ROS2 tests separately.
+@pytest.mark.ros2 @pytest.mark.billable @pytest.mark.timeout(10) @pytest.mark.parametrize(tests/messages/test_transport.py (1)
Line range hint
34-61: Improve ROS2 distribution handling.The QoS profile selection based on ROS_DISTRO could be simplified and made more maintainable.
Consider using a configuration file or environment variables to manage QoS profiles:
-def get_qos_profiles() -> List[str]: - ros_distro = os.environ.get("ROS_DISTRO") - match ros_distro: - case "humble": - return [] - case "jazzy": - return [ - QoSPresetProfiles.DEFAULT.name, - # ... other profiles - ] - case _: - raise ValueError(f"Invalid ROS_DISTRO: {ros_distro}") +def get_qos_profiles() -> List[str]: + profiles = os.environ.get("RAI_QOS_PROFILES", "").split(",") + return [p.strip() for p in profiles if p.strip()]src/rai_hmi/rai_hmi/voice_hmi.py (1)
Line range hint
71-75: Consider using a context manager for executor thread management.The current thread management could be improved using a context manager to ensure proper cleanup.
- self.executor_thread = threading.Thread( - target=executor.spin, daemon=True - ).start() + self.executor_thread = threading.Thread( + target=executor.spin, daemon=True + ) + self.executor_thread.start() + self.executor = executor # Store for cleanupAdd a cleanup method:
def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): if hasattr(self, 'executor'): self.executor.shutdown() if hasattr(self, 'executor_thread'): self.executor_thread.join(timeout=1.0)src/rai/rai/agents/tool_runner.py (1)
31-31: Consider creating a logger abstraction layer.The current approach of using a Union type for loggers suggests a need for abstraction. Consider creating a common logger interface that can wrap both RcutilsLogger and logging.Logger.
from abc import ABC, abstractmethod class LoggerInterface(ABC): @abstractmethod def info(self, msg: str) -> None: pass @abstractmethod def debug(self, msg: str) -> None: pass @abstractmethod def warning(self, msg: str) -> None: pass class ROS2LoggerAdapter(LoggerInterface): def __init__(self, logger: RcutilsLogger) -> None: self._logger = logger def info(self, msg: str) -> None: self._logger.info(msg) class StandardLoggerAdapter(LoggerInterface): def __init__(self, logger: logging.Logger) -> None: self._logger = logger def info(self, msg: str) -> None: self._logger.info(msg)Also applies to: 42-42
src/rai/rai/tools/ros/manipulation.py (1)
27-27: Ensure proper dependency management for ROS2 tools.Since this is under the
ros2directory, its dependencies should be marked as optional in the package configuration. This allows users to install the package without ROS2 while still maintaining the ability to use ROS2 features when needed.Consider adding these dependencies to pyproject.toml as optional:
[tool.poetry.dependencies] python = "^3.8" rclpy = { version = "^4.0.0", optional = true } tf2_geometry_msgs = { version = "^0.25.0", optional = true } [tool.poetry.extras] ros2 = ["rclpy", "tf2_geometry_msgs"]src/rai_hmi/rai_hmi/text_hmi.py (1)
39-41: LGTM! Consider adding import grouping comments.The reintroduced imports are essential for ROS2 node functionality and Streamlit UI components. To improve code organization, consider grouping imports with descriptive comments.
+# ROS2 imports from rclpy.node import Node + +# Streamlit imports from streamlit.delta_generator import DeltaGeneratorsrc/rai/rai/ros2_apis.py (1)
29-31: LGTM! Consider documenting utility functions' purpose.The reintroduced utility imports are essential for ROS2 functionality. Consider adding docstring comments to explain their purpose and usage.
+# Utility functions for ROS2 message handling and node discovery from rai.tools.ros.utils import import_message_from_str from rai.utils.ros import NodeDiscovery from rai.utils.ros_async import get_future_resultsrc/rai/rai/node.py (1)
41-44: LGTM! Consider adding import section headers.The reintroduced imports maintain essential ROS2 functionality. Consider organizing imports with section headers for better readability.
+# ROS2 Action and Node imports from rclpy.action.server import ActionServer, GoalResponse, ServerGoalHandle from rclpy.node import Node + +# ROS2 Service imports from std_srvs.srv import Triggersrc/rai/rai/communication/ros2/api.py (1)
44-44: LGTM! Consider documenting utility functions.The reintroduced utility imports are essential for ROS2 message handling. Consider adding inline comments to explain their purpose.
-from rai.tools.ros.utils import import_message_from_str, wait_for_message +# Utility functions for ROS2 message type conversion and synchronous message waiting +from rai.tools.ros.utils import ( + import_message_from_str, # Converts message type string to message class + wait_for_message, # Synchronously waits for a message on a topic +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
examples/agriculture-demo.py(1 hunks)examples/manipulation-demo-streamlit.py(0 hunks)examples/manipulation-demo.py(0 hunks)examples/rosbot-xl-demo.py(1 hunks)examples/taxi-demo.py(1 hunks)pyproject.toml(3 hunks)src/examples/turtlebot4/turtlebot_demo.py(0 hunks)src/rai/LICENSE(0 hunks)src/rai/package.xml(0 hunks)src/rai/rai/agents/conversational_agent.py(1 hunks)src/rai/rai/agents/state_based.py(1 hunks)src/rai/rai/agents/tool_runner.py(1 hunks)src/rai/rai/agents/voice_agent.py(1 hunks)src/rai/rai/apps/high_level_api.py(0 hunks)src/rai/rai/apps/talk_to_docs.py(0 hunks)src/rai/rai/cli/rai_cli.py(0 hunks)src/rai/rai/communication/hri_connector.py(0 hunks)src/rai/rai/communication/ros2/api.py(1 hunks)src/rai/rai/communication/ros2/connectors.py(1 hunks)src/rai/rai/communication/sound_device_connector.py(1 hunks)src/rai/rai/node.py(1 hunks)src/rai/rai/ros2_apis.py(1 hunks)src/rai/rai/tools/debugging_assistant.py(0 hunks)src/rai/rai/tools/ros/deprecated.py(1 hunks)src/rai/rai/tools/ros/manipulation.py(1 hunks)src/rai/rai/tools/ros/nav2/basic_navigator.py(0 hunks)src/rai/rai/tools/ros/tools.py(1 hunks)src/rai/rai/tools/ros2/actions.py(0 hunks)src/rai/rai/tools/ros2/services.py(0 hunks)src/rai/rai/tools/ros2/topics.py(1 hunks)src/rai/rai/tools/utils.py(1 hunks)src/rai/rai/utils/ros_logs.py(1 hunks)src/rai/setup.cfg(0 hunks)src/rai/setup.py(0 hunks)src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py(1 hunks)src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/segmentation_tools.py(1 hunks)src/rai_extensions/rai_open_set_vision/rai_open_set_vision/vision_markup/segmenter.py(1 hunks)src/rai_hmi/rai_hmi/agent.py(1 hunks)src/rai_hmi/rai_hmi/base.py(1 hunks)src/rai_hmi/rai_hmi/ros.py(1 hunks)src/rai_hmi/rai_hmi/text_hmi.py(1 hunks)src/rai_hmi/rai_hmi/voice_hmi.py(1 hunks)src/rai_interfaces/package.xml(1 hunks)src/rai_whoami/rai_whoami/rai_whoami_node.py(1 hunks)tests/communication/ros2/test_api.py(1 hunks)tests/communication/ros2/test_connectors.py(1 hunks)tests/communication/test_hri_message.py(0 hunks)tests/communication/test_sound_device_connector.py(0 hunks)tests/conftest.py(1 hunks)tests/core/test_rai_cli.py(0 hunks)tests/core/test_ros2_tools.py(1 hunks)tests/core/test_tool_runner.py(0 hunks)tests/messages/test_multimodal.py(0 hunks)tests/messages/test_transport.py(1 hunks)tests/messages/test_utils.py(0 hunks)tests/tools/ros2/test_action_tools.py(1 hunks)tests/tools/ros2/test_service_tools.py(1 hunks)tests/tools/ros2/test_topic_tools.py(1 hunks)tests/tools/test_tool_utils.py(0 hunks)
💤 Files with no reviewable changes (22)
- tests/messages/test_utils.py
- tests/core/test_rai_cli.py
- tests/communication/test_hri_message.py
- examples/manipulation-demo.py
- src/rai/setup.cfg
- src/rai/rai/tools/ros/nav2/basic_navigator.py
- src/rai/rai/tools/ros2/services.py
- src/rai/LICENSE
- src/rai/rai/tools/debugging_assistant.py
- examples/manipulation-demo-streamlit.py
- src/rai/setup.py
- tests/communication/test_sound_device_connector.py
- src/rai/rai/apps/high_level_api.py
- src/rai/package.xml
- tests/core/test_tool_runner.py
- tests/tools/test_tool_utils.py
- tests/messages/test_multimodal.py
- src/rai/rai/tools/ros2/actions.py
- src/rai/rai/cli/rai_cli.py
- src/rai/rai/apps/talk_to_docs.py
- src/examples/turtlebot4/turtlebot_demo.py
- src/rai/rai/communication/hri_connector.py
🔇 Additional comments (32)
tests/communication/ros2/test_connectors.py (2)
19-19: LGTM! Import statement aligns with the new package structure.The import statement correctly reflects the package restructuring while maintaining the same functionality.
Line range hint
156-167: Address the TODO comment regarding blocking callbacks.The skipped test indicates an unresolved issue with callback handling during shutdown. This should be addressed to ensure proper cleanup of resources.
Would you like me to help implement a solution for handling blocking callbacks during shutdown? This could involve:
- Implementing a callback cancellation mechanism
- Adding a timeout for callback execution
- Ensuring proper cleanup of callback threads
tests/communication/ros2/test_api.py (2)
21-21: LGTM! Import statement aligns with the new package structure.The import statement correctly reflects the package restructuring while maintaining the same functionality.
Line range hint
124-143: Implement proper ROS2 node discovery mechanism.The test is currently marked as xfail due to asynchronous ROS2 node discovery. This could lead to reliability issues in production.
Would you like me to help implement a proper discovery mechanism with timeout? This would involve:
- Adding a discovery wait loop with configurable timeout
- Implementing node availability checks
- Adding retry logic with backoff
pyproject.toml (2)
3-3: LGTM! Version bump is appropriate.The version increment from 1.0.0 to 1.1.0 aligns well with semantic versioning principles, given the significant but backward-compatible changes in package structure.
82-83: LGTM! Build system configuration is correct.The transition to poetry-core as the build backend is appropriate for a Poetry-managed package. The version constraint
>=1.0.0is suitable.src/rai/rai/tools/ros/deprecated.py (2)
24-24: LGTM! Import statement reorganization looks good.The import statement has been moved to a more logical position, maintaining functionality while improving code organization.
Line range hint
1-114: Consider migration strategy for ROS2 dependencies.This file heavily relies on ROS2 dependencies (
rclpy,sensor_msgs, etc.) which conflicts with the PR's objective of converting to a pure Python package. While the file is appropriately marked as "deprecated", we should discuss the migration strategy.Let's verify the extent of ROS2 dependencies in the codebase:
Questions to consider:
- Is there a timeline for deprecating this ROS2-dependent code?
- Should we create non-ROS2 alternatives for these utilities?
- Do we need to document migration guides for users?
src/rai/rai/tools/utils.py (2)
29-29: LGTM on the import change.The import statement is correctly modified to maintain the same functionality.
Line range hint
198-300: Verify shell commands in ROS2 utility classes.The following classes use shell commands that assume ROS2 CLI is available:
ReadAvailableTopicsReadAvailableNodesReadAvailableServicesReadAvailableActionsThis might break in a pure Python environment without ROS2 installed.
Let's verify the impact:
✅ Verification successful
Unused ROS2 utility classes can be safely removed
The ROS2 utility classes (
ReadAvailableTopics,ReadAvailableNodes,ReadAvailableServices) are not used anywhere in the codebase. While they do assume ROS2 CLI availability, this isn't currently a problem as the code is effectively dead. Consider removing these unused classes to prevent future maintenance issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if these classes are used in the codebase # and if there are any tests that depend on them # Search for usage of these classes echo "Searching for usage of ROS2 utility classes..." rg -l "ReadAvailable(Topics|Nodes|Services|Actions)" # Search for related test files echo "Searching for related test files..." fd -g "*test*.py" | xargs rg -l "ReadAvailable(Topics|Nodes|Services|Actions)"Length of output: 346
Script:
#!/bin/bash # Search for imports of these classes echo "Searching for imports..." rg "from.*utils.*import.*ReadAvailable|import.*ReadAvailable" # Look for configuration files echo "Searching configuration files..." fd -e yaml -e json -e toml | xargs rg "ReadAvailable" # Check if utils.py is imported anywhere echo "Checking utils.py imports..." rg "from.*utils.*import|import.*utils"Length of output: 5601
src/rai/rai/tools/ros2/topics.py (1)
31-31: Verify ROS2 dependencies in Poetry configuration.The added imports are correctly placed in a ROS2-specific module and properly guarded. However, since we're transitioning to Poetry, ensure these ROS2 dependencies are correctly specified in
pyproject.toml.✅ Verification successful
ROS2 dependencies correctly managed outside of Poetry configuration
The current setup is correct. ROS2-specific imports like
sensor_msgsare intentionally not included inpyproject.tomlas they should be provided by the ROS2 environment when needed. This aligns with the project's architecture where ROS2 code is isolated in dedicated directories and wrapped in try-except blocks for optional integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ROS2 dependencies are properly configured in pyproject.toml echo "Checking pyproject.toml for ROS2 dependencies..." if [ -f "pyproject.toml" ]; then # Look for sensor_msgs and other ROS2 dependencies grep -A 10 "\[tool.poetry.dependencies\]" pyproject.toml else echo "pyproject.toml not found" fiLength of output: 435
Script:
#!/bin/bash # Display complete pyproject.toml and search for ROS2-related terms if [ -f "pyproject.toml" ]; then echo "=== Complete pyproject.toml content ===" cat pyproject.toml echo -e "\n=== Searching for ROS2-related terms ===" echo "Searching for: ros, sensor_msgs, rclpy..." grep -i -E "ros|sensor_msgs|rclpy" pyproject.toml || echo "No ROS2-related terms found" else echo "pyproject.toml not found" fiLength of output: 3074
src/rai/rai/utils/ros_logs.py (1)
27-27: Verify the existence of ros_async module in Poetry package structure.Since we're transitioning to Poetry, ensure that the
rai.utils.ros_asyncmodule is properly included in the package structure and thatget_future_resultis available.✅ Verification successful
Import of get_future_result is valid and available
The
get_future_resultfunction exists in theros_asyncmodule, which is properly placed within the Poetry package structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the ros_async module exists and contains get_future_result echo "Checking for ros_async module..." if [ -f "src/rai/rai/utils/ros_async.py" ]; then echo "Found ros_async.py, checking for get_future_result..." grep -l "def get_future_result" src/rai/rai/utils/ros_async.py else echo "ros_async.py not found" fiLength of output: 331
src/rai_whoami/rai_whoami/rai_whoami_node.py (1)
21-21: LGTM!The import statement is correctly formatted and follows Python import conventions.
src/rai_hmi/rai_hmi/base.py (2)
25-26: LGTM!The import statements are correctly formatted and follow Python import conventions.
Line range hint
1-1: Verify remaining ROS2 dependencies across the codebase.Let's check for remaining ROS2 dependencies to ensure a complete transition to a pure Python package.
✅ Verification successful
No ROS2 dependencies found in the reviewed file
The file
src/rai_hmi/rai_hmi/base.pyis free of ROS2 dependencies. The ROS2-related code found in the codebase is properly isolated in dedicated ROS2-specific modules and example files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining ROS2 dependencies across the codebase echo "Checking for ROS2-related imports..." rg -l 'import.*\b(rclpy|ament_index_python|std_msgs|std_srvs)\b' echo -e "\nChecking for ROS2 Node inheritance..." ast-grep --pattern 'class $_ (Node):' echo -e "\nChecking for ROS2 decorators..." rg '@\s*(rclpy|node)\.'Length of output: 1790
src/rai/rai/communication/sound_device_connector.py (1)
19-19: LGTM! Verify the import path.The import statement aligns with the PR's objective of converting to a pure Python package. The relative import path looks correct.
Let's verify that the imported modules exist in the correct location:
✅ Verification successful
Import statement is correct and properly used
The imported classes
HRIConnectorandHRIMessageare:
- Defined in
hri_connector.py- Correctly exported through
communication/__init__.py- Properly used in the code for inheritance and type annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and location of the imported modules fd -t f "hri_connector.py|hri_message.py" src/rai/rai/communication/Length of output: 113
Script:
#!/bin/bash # Search for HRIMessage class definition ast-grep --pattern 'class HRIMessage'Length of output: 39
Script:
#!/bin/bash # Search for HRIMessage with context rg -A 3 -B 3 "HRIMessage" src/rai/rai/communication/Length of output: 6981
tests/tools/ros2/test_service_tools.py (2)
Line range hint
16-22: LGTM! Proper ROS2 dependency handling.The code correctly handles the case when ROS2 is not installed by skipping the tests at module level.
Line range hint
28-33: Verify helper functions availability.The test now uses helper functions from
tests.communication.ros2.helpers. Since we're transitioning to a pure Python package, we need to ensure these helpers are properly maintained.✅ Verification successful
Helper functions are properly maintained and well-implemented ✅
The helper functions are actively maintained, properly implemented, and extensively used across the test suite. The code shows recent maintenance (2024 copyright) and follows good practices including proper typing, error handling, and cleanup mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the helper functions exist and are maintained # Test: Check if the helper module exists and contains the required functions rg -l "class ServiceServer" "tests/communication/ros2" rg -l "def multi_threaded_spinner" "tests/communication/ros2" rg -l "def ros_setup" "tests/communication/ros2" rg -l "def shutdown_executors_and_threads" "tests/communication/ros2"Length of output: 379
Script:
#!/bin/bash # Get implementations and check maintenance status echo "=== Implementations ===" ast-grep --pattern 'class ServiceServer { $$$ }' echo -e "\n=== Function Implementations ===" ast-grep --pattern 'def multi_threaded_spinner($_) { $$$ }' ast-grep --pattern 'def ros_setup($_) { $$$ }' ast-grep --pattern 'def shutdown_executors_and_threads($_) { $$$ }' echo -e "\n=== Maintenance Status ===" rg -B1 -A1 "(TODO|FIXME|DEPRECATED)" "tests/communication/ros2/helpers.py" echo -e "\n=== Usage Count ===" rg "ServiceServer" "tests/communication/ros2" --count rg "multi_threaded_spinner" "tests/communication/ros2" --count rg "ros_setup" "tests/communication/ros2" --count rg "shutdown_executors_and_threads" "tests/communication/ros2" --countLength of output: 1379
Script:
#!/bin/bash # Get the actual implementations with modified patterns echo "=== Helper Implementations ===" cat tests/communication/ros2/helpers.py echo -e "\n=== Recent Changes ===" git log -p --since="3 months ago" tests/communication/ros2/helpers.pyLength of output: 5844
src/rai_hmi/rai_hmi/agent.py (1)
Line range hint
33-44: LGTM! Well-structured mission memory tool.The
get_mission_memorytool is well-documented and properly typed.src/rai/rai/agents/conversational_agent.py (1)
26-29: LGTM! Proper type definition for loggers.The Union type for loggers correctly supports both RcutilsLogger and logging.Logger.
tests/tools/ros2/test_topic_tools.py (1)
Line range hint
31-39: LGTM! Imports are well-organized.The new imports from
tests.communication.ros2.helpersare properly organized and provide essential test utilities for ROS2 communication.examples/taxi-demo.py (1)
27-28: LGTM! Required import for ROS2 message handling.The
Stringmessage type fromstd_msgs.msgis correctly imported for handling human messages.examples/rosbot-xl-demo.py (1)
36-36: LGTM! Required imports for vision tools.The imports from
rai_open_set_vision.toolsare correctly added for vision-related functionality.src/rai/rai/agents/voice_agent.py (1)
31-31: LGTM! Required imports for voice recognition.The imports from
rai_asr.modelsare correctly added for voice recognition functionality.src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py (1)
22-26: LGTM! Essential imports restored.The reintroduction of these imports is necessary for the proper functioning of ROS message handling and image processing capabilities.
src/rai_interfaces/package.xml (1)
18-24: LGTM! Dependencies align with functionality requirements.The added dependencies are necessary for:
- ROS message generation (buildtool dependencies)
- Navigation functionality (nav2_* packages)
- Quaternion conversions (tf_transformations)
✅ Verification successful
Dependencies verified and available in ROS 2 Humble
All required dependencies are confirmed to be available and properly maintained in the ROS 2 Humble distribution:
- Build tool dependencies (rosidl_default_generators, ament_cmake) are part of core ROS 2 packages
- Navigation2 stack packages (nav2_msgs, nav2_simple_commander) are available through the navigation2 repository
- tf_transformations is present as a supported package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if these packages are available in the ROS distribution. # Test: Search for version constraints in rosdep yaml files. curl -s https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/base.yaml | rg "nav2_|tf_transformations"Length of output: 113
Script:
#!/bin/bash # Check multiple sources for package availability # Check Python packages echo "Checking Python packages..." curl -s https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/python.yaml | rg "nav2_|tf_transformations" # Check ROS packages index echo -e "\nChecking ROS packages..." curl -s https://raw.githubusercontent.com/ros/rosdistro/master/humble/distribution.yaml | rg -A 2 "nav2_msgs:|nav2_simple_commander:|tf_transformations:|rosidl_default_generators:|ament_cmake:"Length of output: 531
Script:
#!/bin/bash # Broader search for Navigation2 and ROSIDL packages echo "Checking Navigation2 and ROSIDL packages..." curl -s https://raw.githubusercontent.com/ros/rosdistro/master/humble/distribution.yaml | rg -A 3 "navigation2:|rosidl" # Also check source repositories echo -e "\nChecking source repositories..." curl -s https://raw.githubusercontent.com/ros/rosdistro/master/humble/source.yaml | rg -A 3 "navigation2:|rosidl"Length of output: 4216
src/rai_hmi/rai_hmi/ros.py (1)
22-22: LGTM! Import ordering follows style guidelines.The import reordering maintains a clean separation between standard library, third-party, and local imports.
src/rai_extensions/rai_open_set_vision/rai_open_set_vision/vision_markup/segmenter.py (1)
23-23: Verify ROS2 dependencies in Poetry configuration.Since this package is being converted to a pure Python package, ensure that ROS2 dependencies (
sensor_msgs,cv_bridge) are properly declared inpyproject.toml.tests/conftest.py (1)
25-25: Verify tabulate dependency in Poetry configuration.Ensure that the
tabulatepackage is properly declared inpyproject.tomlsince it's used for test reporting.✅ Verification successful
Tabulate dependency is properly managed ✅
The package is correctly declared in Poetry configuration files (pyproject.toml and poetry.lock).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if tabulate is declared in pyproject.toml echo "Checking pyproject.toml for tabulate dependency..." cat pyproject.toml | grep -A 10 "\[tool.poetry.dev-dependencies\]"Length of output: 226
Script:
#!/bin/bash # Look for Python dependency management files echo "Searching for dependency management files..." fd -e txt -e py -e cfg -e ini -d 2 "requirements|setup|pip" echo -e "\nChecking content of requirements.txt if exists..." [ -f requirements.txt ] && cat requirements.txt echo -e "\nChecking content of setup.py if exists..." [ -f setup.py ] && cat setup.py echo -e "\nChecking for requirements directory..." [ -d requirements ] && ls -la requirements/Length of output: 545
Script:
#!/bin/bash # Look for Python project structure and tabulate dependency echo "Searching for Python files in root..." fd -e py -d 1 echo -e "\nSearching for any file containing tabulate reference..." rg -l "tabulate" . echo -e "\nSearching for any dependency-related files in the entire project..." fd -t f "deps|dependencies|requirements|poetry|setup|pip"Length of output: 933
src/rai_hmi/rai_hmi/voice_hmi.py (1)
Line range hint
67-68: Address TODO comment regarding ROS2 node requirement.The TODO comment suggests that the RaiBaseNode might not be necessary for simple ROS2 tools. Consider refactoring this as part of the Python package transition.
Would you like me to help investigate alternative approaches that don't require a separate ROS2 node for tool initialization?
src/rai/rai/communication/ros2/connectors.py (1)
21-22: Verify the architectural decision regarding ROS2 support.The file's location under the
ros2subdirectory suggests ROS2 support is being maintained as an optional feature. However, this seems to contradict the PR objective of converting to a pure Python package. Please clarify:
- Is ROS2 support intended to be optional?
- How will the package handle ROS2 dependencies in Poetry?
- Should these dependencies be marked as optional in pyproject.toml?
src/rai/rai/agents/state_based.py (1)
32-32: Implement consistent logger abstraction across files.This file shows the same pattern of handling both ROS2 and standard loggers. Consider applying the logger abstraction suggested in
tool_runner.pyhere as well for consistency.Also applies to: 34-34
src/rai_extensions/rai_open_set_vision/rai_open_set_vision/tools/segmentation_tools.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@maciejmajek Nice work! Did you manage to run open-set vision on this PR?
I tried manipulation demo and I have problems with imports:
See log
[grounding_dino-2] Traceback (most recent call last):
[grounding_dino-2] File "/home/bboczek/projects/01_internal/01_repos/12_rai/install/rai_open_set_vision/lib/rai_open_set_vision/grounding_dino", line 33, in <module>
[grounding_dino-2] sys.exit(load_entry_point('rai-open-set-vision', 'console_scripts', 'grounding_dino')())
[grounding_dino-2] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[grounding_dino-2] File "/home/bboczek/projects/01_internal/01_repos/12_rai/install/rai_open_set_vision/lib/rai_open_set_vision/grounding_dino", line 25, in importlib_load_entry_point
[grounding_dino-2] return next(matches).load()
[grounding_dino-2] ^^^^^^^^^^^^^^^^^^^^
[grounding_dino-2] File "/usr/lib/python3.12/importlib/metadata/__init__.py", line 205, in load
[grounding_dino-2] module = import_module(match.group('module'))
[grounding_dino-2] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[grounding_dino-2] File "/usr/lib/python3.12/importlib/__init__.py", line 90, in import_module
[grounding_dino-2] return _bootstrap._gcd_import(name[level:], package, level)
[grounding_dino-2] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1310, in _find_and_load_unlocked
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1310, in _find_and_load_unlocked
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
[grounding_dino-2] File "<frozen importlib._bootstrap_external>", line 995, in exec_module
[grounding_dino-2] File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
[grounding_dino-2] File "/home/bboczek/projects_u24/01_internal/01_repos/12_rai/build/rai_open_set_vision/rai_open_set_vision/__init__.py", line 17, in <module>
[grounding_dino-2] from .tools import GetDetectionTool, GetDistanceToObjectsTool
[grounding_dino-2] File "/home/bboczek/projects_u24/01_internal/01_repos/12_rai/build/rai_open_set_vision/rai_open_set_vision/tools/__init__.py", line 15, in <module>
[grounding_dino-2] from .gdino_tools import GetDetectionTool, GetDistanceToObjectsTool
[grounding_dino-2] File "/home/bboczek/projects_u24/01_internal/01_repos/12_rai/build/rai_open_set_vision/rai_open_set_vision/tools/gdino_tools.py", line 22, in <module>
[grounding_dino-2] from rai.node import RaiBaseNode
[grounding_dino-2] ModuleNotFoundError: No module named 'rai' 
|
@maciejmajek If I only run open-set vision, the problem persists. It seems like the problem might lay in |
maciejmajek
force-pushed
the
refactor/rai-package-again
branch
from
24666c4 to
e890f3d
Compare
Purpose
Convert RAI (src/rai) from a ROS2 package to a pure Python package managed by Poetry. This change significantly improves the development experience by:
Note
This PR does not remove the need to build other packages (src/rai-*)
Proposed Changes
Testing
poetry installSummary by CodeRabbit
Based on the comprehensive summary, here are the high-level release notes for version 1.1.0:
Release Notes v1.1.0
New Features
Improvements
Bug Fixes
Documentation
Chores