-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: unified connector API #368
base: development
Are you sure you want to change the base?
Conversation
@coderabbitai full review |
WalkthroughThis pull request introduces significant enhancements to the communication module within the RAI (Robot AI) framework. The changes include the addition of new connector classes for Robot-Robot Interaction (RRI) and Human-Robot Interaction (HRI), along with updates to the base communication infrastructure. The modifications introduce more flexible and type-safe communication mechanisms, with a focus on supporting multimodal messaging and generic connector implementations. Changes
Sequence DiagramsequenceDiagram
participant User
participant HRIConnector
participant HRIMessage
participant LangchainMessage
User->>HRIConnector: send_all_targets(message)
HRIConnector->>HRIMessage: from_langchain(message)
HRIMessage-->>HRIConnector: HRIMessage instance
HRIConnector->>HRIConnector: send message to configured targets
HRIConnector->>HRIConnector: receive_all_sources()
HRIConnector->>HRIMessage: to_langchain()
HRIMessage-->>LangchainMessage: Convert to Langchain message
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/rai/rai/communication/ari_connector.py (2)
22-23
: Consider adding documentation and validation rules to ARIMessage.The empty class could benefit from docstring explaining its purpose and any validation rules specific to ARI messages.
class ARIMessage(BaseMessage): + """Base message type for Agent-Robot Interface (ARI) communication. + + This class serves as a marker class and defines the contract for all ARI messages. + Inherit from this class to create specific ARI message types. + """ pass
36-39
: Enhance ARIConnector documentation.The docstring could provide more details about the connector's purpose and usage.
class ARIConnector(BaseConnector[ARIMessage]): """ - Base class for Robot-Robot Interaction (RRI) connectors. + Base class for Agent-Robot Interface (ARI) connectors. + + This class provides the foundation for implementing connectors that facilitate + communication between software agents and robots. It ensures type safety by + accepting only ARIMessage-based messages. + + Usage: + Inherit from this class to implement specific ARI connectors, such as + ROS2-based or custom protocol-based connectors. """src/rai/rai/communication/sound_device_connector.py (4)
89-90
: Fix typo in error message.The error message contains a duplicate word "messages".
def receive_message(self, source: str, timeout_sec: float = 1.0) -> HRIMessage: raise SoundDeviceError( - "StreamingAudioInputDevice does not support receiving messages messages" + "StreamingAudioInputDevice does not support receiving messages" )
99-104
: Document new parameters in start_action method.The method signature has been updated but lacks documentation for the new parameters.
def start_action( self, action_data: Optional[HRIMessage], target: str, on_feedback: Callable[[np.ndarray, dict[str, Any]], None], on_done: Callable = lambda _: None, timeout_sec: float = 1.0, ) -> str: + """Start streaming audio from the specified device. + + Args: + action_data: Optional message containing action parameters + target: Device ID to stream from + on_feedback: Callback for processing audio data + on_done: Callback invoked when streaming ends + timeout_sec: Timeout in seconds for starting the stream + + Returns: + str: Handle for managing the stream + + Raises: + SoundDeviceError: If device is not configured or initialization fails + """
65-65
: Update class documentation for HRIConnector inheritance.The class should document its relationship with the HRIConnector hierarchy.
class StreamingAudioInputDevice(HRIConnector): + """Audio input device connector implementing the Human-Robot Interface. + + This class provides audio streaming capabilities while conforming to the + HRIConnector interface. It supports starting and stopping audio streams + but does not implement message passing or service calls. + """
93-96
: Consider implementing service_call for device configuration.The service_call method could be utilized for device configuration instead of using a separate configure_device method.
This would provide a more consistent interface across connectors. Example implementation:
def service_call( self, message: HRIMessage, target: str, timeout_sec: float = 1.0 ) -> HRIMessage: if isinstance(message, AudioConfigurationMessage): self.configure_device(target, message.config) return AudioConfigurationResponse(success=True) raise SoundDeviceError("Unsupported service type")src/rai/rai/communication/base_connector.py (1)
21-22
:BaseMessage
should not inherit fromABC
without abstract methods.The
BaseMessage
class is inheriting fromABC
but does not define any abstract methods or properties. This may be unnecessary and could be misleading.Apply this diff to remove the
ABC
inheritance:-from abc import ABC, abstractmethod -class BaseMessage(ABC): +class BaseMessage: def __init__(self, payload: Any, *args, **kwargs): self.payload = payloadAlternatively, if
BaseMessage
is intended to be abstract, consider adding abstract methods.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/rai/rai/communication/__init__.py
(1 hunks)src/rai/rai/communication/ari_connector.py
(1 hunks)src/rai/rai/communication/base_connector.py
(1 hunks)src/rai/rai/communication/hri_connector.py
(1 hunks)src/rai/rai/communication/sound_device_connector.py
(4 hunks)tests/communication/test_sound_device_connector.py
(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/rai/rai/communication/base_connector.py
20-20: BaseMessage
is an abstract base class, but it has no abstract methods or properties
(B024)
🔇 Additional comments (6)
src/rai/rai/communication/ari_connector.py (1)
26-33
: 🛠️ Refactor suggestionAddress TODO and enhance field descriptions.
The TODO suggests moving this class to the ros2 module. Additionally, the field descriptions could be more precise.
Let's verify if there's a dedicated ros2 module:
Consider:
- Moving this class to a dedicated ROS2 module if it exists
- Enhancing field descriptions:
ros_message_type: str = Field( - description="The string representation of the ROS message type (e.g. 'std_msgs/String')" + description="The string representation of the ROS message type (e.g. 'std_msgs/msg/String')" ) python_message_class: Optional[type] = Field( - description="The Python class of the ROS message type", default=None + description="The Python class corresponding to the ROS message type (e.g. std_msgs.msg.String)", + default=None )src/rai/rai/communication/base_connector.py (2)
38-38
: Confirm the default timeout value is appropriate.The
receive_message
method now includes atimeout_sec
parameter with a default value of1.0
second. Ensure this default timeout is suitable for all expected use cases or consider making it configurable.
47-52
:⚠️ Potential issueVerify changes to
start_action
method signature across subclasses.The
start_action
method has been updated:
- Added
action_data: Optional[T]
parameter.- Renamed
on_finish
toon_done
.- Added
timeout_sec: float = 1.0
parameter.Please ensure all subclasses and usages of
BaseConnector
have been updated accordingly to prevent runtime errors.Run the following script to identify usages of
start_action
that may need updating:✅ Verification successful
All subclasses properly implement the updated
start_action
signature ✅The verification shows that:
SoundDeviceConnector
implements the method with all required parameters and correct namingHRIConnector
andARIConnector
inherit the base implementation without override- All test usages are compatible with the new signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'start_action' method. # Search for calls to 'start_action' in the codebase rg 'start_action\(' -A 5Length of output: 2418
Script:
#!/bin/bash # Find all classes that inherit from BaseConnector ast-grep --pattern 'class $_ (BaseConnector$_)' # Find any additional imports of BaseConnector to catch potential subclasses in other files rg "from.*base_connector.*import.*BaseConnector" --type pyLength of output: 399
Script:
#!/bin/bash # Check start_action implementations in potential subclass files rg "def start_action" src/rai/rai/communication/hri_connector.py src/rai/rai/communication/ari_connector.py -B 2 -A 6Length of output: 119
Script:
#!/bin/bash # Check content of potential subclass files echo "=== hri_connector.py ===" cat src/rai/rai/communication/hri_connector.py echo -e "\n=== ari_connector.py ===" cat src/rai/rai/communication/ari_connector.pyLength of output: 5656
src/rai/rai/communication/hri_connector.py (2)
82-83
: 🛠️ Refactor suggestionUpdate conditional checks to use the renamed attribute.
After renaming
self.type
toself.message_author
, ensure all conditional checks reflect this change.Apply this diff to update the conditional check:
if message.type not in ["ai", "human"]: - raise ValueError(f"Invalid message type: {message.type} for {cls.__name__}") + raise ValueError(f"Invalid message type: {message.message_author} for {cls.__name__}")Likely invalid or redundant comment.
51-67
: 🛠️ Refactor suggestionRefactor
match
statement for compatibility with Python versions below 3.10.The
match
statement is available in Python 3.10 and above. If your project supports older Python versions, refactor the code usingif-elif
statements.Apply this diff to refactor the
to_langchain
method:def to_langchain(self) -> LangchainBaseMessage: - match self.type: - case "human": + if self.message_author == "human": if self.images is None and self.audios is None: return HumanMessage(content=self.text) return HumanMultimodalMessage( content=self.text, images=self.images, audios=self.audios ) - case "ai": + elif self.message_author == "ai": if self.images is None and self.audios is None: return AIMessage(content=self.text) return AiMultimodalMessage( content=self.text, images=self.images, audios=self.audios ) - case _: + else: raise ValueError( - f"Invalid message type: {self.type} for {self.__class__.__name__}" + f"Invalid message type: {self.message_author} for {self.__class__.__name__}" )src/rai/rai/communication/__init__.py (1)
15-26
: Module exports updated appropriately.The additions of
ARIConnector
,ARIMessage
,HRIConnector
, andHRIMessage
to the module exports are correct and enhance the public API.
@@ -76,7 +76,7 @@ def test_start_action_failed_init( | |||
recording_device = 0 | |||
with pytest.raises(SoundDeviceError, match="Device 0 has not been configured"): | |||
_ = audio_input_device.start_action( | |||
str(recording_device), feedback_callback, finish_callback | |||
None, str(recording_device), feedback_callback, finish_callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance test coverage for new parameters.
The tests have been updated for the new action_data
parameter but could be improved:
- Test cases with non-None
action_data
are missing - No tests for the new
timeout_sec
parameter
Add test cases for:
def test_start_action_with_message():
# Test with a valid HRIMessage
message = HRIMessage() # or specific message type
stream_handle = audio_input_device.start_action(
message, device_id, feedback_callback, finish_callback
)
# Add assertions
def test_start_action_with_timeout():
# Test with custom timeout
stream_handle = audio_input_device.start_action(
None, device_id, feedback_callback, finish_callback, timeout_sec=2.0
)
# Add assertions
Also applies to: 106-106
message_author: Literal["ai", "human"], | ||
): | ||
self.type = message_author | ||
self.text = payload.text | ||
self.images = payload.images | ||
self.audios = payload.audios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid using type
as an attribute name.
Using type
as an attribute name can overshadow the built-in type
function, leading to potential confusion or bugs. Consider renaming self.type
to self.message_author
or another descriptive name.
Apply this diff to rename the attribute:
def __init__(
self,
payload: HRIPayload,
message_author: Literal["ai", "human"],
):
- self.type = message_author
+ self.message_author = message_author
self.text = payload.text
self.images = payload.images
self.audios = payload.audios
Ensure to update all references to self.type
in the class.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
message_author: Literal["ai", "human"], | |
): | |
self.type = message_author | |
self.text = payload.text | |
self.images = payload.images | |
self.audios = payload.audios | |
message_author: Literal["ai", "human"], | |
): | |
self.message_author = message_author | |
self.text = payload.text | |
self.images = payload.images | |
self.audios = payload.audios |
configured_targets: Sequence[str] | ||
configured_sources: Sequence[str] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize configured_targets
and configured_sources
in the constructor.
The attributes configured_targets
and configured_sources
are declared but not initialized. To avoid attribute errors, initialize them in the __init__
method.
Apply this diff to add an initializer:
class HRIConnector(BaseConnector[HRIMessage]):
"""
Base class for Human-Robot Interaction (HRI) connectors.
Used for sending and receiving messages between human and robot from various sources.
"""
+ def __init__(self, configured_targets: Sequence[str], configured_sources: Sequence[str]):
+ self.configured_targets = configured_targets
+ self.configured_sources = configured_sources
Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: Maciej Majek <[email protected]>
5dcde4e
to
d88adcf
Compare
Purpose
There should be a unified base class for both HRI (Human Robot Interface) and ARI (Agent Robot Interface) connectors.
Proposed Changes
Adds unified base class for HRI and ARI connectors.
Issues
Testing
Summary by CodeRabbit
New Features
ARIConnector
andARIMessage
classesHRIConnector
andHRIMessage
classesImprovements