feat!:hpm #21
feat!:hpm #21
Conversation
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces significant restructuring in the HiveMind listener's protocol handling. The changes focus on refactoring audio processing protocols, updating import statements, and modifying dependencies. The modifications involve renaming classes, introducing new protocol implementations, and consolidating audio handling logic into a more streamlined architecture. The changes impact how audio data is processed and how different components of the HiveMind system interact. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AudioBinaryProtocol
participant MessageBus
participant TransformerServices
Client->>AudioBinaryProtocol: Send audio data
AudioBinaryProtocol->>MessageBus: Publish audio events
MessageBus->>TransformerServices: Route audio processing requests
TransformerServices-->>AudioBinaryProtocol: Return processed data
Possibly related PRs
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
hivemind_listener/protocol.py (2)
190-211: Optional callback overwrite and potential future refactoring
- Overwriting
self.hm_protocol.disconnect_client_callbackmay clobber existing logic. Consider passing the callback reference as an explicit kwarg to the protocol at creation, or using a more flexible registration pattern, especially as indicated by the TODO.- Ensure that
self.agent_protocolis reliably set before these event handlers are registered to avoid attribute issues.Would you like help converting the TODO into a GitHub issue or implementing a kwarg-based approach for the callback reference?
239-239: Evaluate per-client bus instances
A newFakeBusis created for each client. While this may be intentional for isolation, consider whether a single bus or a more centralized approach could reduce overhead and simplify event management.requirements.txt (2)
8-9: Confirm necessity ofpybase64
Python’s standard library includesbase64; re-addingpybase64might be needed for performance or older usage patterns. Document reasons if they’re relevant.
10-11: Clarify timeline for future removal
Thehivemind-websocket-protocollibrary is marked as temporary. Consider creating a GitHub issue to track its removal and manage downstream effects.hivemind_listener/__init__.py (1)
104-105: Provision recommended
Configuringhm_protocolandbinary_data_protocoltogether is a sensible approach. However, consider passing plugin configs directly toAudioBinaryProtocolrather than relying on class-level assignments to avoid concurrency issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hivemind_listener/__init__.py(2 hunks)hivemind_listener/protocol.py(5 hunks)requirements.txt(1 hunks)
🔇 Additional comments (6)
hivemind_listener/protocol.py (3)
31-31: Import usage looks good
HiveMindClientConnection is used in the file, and the import aligns with references throughout.
35-35: Consistent use of BinaryDataHandlerProtocol
The new import matches the usage in AudioBinaryProtocol. This is clear and aligned with the new audio handling architecture.
179-180: Appropriate use of @dataclass
The class is properly decorated, and field defaults are clearly defined. This enhances readability and initialization consistency.
requirements.txt (1)
5-6: Check plugin-manager and bus-client versions
Both dependencies look valid. Ensure security and feature compatibility.
✅ Verification successful
Dependencies are using the latest available versions
The specified version constraints in requirements.txt are aligned with the latest releases on PyPI:
- hivemind-plugin-manager:
>=0.0.2,<1.0.0(latest is 0.0.2) - ovos-bus-client:
>=1.3.0,<2.0.0(latest is 1.3.0)
The version constraints are appropriate, allowing for compatible updates while preventing breaking changes through major version bumps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest versions on PyPI for hivemind-plugin-manager and ovos-bus-client
curl -s https://pypi.org/pypi/hivemind-plugin-manager/json | jq '.info.version'
curl -s https://pypi.org/pypi/ovos-bus-client/json | jq '.info.version'
Length of output: 167
hivemind_listener/__init__.py (2)
10-10: Updated import source
Switching to hivemind_websocket_protocol is straightforward. Verify that the new source is compatible with the rest of the system.
12-14: Reorganized imports
These protocol and plugin-related imports match the new service architecture.
✅ Verification successful
Imports are correctly used and integrated in the codebase
The verification confirms that all imported protocols are properly utilized within the codebase:
HiveMindListenerProtocolis used as the protocol handler in theHiveMindServiceinitializationOVOSProtocolis correctly set as the agent protocol in the service setupAudioBinaryProtocolis extensively used throughout the codebase for handling audio-related operations, including TTS, STT, and binary data processing
The imports are not only organized but are essential components actively used in the service architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify references to ensure consistency
rg -A5 'HiveMindListenerProtocol'
rg -A5 'OVOSProtocol'
rg -A5 'AudioBinaryProtocol'
Length of output: 7654
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
hivemind_listener/protocol.py (4)
Line range hint
67-106: Fix instance method definitions in AudioCallbacks.The methods are defined as class methods using
clsbut they should be instance methods usingselfas they operate on instance attributes.Apply this fix to all methods in the class:
- def listen_callback(cls): + def listen_callback(self): """ Callback triggered when listening starts. """ LOG.info("New loop state: IN_COMMAND") - cls.bus.emit(Message("mycroft.audio.play_sound", + self.bus.emit(Message("mycroft.audio.play_sound",Similar changes needed for
end_listen_callback,error_callback, andtext_callback.
Line range hint
179-207: Improve initialization robustness and type safety.The class initialization has several areas for improvement:
- Missing error handling for plugin initialization
- No cleanup if service initialization fails
- Missing type hints for class attributes
Add type hints and error handling:
@dataclass class AudioBinaryProtocol(BinaryDataHandlerProtocol): - plugins: Optional[PluginOptions] = None + plugins: Optional[PluginOptions] = field(default=None) + agent_protocol: MessageBusClient = field(init=True) + utterance_transformers: Optional[UtteranceTransformersService] = field(default=None) + metadata_transformers: Optional[MetadataTransformersService] = field(default=None) + dialog_transformers: Optional[DialogTransformersService] = field(default=None) + hm_protocol: Optional['AudioReceiverProtocol'] = field(default=None) + listeners: Dict[str, SimpleListener] = field(default_factory=dict) def __post_init__(self) -> None: + try: if self.plugins is None: self.plugins = PluginOptions() # Initialize services self._init_transformer_services() self._setup_event_handlers() + except Exception as e: + self._cleanup() + raise RuntimeError(f"Failed to initialize AudioBinaryProtocol: {e}")Consider extracting service initialization and event handler setup into separate methods for better organization.
🧰 Tools
🪛 Ruff (0.8.2)
187-187: Undefined name
AudioReceiverProtocol(F821)
Line range hint
314-397: Improve error handling and method organization in audio processing.The audio processing methods are handling multiple responsibilities and lack proper error handling. This could lead to silent failures or inconsistent state.
- Add error handling for audio processing
- Split methods into smaller, focused functions
- Add logging for debugging
Example for
handle_stt_handle_request:def handle_stt_handle_request(self, bin_data: bytes, sample_rate: int, sample_width: int, lang: str, client: HiveMindClientConnection) -> None: - LOG.debug(f"Received binary STT input: {len(bin_data)} bytes") - audio = sr.AudioData(bin_data, sample_rate, sample_width) - tx = self.plugins.stt.transcribe(audio, lang) - if tx: - utts = [t[0].rstrip(" '\"").lstrip(" '\"") for t in tx] - utts, context = self.transform_utterances(utts, lang) - context = self.metadata_transformers.transform(context) - m = Message("recognizer_loop:utterance", - {"utterances": utts, "lang": lang}, - context=context) - self.hm_protocol.handle_inject_agent_msg(m, client) - else: - LOG.info(f"STT transcription error for client: {client.peer}") - m = Message("recognizer_loop:speech.recognition.unknown") - client.send(HiveMessage(HiveMessageType.BUS, payload=m)) + try: + LOG.debug(f"Processing STT request from {client.peer}: {len(bin_data)} bytes") + audio = self._create_audio_data(bin_data, sample_rate, sample_width) + transcriptions = self._transcribe_audio(audio, lang) + + if transcriptions: + message = self._create_utterance_message(transcriptions, lang, client) + self.hm_protocol.handle_inject_agent_msg(message, client) + else: + self._handle_transcription_error(client) + + except Exception as e: + LOG.error(f"Failed to process STT request: {e}") + self._handle_transcription_error(client)
Line range hint
403-450: Add validation and error handling in message handlers.The message handlers directly access client dictionary and lack proper validation of message context. This could lead to KeyError exceptions or invalid message handling.
Add validation and error handling:
def handle_speak_synth(self, message: Message): + if not message.context.get("source"): + LOG.error("Message missing source in context") + return + + try: + client = self.hm_protocol.clients.get(message.context["source"]) + if not client: + LOG.error(f"Unknown client source: {message.context['source']}") + return + + lang = get_message_lang(message) + if not message.data.get("utterance"): + LOG.error("Message missing utterance in data") + return + message.data["utterance"], context = self.transform_dialogs(message.data["utterance"], lang) wav = self.get_tts(message) with open(wav, "rb") as f: bin_data = f.read() metadata = {"lang": lang, "file_name": wav.split("/")[-1], "utterance": message.data["utterance"]} metadata.update(context) payload = HiveMessage(HiveMessageType.BINARY, payload=bin_data, metadata=metadata, bin_type=HiveMindBinaryPayloadType.TTS_AUDIO) client.send(payload) + except Exception as e: + LOG.error(f"Failed to handle speak synth: {e}")Similar validation and error handling should be added to all message handlers.
🧹 Nitpick comments (2)
hivemind_listener/__init__.py (1)
104-106: Consider improving the service initialization design.While the changes align with the new architecture, there are a few potential improvements:
- Using a class method (
AudioBinaryProtocol.stop_listener) as a callback creates tight coupling.- This coupling might make unit testing more challenging.
Consider injecting the callback as a configuration parameter:
- callbacks=ClientCallbacks(on_disconnect=AudioBinaryProtocol.stop_listener), + callbacks=ClientCallbacks(on_disconnect=config.get("on_disconnect_callback", AudioBinaryProtocol.stop_listener)),This would make the code more flexible and easier to test.
hivemind_listener/protocol.py (1)
252-276: Reduce code duplication in transform methods.The
transform_utterancesandtransform_dialogsmethods share similar logic and could be refactored to use a common implementation.Consider this refactor:
+ def _transform(self, + content: Union[str, List[str]], + lang: str, + transformer: Union[UtteranceTransformersService, DialogTransformersService]) -> Tuple[Union[str, List[str]], Dict]: + original = content + context = {} + if content: + content, context = transformer.transform(content, dict(lang=lang)) + if original != content: + LOG.debug(f"transformed: {original} -> {content}") + return content, context - def transform_utterances(self, utterances: List[str], lang: str) -> Tuple[str, Dict]: + def transform_utterances(self, utterances: List[str], lang: str) -> Tuple[List[str], Dict]: - original = list(utterances) - context = {} - if utterances: - utterances, context = self.utterance_transformers.transform(utterances, dict(lang=lang)) - if original != utterances: - LOG.debug(f"utterances transformed: {original} -> {utterances}") - return utterances, context + return self._transform(utterances, lang, self.utterance_transformers) def transform_dialogs(self, utterance: str, lang: str) -> Tuple[str, Dict]: - original = utterance - context = {} - if utterance: - utterance, context = self.dialog_transformers.transform(utterance, dict(lang=lang)) - if original != utterance: - LOG.debug(f"speak transformed: {original} -> {utterance}") - return utterance, context + return self._transform(utterance, lang, self.dialog_transformers)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hivemind_listener/__init__.py(2 hunks)hivemind_listener/protocol.py(14 hunks)
🔇 Additional comments (1)
hivemind_listener/__init__.py (1)
10-14: LGTM! Import changes align with the new protocol structure.
The reorganization of imports reflects the architectural changes, properly separating websocket protocol handling and bus client functionality.
companion to JarbasHiveMind/HiveMind-core#28
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Refactor