feat: add basic implementation of the Connector interface and a StreamingAudioInputDevice Connector
#350
Conversation
rachwalk
force-pushed
the
refactor/rai_asr
branch
from
10a04c8 to
7503498
Compare
boczekbartek
changed the title
Connector interface and a StreamingAudioInputDevice Connector
rachwalk
force-pushed
the
refactor/rai_asr
branch
from
9dcf811 to
d3b9ece
Compare
|
@coderabbitai full review |
WalkthroughThis pull request introduces a comprehensive audio communication module for the RAI project. The changes include setting up a GitHub Actions workflow with audio device support, updating pytest configuration, and implementing a new communication framework. The core addition is a Changes
Sequence DiagramsequenceDiagram
participant User
participant StreamingAudioInputDevice
participant SoundDevice
User->>StreamingAudioInputDevice: configure_device()
StreamingAudioInputDevice-->>User: Device configured
User->>StreamingAudioInputDevice: start_action()
StreamingAudioInputDevice->>SoundDevice: Initialize input stream
SoundDevice-->>StreamingAudioInputDevice: Stream callback
StreamingAudioInputDevice->>User: Return action handle
User->>StreamingAudioInputDevice: terminate_action()
StreamingAudioInputDevice->>SoundDevice: Stop input stream
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: 3
🧹 Nitpick comments (5)
src/rai/rai/communication/base_connector.py (2)
20-22: Consider removing or justifying ABC inheritance inBaseMessage
BaseMessageis declared as an abstract base class but lacks abstract methods or properties. If you don't plan to add any, you can remove theABCinheritance to avoid confusion. Alternatively, you can introduce abstract methods or properties if you foresee extension.🧰 Tools
🪛 Ruff (0.8.2)
20-20:
BaseMessageis an abstract base class, but it has no abstract methods or properties(B024)
24-49: Add docstrings for public abstract methods
While the abstract methods inBaseConnectorare clear from their names and signatures, adding docstrings or usage examples would improve readability and clarity. Explaining the expected parameters, return types, and usage scenarios encourages consistent implementations in subclasses.src/rai/rai/communication/sound_device_connector.py (2)
66-83: Typo inconfigred_devices
self.configred_devicesappears to be a misspelling. Consider renaming it toself.configured_devicesfor clarity and consistency.- self.configred_devices: dict[str, ConfiguredDevice] = {} + self.configured_devices: dict[str, ConfiguredDevice] = {}
84-98: Typo in error messages: "suport"
The error messages raised insend_message,receive_message, andsend_and_waithave a small spelling mistake ("suport" instead of "support"). Fixing it will improve readability.- "StreamingAudioInputDevice does not suport sending messages" + "StreamingAudioInputDevice does not support sending messages"pyproject.toml (1)
90-90: Document CI-specific test requirements.The
addoptsconfiguration now excludes both billable and CI-only tests by default. Consider adding documentation about which tests require CI environment and why, to help developers understand when to use theci_onlymarker.Add a comment in the README.md or testing documentation:
+ ## Test Markers + + - `billable`: Tests that incur costs and are excluded by default + - `ci_only`: Tests that require CI environment (e.g., specific audio device setup) + + To run CI-only tests locally: `pytest -m "ci_only"`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/poetry-test.yml(2 hunks)pyproject.toml(1 hunks)src/rai/rai/communication/__init__.py(1 hunks)src/rai/rai/communication/base_connector.py(1 hunks)src/rai/rai/communication/sound_device_connector.py(1 hunks)tests/communication/test_sound_device_connector.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/communication/test_sound_device_connector.py
55-55: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
103-103: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
src/rai/rai/communication/base_connector.py
20-20: BaseMessage is an abstract base class, but it has no abstract methods or properties
(B024)
src/rai/rai/communication/sound_device_connector.py
136-136: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (7)
src/rai/rai/communication/base_connector.py (1)
1-14: License header check
The Apache License header is present and correctly formatted.tests/communication/test_sound_device_connector.py (3)
40-63: Device configuration tests
The test verifies thatconfigure_deviceapplies configuration attributes correctly, including consumer sampling rate, window size, target sampling rate, and data type. Looks good for coverage.🧰 Tools
🪛 Ruff (0.8.2)
55-55: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
65-82: Unconfigured device failure test
The testtest_start_action_failed_initcorrectly asserts that aSoundDeviceErroris raised when trying to start an action on an unconfigured device. This ensures robust error handling.
84-115: Action start test coverage
The test validates successful stream creation and callback handling. The usage of fixtures and mocks ensures reliable coverage of thestart_actionflow. Looks good.🧰 Tools
🪛 Ruff (0.8.2)
103-103: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
src/rai/rai/communication/__init__.py (1)
1-23: Re-exporting classes and errors
Importing and enumeratingBaseMessage,BaseConnector,StreamingAudioInputDevice, andSoundDeviceErrorin__all__helps define a clear public API. Looks good..github/workflows/poetry-test.yml (1)
41-46: Virtual audio device setup
Installing and runningjackdin dummy mode is a good approach for enabling audio-related tests in CI. This step looks properly integrated.pyproject.toml (1)
88-88: LGTM! Verify test markers in the codebase.The new
ci_onlymarker is correctly defined and follows the existing pattern.Let's verify the usage of this marker in test files:
✅ Verification successful
ci_onlymarker is correctly used in sound device testsThe new
ci_onlymarker is properly used intests/communication/test_sound_device_connector.pyfor three test functions that verify sound device configuration and initialization. This makes sense as these tests likely require specific audio hardware setup that's only available in the CI environment.
- Found in
test_sound_device_connector.py:
test_configure: Tests device configurationtest_start_action_failed_init: Tests failure handlingtest_start_action: Tests start action🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for tests marked with ci_only to understand the testing requirements # Expected: Find test files using the new marker # Search for @pytest.mark.ci_only decorators rg "@pytest\.mark\.ci_only" -t python # Also check for alternative syntax rg "pytest\.mark\.ci_only" -t pythonLength of output: 140
Script:
#!/bin/bash # Description: Check for tests marked with ci_only to understand the testing requirements # Let's try without the file type flag since it's not supported # Search for @pytest.mark.ci_only decorators rg "@pytest\.mark\.ci_only" # Also check for alternative syntax rg "pytest\.mark\.ci_only" # Let's also check for files that might contain pytest tests fd "test_.*\.py$"Length of output: 712
Script:
#!/bin/bash # Let's check the context of the ci_only markers to understand their usage rg "@pytest\.mark\.ci_only" -B 2 -A 5 tests/communication/test_sound_device_connector.pyLength of output: 558
| def start_action( | ||
| self, | ||
| target: str, | ||
| on_feedback: Callable[[np.ndarray, dict[str, Any]], None], | ||
| on_finish: Callable = lambda _: None, | ||
| ) -> str: | ||
|
|
||
| target_device = self.configred_devices.get(target) | ||
| if target_device is None: | ||
| raise SoundDeviceError(f"Device {target} has not been configured") | ||
|
|
||
| def callback(indata: np.ndarray, frames: int, _, status: CallbackFlags): | ||
| indata = indata.flatten() | ||
| sample_time_length = len(indata) / target_device.target_sampling_rate | ||
| if target_device.sample_rate != target_device.target_sampling_rate: | ||
| indata = resample(indata, int(sample_time_length * target_device.target_samping_rate)) # type: ignore | ||
| flag_dict = { | ||
| "input_overflow": status.input_overflow, | ||
| "input_underflow": status.input_underflow, | ||
| "output_overflow": status.output_overflow, | ||
| "output_underflow": status.output_underflow, | ||
| "priming_output": status.priming_output, | ||
| } | ||
| on_feedback(indata, flag_dict) | ||
|
|
||
| handle = self._generate_handle() | ||
| try: | ||
| stream = sd.InputStream( | ||
| samplerate=target_device.sample_rate, | ||
| channels=1, | ||
| device=int(target), | ||
| dtype=target_device.dtype, | ||
| blocksize=target_device.window_size_samples, | ||
| callback=callback, | ||
| finished_callback=on_finish, | ||
| ) | ||
| except AttributeError: | ||
| raise SoundDeviceError(f"Device {target} has not been correctly configured") | ||
| stream.start() | ||
| self.streams[handle] = stream | ||
| return handle | ||
|
|
||
| def terminate_action(self, action_handle: str): | ||
| self.streams[action_handle].stop() |
There was a problem hiding this comment.
Fix typos and enhance exception chaining
- The
resamplecall references the incorrect keytarget_samping_rate; it should readtarget_sampling_rate. - In the
except AttributeErrorblock, you can leverage exception chaining by usingraise <Exception> from excorraise SoundDeviceError(...) from Noneto distinguish the new exception from the original cause.
-indata = resample(indata, int(sample_time_length * target_device.target_samping_rate))
+indata = resample(indata, int(sample_time_length * target_device.target_sampling_rate))
...
except AttributeError as exc:
- raise SoundDeviceError(f"Device {target} has not been correctly configured")
+ raise SoundDeviceError(f"Device {target} has not been correctly configured") from exc📝 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.
| def start_action( | |
| self, | |
| target: str, | |
| on_feedback: Callable[[np.ndarray, dict[str, Any]], None], | |
| on_finish: Callable = lambda _: None, | |
| ) -> str: | |
| target_device = self.configred_devices.get(target) | |
| if target_device is None: | |
| raise SoundDeviceError(f"Device {target} has not been configured") | |
| def callback(indata: np.ndarray, frames: int, _, status: CallbackFlags): | |
| indata = indata.flatten() | |
| sample_time_length = len(indata) / target_device.target_sampling_rate | |
| if target_device.sample_rate != target_device.target_sampling_rate: | |
| indata = resample(indata, int(sample_time_length * target_device.target_samping_rate)) # type: ignore | |
| flag_dict = { | |
| "input_overflow": status.input_overflow, | |
| "input_underflow": status.input_underflow, | |
| "output_overflow": status.output_overflow, | |
| "output_underflow": status.output_underflow, | |
| "priming_output": status.priming_output, | |
| } | |
| on_feedback(indata, flag_dict) | |
| handle = self._generate_handle() | |
| try: | |
| stream = sd.InputStream( | |
| samplerate=target_device.sample_rate, | |
| channels=1, | |
| device=int(target), | |
| dtype=target_device.dtype, | |
| blocksize=target_device.window_size_samples, | |
| callback=callback, | |
| finished_callback=on_finish, | |
| ) | |
| except AttributeError: | |
| raise SoundDeviceError(f"Device {target} has not been correctly configured") | |
| stream.start() | |
| self.streams[handle] = stream | |
| return handle | |
| def terminate_action(self, action_handle: str): | |
| self.streams[action_handle].stop() | |
| def start_action( | |
| self, | |
| target: str, | |
| on_feedback: Callable[[np.ndarray, dict[str, Any]], None], | |
| on_finish: Callable = lambda _: None, | |
| ) -> str: | |
| target_device = self.configred_devices.get(target) | |
| if target_device is None: | |
| raise SoundDeviceError(f"Device {target} has not been configured") | |
| def callback(indata: np.ndarray, frames: int, _, status: CallbackFlags): | |
| indata = indata.flatten() | |
| sample_time_length = len(indata) / target_device.target_sampling_rate | |
| if target_device.sample_rate != target_device.target_sampling_rate: | |
| indata = resample(indata, int(sample_time_length * target_device.target_sampling_rate)) # type: ignore | |
| flag_dict = { | |
| "input_overflow": status.input_overflow, | |
| "input_underflow": status.input_underflow, | |
| "output_overflow": status.output_overflow, | |
| "output_underflow": status.output_underflow, | |
| "priming_output": status.priming_output, | |
| } | |
| on_feedback(indata, flag_dict) | |
| handle = self._generate_handle() | |
| try: | |
| stream = sd.InputStream( | |
| samplerate=target_device.sample_rate, | |
| channels=1, | |
| device=int(target), | |
| dtype=target_device.dtype, | |
| blocksize=target_device.window_size_samples, | |
| callback=callback, | |
| finished_callback=on_finish, | |
| ) | |
| except AttributeError as exc: | |
| raise SoundDeviceError(f"Device {target} has not been correctly configured") from exc | |
| stream.start() | |
| self.streams[handle] = stream | |
| return handle | |
| def terminate_action(self, action_handle: str): | |
| self.streams[action_handle].stop() |
🧰 Tools
🪛 Ruff (0.8.2)
136-136: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
@maciejmajek, here’s what we have from the latest script output:
-
Typo correction:
- The string
"target_samping_rate"no longer appears in the repository, confirming the mismatch is presumably corrected to"target_sampling_rate".
- The string
-
Exception chaining:
- We still don’t see
"from exc"usage orexcept AttributeError as excin the file. The script found anexcept AttributeError:but not with exception chaining (from exc).
- We still don’t see
Thus, it appears that the typo fix has been integrated, but the recommended exception chaining is not in place yet. If you wish to apply exception chaining, please modify that block to use, for example:
except AttributeError as exc:
raise SoundDeviceError(f"Device {target} has not been correctly configured") from excThat should address both points from your earlier feedback. Let me know if you have any other questions!
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Let's verify that the typo "target_samping_rate" is removed and replaced by "target_sampling_rate".
echo "Searching for 'target_samping_rate' occurrences..."
rg "target_samping_rate"
# Let's also verify that we're now using exception chaining with 'from exc' in the except block.
echo ""
echo "Searching for 'except AttributeError as exc' lines and the following lines..."
rg -A 3 "except AttributeError as exc" src/rai/rai/communication/sound_device_connector.py
Length of output: 416
Script:
#!/bin/bash
# Let's broaden our search efforts to see if the fix was applied anywhere, or if "from exc" occurs in the codebase.
# We will search for "from exc" usage, references to raising SoundDeviceError, and references to "AttributeError".
echo "Searching for 'from exc' usage..."
rg "from exc"
echo ""
echo "Searching for 'raise SoundDeviceError' lines to see if chaining might have been manually inserted..."
rg -A 3 "raise SoundDeviceError"
echo ""
echo "Searching for 'AttributeError' references in entire codebase..."
rg "AttributeError" -A 5
Length of output: 3554
There was a problem hiding this comment.
@rachwalk please apply exception chaining suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Purpose
The purpose of the connector interface is to provide an abstraction over different devices and communication protocols RAI agents may use. This connector will be used in the ASR refactor, to provide an easily configurable abstraction for sound devices.
Proposed Changes
Adds the Connector interface and implementation of it in the for of an
StreamingAudioInputDevice, and its required tests.Issues
#309
Testing
Unit tests for the
StreamingAudioInputDevicewere added.Summary by CodeRabbit
New Features
Tests
Chores