feat: chat history #286
feat: chat history #286
Conversation
WalkthroughThe pull request introduces a new solver type, Changes
Possibly related PRs
Suggested labels
Poem
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 24 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)ovos_plugin_manager/templates/solvers.py (5)
The class definition is clear and the docstring provides an excellent example of the expected message format.
The abstract method is properly defined with comprehensive documentation of parameters and return type.
The method correctly implements language detection and translation through decorators while maintaining a clean delegation to
The override properly maintains API compatibility while clearly documenting the conversion process from single query to chat format.
Fix Liskov Substitution Principle violation. The method signature differs from the base class's Since this issue was previously identified and the fix suggestion is still valid, I'm marking this as a duplicate of the previous review comment. Please refer to the earlier suggestion for the recommended fixes:
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: 1
🧹 Outside diff range and nitpick comments (3)
ovos_plugin_manager/utils/__init__.py (1)
31-61: Consider tracking the planned plugin type renaming systematicallyThe multiple TODO comments indicate a significant planned migration to rename plugin types with an "opm.*" prefix. This kind of widespread change requires careful planning to avoid breaking changes.
Consider:
- Creating a tracking issue for this migration
- Documenting the migration plan including:
- Timeline for deprecation
- Communication strategy for downstream dependencies
- Migration guide for plugin developers
- Implementing a deprecation mechanism before the rename
Would you like me to help create a GitHub issue to track this migration plan?
ovos_plugin_manager/templates/solvers.py (2)
402-409: Improve docstring formatting forChatMessageSolverThe docstring for the
ChatMessageSolverclass can be enhanced for clarity and adherence to PEP 257 conventions. Consider reformatting the example to use proper indentation and provide a more detailed description of the class's purpose.Apply this diff to adjust the docstring:
- """take chat history as input LLM style - messages = [ - {"role": "system", "content": "You are a helpful assistant."}, - {"role": "user", "content": "Knock knock."}, - {"role": "assistant", "content": "Who's there?"}, - {"role": "user", "content": "Orange."}, - ] - """ + """ + Take chat history as input in LLM style. + + Example: + messages = [ + {"role": "system", "content": "You are a helpful assistant."}, + {"role": "user", "content": "Knock knock."}, + {"role": "assistant", "content": "Who's there?"}, + {"role": "user", "content": "Orange."}, + ] + """
411-413: Set default value forlangparameter incontinue_chatmethodFor consistency with other methods in the class and to handle cases where
langmight not be provided, consider setting a default value ofNonefor thelangparameter in thecontinue_chatmethod.Apply this diff:
- def continue_chat(self, messages: List[Dict[str, str]], - lang: Optional[str]) -> Optional[str]: + def continue_chat(self, messages: List[Dict[str, str]], + lang: Optional[str] = None) -> Optional[str]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ovos_plugin_manager/solvers.py(3 hunks)ovos_plugin_manager/templates/solvers.py(1 hunks)ovos_plugin_manager/utils/__init__.py(3 hunks)
🔇 Additional comments (7)
ovos_plugin_manager/utils/__init__.py (2)
Line range hint 178-182: LGTM! Well-structured deprecation notice
The deprecation of normalize_lang is handled appropriately with:
- Clear migration instructions to the new function
- Version targeting for removal
- Maintained functionality during transition
51-51: Verify documentation for the new chat solver plugin type
The addition of CHAT_SOLVER looks good and follows the established pattern. However, we should ensure it's properly documented.
Let's check for documentation:
Also applies to: 95-95
ovos_plugin_manager/solvers.py (3)
1-4: LGTM: Import changes follow existing patterns
The addition of ChatMessageSolver to the imports maintains consistency with the existing codebase structure.
191-191: LGTM: Minor formatting improvements
The indentation adjustments in function signatures improve readability while maintaining functionality.
Also applies to: 322-322
11-12: Verify PluginTypes.CHAT_SOLVER enum value exists
The new functions rely on PluginTypes.CHAT_SOLVER. Let's verify this enum value is properly defined.
Also applies to: 21-22
✅ Verification successful
PluginTypes.CHAT_SOLVER is properly defined and used consistently
The verification confirms that:
CHAT_SOLVERis defined inPluginTypeswith value"opm.solver.chat"- It's correctly used in
solvers.pyfor both finding and loading plugins
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CHAT_SOLVER is defined in PluginTypes
# Search for CHAT_SOLVER definition in PluginTypes
rg -l "class PluginTypes" | xargs rg "CHAT_SOLVER"
# Also check if any existing plugins are using this type
fd -e py | xargs rg "PluginTypes\.CHAT_SOLVER"
Length of output: 350
ovos_plugin_manager/templates/solvers.py (2)
423-424: Confirm overridden get_spoken_answer behavior
The get_spoken_answer method is overridden to funnel the query into continue_chat by wrapping it in a chat message. This approach is consistent with the intended design of ChatMessageSolver.
415-420: Verify compatibility of decorators with messages parameter
The @auto_detect_lang and @auto_translate decorators are applied to get_chat_completion with text_keys=["messages"] and translate_keys=["messages"]. Since messages is a list of dictionaries, ensure that these decorators can handle this data structure appropriately.
Run the following script to check if the decorators support lists of dictionaries:
✅ Verification successful
Let me gather more context about how these decorators handle the data structures.
Let me check how the messages parameter is handled in the actual implementation.
Let me check the translation handling in the solver base class.
Let me check the translation handling in the language module since it's imported.
Decorators correctly handle list of dictionaries parameter
The @auto_detect_lang and @auto_translate decorators are compatible with the messages parameter. The translation system includes recursive handling of nested structures through translate_dict and translate_list methods, which can process the chat messages format:
messages = [
{"role": "system", "content": "You are a helpful assistant."},
{"role": "user", "content": "Knock knock."}
]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if `auto_detect_lang` and `auto_translate` can process `messages` as a list of dictionaries.
# Search for implementations of `auto_detect_lang` and `auto_translate` to analyze their handling of complex data types
rg -A 10 'def auto_detect_lang' ovos_plugin_manager/templates/solvers.py
rg -A 10 'def auto_translate' ovos_plugin_manager/templates/solvers.py
Length of output: 1010
Script:
#!/bin/bash
# Get more context around the decorator implementations
rg -B 5 -A 30 'def auto_detect_lang' ovos_plugin_manager/templates/solvers.py
rg -B 5 -A 30 'def auto_translate' ovos_plugin_manager/templates/solvers.py
# Also search for any usage examples of these decorators with list/dict parameters
ast-grep --pattern 'auto_detect_lang(text_keys=$_)'
ast-grep --pattern 'auto_translate(translate_keys=$_)'
Length of output: 5876
Script:
#!/bin/bash
# Get the implementation details of how messages are processed
rg -B 10 -A 20 'def continue_chat' ovos_plugin_manager/templates/solvers.py
# Also check for any recursive handling of dictionaries in the translation logic
rg -A 20 'def translate' ovos_plugin_manager/templates/solvers.py
Length of output: 1400
Script:
#!/bin/bash
# Search for translation handling in the AbstractSolver class
rg -B 5 -A 20 'class AbstractSolver' ovos_plugin_manager/templates/solvers.py
# Check for any dictionary/list handling in translation methods
rg -B 5 -A 20 'def translate.*dict' ovos_plugin_manager/templates/solvers.py
rg -B 5 -A 20 'def translate.*list' ovos_plugin_manager/templates/solvers.py
Length of output: 237
Script:
#!/bin/bash
# Check the language module for translation handling
rg -B 5 -A 20 'class LanguageTranslator' ovos_plugin_manager/templates/language.py
# Look for any recursive translation methods that might handle nested structures
rg -B 5 -A 20 'def translate' ovos_plugin_manager/templates/language.py
Length of output: 3672
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
ovos_plugin_manager/templates/solvers.py (2)
401-410: Enhance class docstring with more detailsThe docstring could be more comprehensive by including:
- Purpose and use cases of the class
- Description of the message format requirements
- Return type expectations
class ChatMessageSolver(QuestionSolver): - """take chat history as input LLM style + """A solver that processes chat history in LLM-style format to generate contextual responses. + + This class extends QuestionSolver to handle multi-turn conversations by maintaining + chat history in a format compatible with Large Language Models (LLMs). + + The chat messages should be provided as a list of dictionaries, where each message + contains a 'role' (system/user/assistant) and 'content' (the message text). + messages = [ {"role": "system", "content": "You are a helpful assistant."}, {"role": "user", "content": "Knock knock."}, {"role": "assistant", "content": "Who's there?"}, {"role": "user", "content": "Orange."}, ] + + Returns: + str: The generated response based on the chat history. """
411-416: Add docstring and proper type hints to abstract methodThe abstract method lacks documentation and proper type hints.
@abc.abstractmethod def continue_chat(self, messages: List[Dict[str, str]], lang: Optional[str], - units: Optional[str] = None) -> Optional[str]: + units: Optional[str] = None) -> str: + """Process the chat history and generate the next response. + + Args: + messages (List[Dict[str, str]]): List of message dictionaries containing + 'role' and 'content' keys. + lang (Optional[str]): The language code for the response. + units (Optional[str]): Optional unit system for numerical values. + + Returns: + str: The generated response based on the chat history. + + Raises: + NotImplementedError: When the method is not implemented by a concrete class. + """ pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_plugin_manager/templates/solvers.py(2 hunks)
🔇 Additional comments (2)
ovos_plugin_manager/templates/solvers.py (2)
417-423: LGTM! Well-structured implementation with proper language handling
The implementation correctly uses decorators for language detection and translation, maintaining consistency with the rest of the codebase.
443-448: LGTM! Good implementation maintaining API compatibility
The method correctly implements the base class interface by converting a single query into the chat message format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ovos_plugin_manager/templates/solvers.py (4)
401-410: Enhance class docstring documentationWhile the example is helpful, the docstring could be more comprehensive by including:
- Purpose and use cases of the class
- Description of the chat message format requirements
- Example of the expected return value
class ChatMessageSolver(QuestionSolver): - """take chat history as input LLM style + """A solver that processes chat history in LLM-style format to generate contextual responses. + + This class extends QuestionSolver to handle multi-turn conversations, maintaining + context across messages. It expects chat messages in a format similar to LLM APIs: + messages = [ {"role": "system", "content": "You are a helpful assistant."}, {"role": "user", "content": "Knock knock."}, {"role": "assistant", "content": "Who's there?"}, {"role": "user", "content": "Orange."}, ] + + Returns: + A string containing the assistant's response based on the chat history. """
411-416: Add docstring to abstract methodThe abstract method should include a comprehensive docstring to guide implementers.
@abc.abstractmethod def continue_chat(self, messages: List[Dict[str, str]], lang: Optional[str], units: Optional[str] = None) -> Optional[str]: + """Generate a response based on the chat history. + + Args: + messages (List[Dict[str, str]]): List of chat messages, each containing 'role' and 'content'. + lang (Optional[str]): The language code for the response. If None, will be auto-detected. + units (Optional[str]): Optional unit system for numerical values. + + Returns: + Optional[str]: The generated response or None if no response could be generated. + """ pass
424-441: Consider adding language handling decoratorsThe method should handle language detection and translation consistently with
get_chat_completion.+ @auto_detect_lang(text_keys=["messages"]) + @auto_translate(translate_keys=["messages"]) def stream_chat_utterances(self, messages: List[Dict[str, str]], lang: Optional[str] = None, units: Optional[str] = None) -> Iterable[str]:
442-447: Improve method documentationThe comment could better explain the purpose and implementation details of this override.
def get_spoken_answer(self, query: str, lang: Optional[str] = None, units: Optional[str] = None) -> Optional[str]: - # just for api compat since it's a subclass, shouldn't be directly used + """Override of QuestionSolver.get_spoken_answer for API compatibility. + + This implementation converts the single query into a chat message format + and delegates to continue_chat. While functional, direct use of chat-specific + methods is recommended for chat-based interactions. + """ return self.continue_chat(messages=[{"role": "user", "content": query}], lang=lang, units=units)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_plugin_manager/templates/solvers.py(2 hunks)
🔇 Additional comments (1)
ovos_plugin_manager/templates/solvers.py (1)
417-423: LGTM! Well-structured implementation.
The method correctly implements language detection and translation using decorators, maintaining consistency with the parent class pattern.
for usage with https://github.com/OpenVoiceOS/ovos-persona
Summary by CodeRabbit
New Features
ChatMessageSolverclass for enhanced chat interactions.Bug Fixes
Documentation
Refactor