refactor: internal api #520
Conversation
|
@coderabbitai full review |
WalkthroughThe changes refactor the communication and initialization mechanisms across several modules. The ASR and TTS agent files now update their import statements and connector initialization, switching to dedicated ROS2 and sound device modules. In the core communication layer, several legacy connectors, messages, and configurations have been removed from the public API. In addition, new ROS2 API modules have been introduced under the Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/rai_core/rai/communication/ros2/__init__.py (1)
15-18: Consider removing TODO comments from production code.The TODO comments indicate that
IROS2MessageandTopicConfigshould not be part of the public API, yet they're still included in the exports. If these components should be private, consider moving them to a private module or using naming conventions (e.g., prefixing with underscore).-from .api import ( - IROS2Message, # TODO: IROS2Message should not be a part of the public API - TopicConfig, # TODO: TopicConfig should not be a part of the public API -) +from .api import ( + IROS2Message, + TopicConfig, +)src/rai_core/rai/communication/ros2/api/service.py (2)
48-75: Consider asynchronous alternatives for service calls.Using
wait_for_servicefollowed byservice_client.callprovides a synchronous approach that can block the node if the service is unavailable for longer periods. As an alternative, consider using asynchronous calls or timeouts/cancellation tokens to avoid blocking the executor.
79-90: Constructor argument validation might be required for safety.Although we import the service class by string and store it, there's minimal validation around its callback signature. If a user provides a non-callable or incorrect arguments in
callback, it could lead to runtime failures. Consider adding extra guard checks or try/except blocks to validate the callback’s correctness.src/rai_core/rai/communication/ros2/api/common.py (1)
55-101: QoS profile adaptation is coherent but consider a more explicit approach to partial matche 8000 s.The logic for falling back to
BEST_EFFORTorVOLATILEQoS if only some publishers provide stronger policies is correct. However, consider raising or logging a more detailed warning when there's a partial mismatch to help users debug subtle QoS conflicts.src/rai_core/rai/communication/ros2/api/action.py (2)
191-193: Re-raise exceptions with context (Ruff B904).When throwing a new
ValueErrorin anexceptblock, consider usingraise ValueError(...) from eto preserve exception context.- raise ValueError( - f"Failed to create action server: {str(e)}. Valid arguments are: {args}" - ) + raise ValueError( + f"Failed to create action server: {str(e)}. Valid arguments are: {args}" + ) from e🧰 Tools
🪛 Ruff (0.8.2)
191-193: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
197-254: Check concurrency blocking in 'send_goal'.The while loop with
time.sleep(0.01)might cause the thread to block if goals take longer to be accepted. Using an asynchronous or callback-based approach could improve performance under heavy load.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/rai_asr/rai_asr/agents/asr_agent.py(2 hunks)src/rai_core/rai/communication/__init__.py(0 hunks)src/rai_core/rai/communication/ros2/__init__.py(1 hunks)src/rai_core/rai/communication/ros2/api/__init__.py(1 hunks)src/rai_core/rai/communication/ros2/api/action.py(1 hunks)src/rai_core/rai/communication/ros2/api/common.py(1 hunks)src/rai_core/rai/communication/ros2/api/service.py(1 hunks)src/rai_core/rai/communication/ros2/api/topic.py(2 hunks)src/rai_tts/rai_tts/agents/tts_agent.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/rai_core/rai/communication/init.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/rai_tts/rai_tts/agents/tts_agent.py (6)
src/rai_core/rai/communication/ros2/api/common.py (1)
IROS2Message(49-52)src/rai_core/rai/communication/ros2/connectors/hri_connector.py (1)
ROS2HRIConnector(40-158)src/rai_core/rai/communication/ros2/messages.py (1)
ROS2HRIMessage(44-107)src/rai_core/rai/communication/ros2/api/topic.py (1)
TopicConfig(342-359)src/rai_core/rai/communication/sound_device/api.py (1)
SoundDeviceConfig(31-80)src/rai_core/rai/communication/sound_device/connector.py (1)
SoundDeviceConnector(38-161)
src/rai_core/rai/communication/ros2/api/__init__.py (4)
src/rai_core/rai/communication/ros2/api/action.py (1)
ROS2ActionAPI(74-287)src/rai_core/rai/communication/ros2/api/common.py (1)
IROS2Message(49-52)src/rai_core/rai/communication/ros2/api/service.py (1)
ROS2ServiceAPI(40-90)src/rai_core/rai/communication/ros2/api/topic.py (3)
ConfigurableROS2TopicAPI(362-536)ROS2TopicAPI(54-338)TopicConfig(342-359)
src/rai_core/rai/communication/ros2/__init__.py (2)
src/rai_core/rai/communication/ros2/api/common.py (1)
IROS2Message(49-52)src/rai_core/rai/communication/ros2/api/topic.py (1)
TopicConfig(342-359)
src/rai_core/rai/communication/ros2/api/common.py (2)
src/rai_core/rai/communication/ros2/connectors/ari_connector.py (1)
node(225-226)src/rai_core/rai/tools/ros/utils.py (1)
import_message_from_str(39-41)
src/rai_core/rai/communication/ros2/api/topic.py (1)
src/rai_core/rai/communication/ros2/api/common.py (3)
IROS2Message(49-52)adapt_requests_to_offers(55-100)build_ros2_msg(103-113)
src/rai_core/rai/communication/ros2/api/action.py (2)
src/rai_core/rai/communication/ros2/api/common.py (1)
IROS2Message(49-52)src/rai_core/rai/tools/ros/utils.py (1)
import_message_from_str(39-41)
🪛 Ruff (0.8.2)
src/rai_core/rai/communication/ros2/api/action.py
133-133: Do not perform function call QoSProfile in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
191-193: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test-ros2 (jazzy)
🔇 Additional comments (23)
src/rai_core/rai/communication/ros2/api/__init__.py (2)
15-19: Imports structure looks good.The file properly imports all necessary components from their respective modules, establishing a clear public API for the ROS2 API package.
20-27: Well-defined public API with appropriate exports.The
__all__list clearly defines which components should be part of the public API. This follows Python best practices and makes it easier for users to understand what's available.src/rai_tts/rai_tts/agents/tts_agent.py (4)
25-30: Updated imports align with new module structure.These import changes properly reflect the refactored API structure, now specifically importing from
rai.communication.ros2.
31-31: Sound device components now imported from dedicated module.Moving the sound device imports to their own dedicated module (
rai.communication.sound_device) improves code organization by separating different concerns.
225-226: Method correctly uses IROS2Message interface for type checking.The method properly utilizes the IROS2Message interface for parameter typing, which allows for better static type checking while maintaining the protocol-based approach.
245-246: Method correctly uses IROS2Message interface for type checking.Similar to the previous method, this one properly uses the IROS2Message interface for type checking while also correctly asserting the specific message type.
src/rai_asr/rai_asr/agents/asr_agent.py (3)
25-30: Updated imports align with new module structure.These import changes properly reflect the refactored API structure, now specifically importing from
rai.communication.ros2.
31-35: Sound device components now imported from dedicated module.Moving the sound device imports to a dedicated module improves code organization and separation of concerns.
85-90: Modified initialization pattern for connectors.The initialization pattern has been changed to assign connectors to an instance variable before calling the superclass constructor. This change provides better control over the initialization sequence.
src/rai_core/rai/communication/ros2/__init__.py (1)
22-29:❓ Verification inconclusive
Consistent API exposure in all.
The
__all__list correctly includes all the imported components. However, it still includesIROS2MessageandTopicConfigwhich the TODO comments suggest should not be part of the public API. Consider creating a plan to properly encapsulate these components if they're meant to be internal.Consider following up with a plan to address the TODO comments:
- Are these components actually used by external code?
- Can they be moved to a private module?
- Can they be renamed to indicate they're internal (e.g.,
_IROS2Message)?
🏁 Script executed:
#!/bin/bash # Check how IROS2Message and TopicConfig are used outside of the ros2 module rg --type py "IROS2Message|TopicConfig" --glob "!src/rai_core/rai/communication/ros2/**" src/Length of output: 557
Evaluate API Exposure for IROS2Message and TopicConfig
Our analysis confirms that both
IROS2MessageandTopicConfigare referenced outside theros2module (e.g., insrc/rai_tts/rai_tts/agents/tts_agent.py). Given this external dependency, any move to mark these components as internal will require a careful deprecation and migration plan. Please confirm whether these elements should remain part of the public API. If internalization is desired, outline the strategy for renaming/moving them (possibly with a deprecation timeline) while ensuring backwards compatibility.
- Locations to review:
src/rai_core/rai/communication/ros2/__init__.py(lines 22-29)- External usage found in
src/rai_tts/rai_tts/agents/tts_agent.pysrc/rai_core/rai/communication/ros2/api/service.py (3)
1-14: No issues with licensing header.The Apache 2.0 license text is included correctly and follows standard usage guidelines.
15-23: Imports are well organized.These imports cover standard library modules (uuid, typing) and neatly separate them from third-party and local dependencies.
40-46: Constructor logic looks good.Storing services in a dictionary keyed by handles aids clarity and reusability. No concurrency issues appear present here since each handle is unique and maintained locally.
src/rai_core/rai/communication/ros2/api/common.py (4)
1-14: No issues with licensing header.The Apache 2.0 license text is present and correct.
48-53: Protocol usage is appropriate.Using
IROS2MessagewithProtocolandruntime_checkableis an effective way to define a common interface for ROS2 messages across the codebase.
103-114: ROS2 message builder is concise and effective.The function cleanly distinguishes string-based imports from direct message types, leveraging
rosidl_runtime_py.set_message.set_message_fields.
116-123: Service request builder follows a consistent pattern.The usage of
service_typeto load the request class and subsequent field setup matches the overall approach. Good consistency withbuild_ros2_msg.src/rai_core/rai/communication/ros2/api/action.py (4)
1-14: License header is correctly included.No issues spotted.
74-81: Constructor concurrency approach is valid.Instantiating a
ThreadPoolExecutorfor feedback callbacks is a good approach. Just ensure eventual shutdown to prevent resource leaks. The code calls_callback_executor.shutdown(wait=False)inshutdown(), which is correct.
116-180: Action server creation is well-structured.You handle the dynamic import of the action type, creation of the server, and fallback logging. This fosters clean code separation between ROS2 node ownership and custom logic.
🧰 Tools
🪛 Ruff (0.8.2)
133-133: Do not perform function call
QoSProfilein argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
284-288: Graceful shutdown ensures resource cleanup.Shutting down the executor without waiting is a valid approach if no further callbacks are expected, preventing indefinite hangs.
src/rai_core/rai/communication/ros2/api/topic.py (2)
1-1: No concerns with the updated license header.
Thank you for including the correct copyright year.
46-50: New imports fromcommonare well-integrated.
All imported symbols (IROS2Message,adapt_requests_to_offers,build_ros2_msg) are properly used throughout the file. No issues found.
refactor: update __init__ imports to better reflect public api
4caa7c9 to
abd165c
Compare
There was a problem hiding this comment.
Why not wrap this functionality into a base class?
Purpose
Adhering to #516
Proposed Changes
Splitting ros2 api into separate files to isolate concerns and provide cleaner, smaller files.
Updating imports to reflect private and public api.
Issues
Testing
Summary by CodeRabbit
New Features
Refactor
Chores