feat/transformers #83
Conversation
closes #82
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThe recent updates enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HiveMindListenerProtocol
participant UtteranceTransformersService
participant DialogTransformersService
participant MetadataTransformersService
Client->>HiveMindListenerProtocol: Send bus message
HiveMindListenerProtocol->>UtteranceTransformersService: Transform utterance
UtteranceTransformersService-->>HiveMindListenerProtocol: Transformed utterance
HiveMindListenerProtocol->>DialogTransformersService: Transform dialog
DialogTransformersService-->>HiveMindListenerProtocol: Transformed dialog
HiveMindListenerProtocol->>MetadataTransformersService: Transform metadata
MetadataTransformersService-->>HiveMindListenerProtocol: Transformed metadata
HiveMindListenerProtocol-->>Client: Processed message
Assessment against linked issues
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 as PR comments)
Additionally, you can add 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: 3
Outside diff range, codebase verification and nitpick comments (1)
hivemind_core/protocol.py (1)
Line range hint
261-504: Review the integration of transformer services inHiveMindListenerProtocol.The integration of
UtteranceTransformersService,MetadataTransformersService, andDialogTransformersServiceinto theHiveMindListenerProtocolis a significant enhancement. Consider the following:
- Error Handling: Ensure that errors in the transformer services do not halt the entire message handling process.
- Performance: The transformation process should be optimized to avoid delays in message processing.
- Security: Carefully manage the flow of sensitive data through the transformers to prevent leakage.
The implementation is robust but could benefit from additional error handling and performance optimizations.
- except: + except SpecificException as e: # Replace with more specific exceptions- utterances, message.context = self.utterance_plugins.transform(utterances, message.context) + try: + utterances, message.context = self.utterance_plugins.transform(utterances, message.context) + except TransformerException as e: + LOG.error(f"Error in utterance transformation: {e}") + # Handle error appropriately
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- hivemind_core/protocol.py (10 hunks)
- hivemind_core/transformers.py (1 hunks)
- requirements.txt (1 hunks)
Additional context used
Ruff
hivemind_core/transformers.py
41-41: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
132-135: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
134-134: Do not use bare
except(E722)
188-191: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
190-190: Do not use bare
except(E722)
Additional comments not posted (1)
requirements.txt (1)
5-6: Verify dependency versions for compatibility.The addition of
ovos-bus-clientandovos-plugin-manager, and the update ofHiveMind_presenceare crucial for the new functionality. It's important to ensure these versions are compatible with each other and the existing project dependencies.
| class DialogTransformersService: | ||
| """ transform dialogs before being sent to TTS """ | ||
|
|
||
| def __init__(self, bus, config=None): | ||
| self.loaded_plugins = {} | ||
| self.has_loaded = False | ||
| self.bus = bus | ||
| # to activate a plugin, just add an entry to mycroft.conf for it | ||
| self.config = config or Configuration().get("dialog_transformers", {}) | ||
| self.load_plugins() | ||
|
|
||
| @property | ||
| def blacklisted_skills(self): | ||
| # dialog should NEVER be rewritten if it comes from these skills | ||
| return self.config.get("blacklisted_skills", | ||
| ["skill-ovos-icanhazdadjokes.openvoiceos"] # blacklist jokes by default | ||
| ) | ||
|
|
||
| def load_plugins(self): | ||
| for plug_name, plug in find_dialog_transformer_plugins().items(): | ||
| if plug_name in self.config: | ||
| # if disabled skip it | ||
| if not self.config[plug_name].get("active", True): | ||
| continue | ||
| try: | ||
| self.loaded_plugins[plug_name] = plug(config=self.config[plug_name]) | ||
| self.loaded_plugins[plug_name].bind(self.bus) | ||
| LOG.info(f"loaded audio transformer plugin: {plug_name}") | ||
| except Exception as e: | ||
| LOG.exception(f"Failed to load dialog transformer plugin: " | ||
| f"{plug_name}") | ||
| self.has_loaded = True | ||
|
|
||
| @property | ||
| def plugins(self) -> list: | ||
| """ | ||
| Return loaded transformers in priority order, such that modules with a | ||
| higher `priority` rank are called first and changes from lower ranked | ||
| transformers are applied last. | ||
|
|
||
| A plugin of `priority` 1 will override any existing context keys and | ||
| will be the last to modify `audio_data` | ||
| """ | ||
| return sorted(self.loaded_plugins.values(), | ||
| key=lambda k: k.priority, reverse=True) | ||
|
|
||
| def shutdown(self): | ||
| """ | ||
| Shutdown all loaded plugins | ||
| """ | ||
| for module in self.plugins: | ||
| try: | ||
| module.shutdown() | ||
| except Exception as e: | ||
| LOG.warning(e) | ||
|
|
||
| def transform(self, dialog: str, context: dict = None, sess: Session = None) -> Tuple[str, dict]: | ||
| """ | ||
| Get transformed audio and context for the preceding audio | ||
| @param dialog: str to be spoken | ||
| @return: transformed dialog to be sent to TTS | ||
| """ | ||
|
|
||
| # TODO property not yet introduced in Session | ||
| sess = sess or SessionManager.get() | ||
| # if isinstance(sess, dict): | ||
| # sess = Session.deserialize(sess) | ||
| # active_transformers = sess.dialog_transformers or self.plugins | ||
|
|
||
| active_transformers = self.plugins | ||
|
|
||
| for module in active_transformers: | ||
| try: | ||
| LOG.debug(f"checking dialog transformer: {module}") | ||
| dialog, context = module.transform(dialog, context=context) | ||
| LOG.debug(f"{module.name}: {dialog}") | ||
| except Exception as e: | ||
| LOG.exception(e) | ||
| return dialog, context |
There was a problem hiding this comment.
Review the implementation of DialogTransformersService.
The class is well-structured with methods for loading plugins, transforming dialog, and shutting down. However, there are several points to consider:
- Error Handling: The exception handling in the
load_pluginsandtransformmethods could be more specific. Catching general exceptions is not a best practice. - Logging: The use of logging is appropriate, but ensure sensitive data is not logged.
- Performance: The sorting of plugins every time they are accessed might impact performance if the list is large. Consider caching the sorted list.
Overall, the implementation aligns with the objectives but could be refined to improve error handling and performance.
- except Exception as e:
+ except SpecificException as e: # Replace with more specific exceptions- return sorted(self.loaded_plugins.values(),
+ if not hasattr(self, '_sorted_plugins'):
+ self._sorted_plugins = sorted(self.loaded_plugins.values(),
+ key=lambda k: k.priority, reverse=True)
+ return self._sorted_pluginsTools
Ruff
41-41: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
| class UtteranceTransformersService: | ||
|
|
||
| def __init__(self, bus, config=None): | ||
| self.config_core = config or {} | ||
| self.loaded_plugins = {} | ||
| self.has_loaded = False | ||
| self.bus = bus | ||
| self.config = self.config_core.get("utterance_transformers") or {} | ||
| self.load_plugins() | ||
|
|
||
| def load_plugins(self): | ||
| for plug_name, plug in find_utterance_transformer_plugins().items(): | ||
| if plug_name in self.config: | ||
| # if disabled skip it | ||
| if not self.config[plug_name].get("active", True): | ||
| continue | ||
| try: | ||
| self.loaded_plugins[plug_name] = plug() | ||
| LOG.info(f"loaded utterance transformer plugin: {plug_name}") | ||
| except Exception as e: | ||
| LOG.error(e) | ||
| LOG.exception(f"Failed to load utterance transformer plugin: {plug_name}") | ||
|
|
||
| @property | ||
| def plugins(self): | ||
| """ | ||
| Return loaded transformers in priority order, such that modules with a | ||
| higher `priority` rank are called first and changes from lower ranked | ||
| transformers are applied last | ||
|
|
||
| A plugin of `priority` 1 will override any existing context keys and | ||
| will be the last to modify utterances` | ||
| """ | ||
| return sorted(self.loaded_plugins.values(), | ||
| key=lambda k: k.priority, reverse=True) | ||
|
|
||
| def shutdown(self): | ||
| for module in self.plugins: | ||
| try: | ||
| module.shutdown() | ||
| except: | ||
| pass | ||
|
|
||
| def transform(self, utterances: List[str], context: Optional[dict] = None): | ||
| context = context or {} | ||
|
|
||
| for module in self.plugins: | ||
| try: | ||
| utterances, data = module.transform(utterances, context) | ||
| _safe = {k: v for k, v in data.items() if k != "session"} # no leaking TTS/STT creds in logs | ||
| LOG.debug(f"{module.name}: {_safe}") | ||
| context = merge_dict(context, data) | ||
| except Exception as e: | ||
| LOG.warning(f"{module.name} transform exception: {e}") | ||
| return utterances, context |
There was a problem hiding this comment.
Review the implementation of UtteranceTransformersService.
This class follows a similar structure to DialogTransformersService. The comments for error handling, logging, and performance optimizations apply here as well. Additionally, consider the following:
- Data Handling: The transformation logic should ensure that no data integrity issues occur, especially when merging dictionaries.
- Security: Ensure that no sensitive data is leaked through logs or transformations.
Refinements in error handling and data security are recommended.
- except Exception as e:
+ except SpecificException as e: # Replace with more specific exceptions- _safe = {k: v for k, v in data.items() if k != "session"}
+ _safe = {k: v for k, v in data.items() if k not in ["session", "other_sensitive_key"]}Tools
Ruff
132-135: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
134-134: Do not use bare
except(E722)
| class MetadataTransformersService: | ||
|
|
||
| def __init__(self, bus, config=None): | ||
| self.config_core = config or {} | ||
| self.loaded_plugins = {} | ||
| self.has_loaded = False | ||
| self.bus = bus | ||
| self.config = self.config_core.get("metadata_transformers") or {} | ||
| self.load_plugins() | ||
|
|
||
| def load_plugins(self): | ||
| for plug_name, plug in find_metadata_transformer_plugins().items(): | ||
| if plug_name in self.config: | ||
| # if disabled skip it | ||
| if not self.config[plug_name].get("active", True): | ||
| continue | ||
| try: | ||
| self.loaded_plugins[plug_name] = plug() | ||
| LOG.info(f"loaded metadata transformer plugin: {plug_name}") | ||
| except Exception as e: | ||
| LOG.error(e) | ||
| LOG.exception(f"Failed to load metadata transformer plugin: {plug_name}") | ||
|
|
||
| @property | ||
| def plugins(self): | ||
| """ | ||
| Return loaded transformers in priority order, such that modules with a | ||
| higher `priority` rank are called first and changes from lower ranked | ||
| transformers are applied last. | ||
|
|
||
| A plugin of `priority` 1 will override any existing context keys | ||
| """ | ||
| return sorted(self.loaded_plugins.values(), | ||
| key=lambda k: k.priority, reverse=True) | ||
|
|
||
| def shutdown(self): | ||
| for module in self.plugins: | ||
| try: | ||
| module.shutdown() | ||
| except: | ||
| pass | ||
|
|
||
| def transform(self, context: Optional[dict] = None): | ||
| context = context or {} | ||
|
|
||
| for module in self.plugins: | ||
| try: | ||
| data = module.transform(context) | ||
| _safe = {k: v for k, v in data.items() if k != "session"} # no leaking TTS/STT creds in logs | ||
| LOG.debug(f"{module.name}: {_safe}") | ||
| context = merge_dict(context, data) | ||
| except Exception as e: | ||
| LOG.warning(f"{module.name} transform exception: {e}") | ||
| return context |
There was a problem hiding this comment.
Review the implementation of MetadataTransformersService.
This class handles metadata transformations. The same recommendations for error handling, logging, and data handling apply. Additionally:
- Modularity: Consider abstracting some of the repeated logic across the transformer services into a base class or utility functions to reduce code duplication and improve maintainability.
Propose a refactor to improve modularity and reduce code duplication.
+class BaseTransformerService:
+ def load_plugins(self, plugin_finder):
+ for plug_name, plug in plugin_finder().items():
+ if plug_name in self.config:
+ if not self.config[plug_name].get("active", True):
+ continue
+ try:
+ self.loaded_plugins[plug_name] = plug()
+ LOG.info(f"loaded transformer plugin: {plug_name}")
+ except Exception as e:
+ LOG.error(e)
+ LOG.exception(f"Failed to load transformer plugin: {plug_name}")
+
- def load_plugins(self):
- for plug_name, plug in find_metadata_transformer_plugins().items():
- ...Committable suggestion was skipped due to low confidence.
Tools
Ruff
188-191: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
190-190: Do not use bare
except(E722)
closes #82
in the regular mycroft.conf add the transformers configs as usual, but this time under "hivemind" section (config file may be shared with OVOS with different plugins in each)
relevant plugins:
this PR brings transformer functionality directly to hivemind, allowing plugins to also be used with non ovos-core minds
Summary by CodeRabbit
New Features
Dependencies
ovos-bus-client(v0.0.6),ovos-plugin-manager, andHiveMind_presence(v0.0.2a3).