Exercise Chat: Implement native function calling agent
#154
Conversation
…e-chat/submission-failure-event
WalkthroughThe changes involve updates to multiple files, enhancing message handling and pipeline functionality. Key modifications include the addition of new message types and subclasses, adjustments to method signatures for tool binding, and the introduction of new parameters in pipeline execution methods. The Changes
Possibly related PRs
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: 11
Outside diff range, codebase verification and nitpick comments (3)
app/domain/data/programming_exercise_dto.py (1)
34-38:recent_changesfield lacks integration.The
recent_changesfield is defined inprogramming_exercise_dto.pybut is not currently integrated with version control or change tracking systems. Consider implementing functionality to utilize this field effectively for tracking Git diffs and changes.
- Location:
app/domain/data/programming_exercise_dto.pyAnalysis chain
Addition of
recent_changesenhances transparency.The new
recent_changesfield, which captures the Git diff of recent changes, is a valuable addition for tracking updates. Ensure that this field is properly integrated with version control and change tracking systems.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `recent_changes` with version control systems. # Test: Search for handling of `recent_changes`. Expect: Proper integration with version control systems. rg --type python -A 5 $'recent_changes'Length of output: 69
Script:
#!/bin/bash # Description: Verify integration of `recent_changes` with version control systems. # Test: Search for handling of `recent_changes`. Expect: Proper integration with version control systems. rg --type py -A 5 $'recent_changes'Length of output: 410
Script:
#!/bin/bash # Description: Search for references to `recent_changes` across the codebase to verify its integration. # Test: Find all occurrences of `recent_changes` to check for integration with version control systems. rg 'recent_changes'Length of output: 106
app/domain/data/exercise_with_submissions_dto.py (1)
37-37: Field addition approved; consider reviewing the alias usage.The addition of the
urlfield with a default value ofNoneis well-implemented and ensures backward compatibility. However, the use of an alias that directly matches the field name might be unnecessary unless there are specific serialization requirements that necessitate this approach.app/common/message_converters.py (1)
Line range hint
28-47: Refactor suggestion for handlingPyrisAIMessageandAIMessageconversions.The function
convert_iris_message_to_langchain_messagehas been updated to handlePyrisAIMessagespecifically, which includes processing tool calls. This is a significant enhancement for supporting interactive tool functionalities. However, the current implementation could be improved for clarity and maintainability:
- Extract Tool Call Conversion: The conversion logic for tool calls (lines 39-46) could be extracted into a separate function. This would make the
convert_iris_message_to_langchain_messagefunction cleaner and more focused on its primary responsibility.- Error Handling: Consider adding more specific error messages or custom exceptions for better debugging and user feedback.
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (9)
app/domain/chat/exercise_chat/exercise_chat_pipeline_execution_dto.py (1)
16-16: Well-implemented attribute addition.The
event_payloadattribute is correctly implemented with an alias for serialization purposes. Consider adding a comment explaining the purpose of this attribute for future maintainability.app/domain/pyris_message.py (2)
31-34: Well-designed subclass for AI messages.
PyrisAIMessageis appropriately designed for AI-generated messages. Consider adding documentation for thetool_callsattribute to clarify its usage.
37-40: Clear and functional design for tool messages.
PyrisToolMessageis well-designed for messages from tools. Adding documentation for thecontentsattribute could enhance clarity and maintainability.app/domain/data/programming_exercise_dto.py (1)
33-38: Useful additions to the DTO.The new fields
max_pointsandrecent_changesare well-implemented and enhance the functionality of theProgrammingExerciseDTO. Consider adding validation for themax_pointsfield to ensure it remains within a sensible range.app/llm/request_handler/capability_request_handler.py (1)
74-81: Well-implemented method for tool binding.The
bind_toolsmethod is correctly implemented and integrates well with the existing class structure. The use of type annotations is commendable as it enhances code readability and maintainability.Consider adding error handling or logging within the
bind_toolsmethod to manage potential issues during the tool binding process, such as when the selected model does not support tool binding or when an invalid tool type is passed.app/llm/external/model.py (1)
53-63: New abstract methodbind_toolsadded.The addition of the
bind_toolsmethod is a positive enhancement, increasing the extensibility of theLanguageModelclass. It allows for more complex interactions with various tools.Consider providing more detailed documentation or examples on how to implement this method in subclasses, especially given the variety of tool types that can be bound.
app/web/routers/pipelines.py (1)
28-37: Enhanced flexibility inrun_exercise_chat_pipeline_worker.The addition of the
variantparameter and the use ofExerciseChatAgentPipelineenhance the flexibility and adaptability of the pipeline. This change allows for variant-specific processing, which could lead to improved performance in handling different scenarios.Consider adding more detailed logging or error handling specific to the
variantparameter to ensure that any issues related to variant-specific configurations are captured and addressed promptly.app/llm/external/ollama.py (2)
Line range hint
83-135: Approve existing methods and suggest documentation forbind_tools.The existing methods in the
OllamaModelclass are correctly implemented. However, thebind_toolsmethod is marked as not implemented. It's important to document its intended future functionality to guide development and integration.Would you like help drafting the documentation for the
bind_toolsmethod?
Potential Issue with
convert_to_iris_messageFunctionThe
convert_to_iris_messagefunction inapp/llm/external/ollama.pycorrectly maps theroleandcontentfields from aMessageto aPyrisMessage. However, it usesdatetime.now()for thesend_atfield, which may not preserve the original timestamp from theMessage. Additionally, other fields in theMessageare not handled, which could affect data integrity if those fields are important.
- Consider preserving the original timestamp if it is significant.
- Review if additional fields from the
Messageshould be included in thePyrisMessage.Analysis chain
Line range hint
43-66: Verify correct mapping and data integrity inconvert_to_iris_message.Ensure that the
convert_to_iris_messagefunction correctly maps all fields and maintains data integrity during the conversion.Run the following script to verify the correct mapping and data integrity:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct mapping and data integrity in `convert_to_iris_message`. # Test: Search for the function usage. Expect: Correct mapping and data integrity. rg --type python -A 5 $'convert_to_iris_message'Length of output: 78
Script:
#!/bin/bash # Description: Verify the correct mapping and data integrity in `convert_to_iris_message`. # Test: Search for the function definition and usage. Expect: Correct mapping and data integrity. rg --type py -A 5 $'def convert_to_iris_message'Length of output: 827
Script:
#!/bin/bash # Description: Extract the complete implementation of `convert_to_iris_message` from `app/llm/external/ollama.py`. # Extract the function implementation to verify correct mapping and data integrity. ast-grep --lang python --pattern $'def convert_to_iris_message($_) {\n $$$\n}' app/llm/external/ollama.pyLength of output: 569
Script:
#!/bin/bash # Description: Extract the complete implementation of `convert_to_iris_message` from `app/llm/external/ollama.py`. # Extract the function implementation to verify correct mapping and data integrity. rg --type py -A 20 $'def convert_to_iris_message' app/llm/external/ollama.pyLength of output: 545
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range, codebase verification and nitpick comments (8)
app/domain/data/exercise_with_submissions_dto.py (1)
37-37: Field definition forurlis correct but consider simplifying the alias.The implementation of the
urlfield inExerciseWithSubmissionsDTOis correct. However, the alias "url" is redundant since it matches the field name. Consider removing the alias for simplicity unless there is a specific reason for its inclusion.app/llm/llm_manager.py (1)
42-42: Good shift towards validation inload_llmsmethod.The change to use
model_validateinstead ofparse_objin theload_llmsmethod is a positive step towards ensuring data integrity. Consider adding error handling or logging to monitor the impact of this change on the system.app/llm/request_handler/capability_request_handler.py (1)
74-81: Lack of Validation fortoolsParameter inbind_toolsMethodThe
bind_toolsmethod incapability_request_handler.pydoes not perform any validation or security checks on thetoolsparameter before binding them to the language model. This could lead to potential security vulnerabilities. It is recommended to implement validation checks to ensure that only valid and safe tools are bound to the model.
- File:
app/llm/request_handler/capability_request_handler.py- Lines: 74-81
Analysis chain
Review the
bind_toolsmethod implementation.The implementation of
bind_toolsappears correct and aligns with the PR objectives. However, consider adding security checks or validations for thetoolsparameter to ensure that only valid tools are bound to the model.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify security checks for the `tools` parameter. # Test: Search for security checks related to `tools`. Expect: Security checks or validations. rg --type python -A 5 $'tools: Sequence[Union[Dict[str, Any], Type[BaseModel], Callable, BaseTool]]'Length of output: 132
Script:
#!/bin/bash # Description: Search for occurrences of `tools` and any associated validation or security checks in the codebase. # Search for occurrences of `tools` in the codebase to identify any validation or security checks. rg 'tools' -A 5Length of output: 27543
app/llm/external/model.py (1)
47-47: Potential issues with return type change inchatmethod.The return type of the
chatmethod inapp/llm/external/model.pyhas been changed toChatCompletionMessage, but other implementations and usages still expectPyrisMessage. This discrepancy could lead to compatibility issues. Ensure that all usages of thechatmethod are updated to handle the new return type correctly.
- Files to check:
app/pipeline/lecture_ingestion_pipeline.pyapp/pipeline/competency_extraction_pipeline.pyapp/llm/external/openai_chat.pyapp/llm/external/ollama.py- Various request handler files
Analysis chain
Verify the impact of the return type change in the
chatmethod.The change in return type from
PyrisMessagetoChatCompletionMessagecould affect downstream processing. Verify that all existing usages of thechatmethod have been updated to handle the new return type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the return type change on existing code. # Test: Search for usages of the `chat` method. Expect: Updated handling for `ChatCompletionMessage`. rg --type python -A 5 $'chat\('Length of output: 63
Script:
#!/bin/bash # Description: Verify the impact of the return type change on existing code. # Test: Search for usages of the `chat` method. Expect: Updated handling for `ChatCompletionMessage`. rg --type py -A 5 $'chat\('Length of output: 5840
app/llm/external/ollama.py (1)
Line range hint
83-135: Review: New methodbind_toolsand class structure.The class
OllamaModelis well-structured and inherits appropriately from related classes. The new methodbind_toolsis designed to raise aNotImplementedError, indicating it's a placeholder for future functionality. This approach is acceptable as it clearly communicates the current limitations and future intentions.However, consider adding more detailed documentation to the
bind_toolsmethod to explain its future purpose and expected integration.Would you like help drafting the additional documentation for this method?
app/pipeline/chat/interaction_suggestion_pipeline.py (1)
Line range hint
115-145: Approved: Streamlined prompt handling and improved error handling.The modifications to the
__call__method in theInteractionSuggestionPipelineclass simplify the handling of chat history and prompts. The removal of specific prompts related to chat history reduces redundancy and potential confusion. The addition of aValueErrorwhen no last message is provided enhances the robustness of the method by ensuring necessary data is present.Consider adding unit tests to cover these changes, especially the new error handling paths, to ensure they work as expected under various scenarios.
Would you like assistance in creating these unit tests?
app/common/message_converters.py (1)
Line range hint
22-47: Approved: Expanded message conversion functionality.The function
convert_iris_message_to_langchain_messagehas been effectively expanded to handle new message types such asToolMessageandPyrisToolMessage. The robust error handling for empty messages and type mismatches ensures reliable operation.Consider adding detailed documentation to describe the handling of these new message types, which will aid in maintenance and future enhancements.
Would you like help with drafting this documentation?
app/pipeline/chat/course_chat_pipeline.py (1)
Line range hint
585-591: Suggest refining exception handling.The broad exception handling in the
__call__method could be refined to catch specific exceptions. This would allow for more targeted error responses and recovery actions, improving the robustness and maintainability of the code.Consider handling specific exceptions like
ValueErrororTypeErrorseparately to provide more informative error messages and recovery options.
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (16)
app/common/pyris_message.py (2)
17-17: Consider adding docstring for the new TOOL role.While the addition of the TOOL role is appropriate, adding documentation would help clarify its purpose and usage in the context of native function calling.
- TOOL = "TOOL" + TOOL = "TOOL" # Represents messages from tool/function executions in native function calling
34-37: Add class-level documentation for PyrisAIMessage.The implementation looks good, but adding a docstring would help explain its role in the tool calling system.
class PyrisAIMessage(PyrisMessage): + """ + Represents an AI assistant message that may include tool/function calls. + Used in the context of native function calling to request tool executions. + """ model_config = ConfigDict(populate_by_name=True)app/llm/request_handler/request_handler_interface.py (1)
46-52: Consider the implications of making tool binding mandatory.Making
bind_toolsan abstract method requires all request handlers to implement tool binding, which might not be necessary for simpler handlers. Consider:
- Moving tool binding to a separate interface (e.g.,
ToolBindingCapable)- Or making it an optional capability through a default implementation that returns self
This would provide more flexibility in implementing request handlers that don't need tool binding capabilities.
app/llm/request_handler/capability_request_handler.py (1)
77-84: Consider implementing a broader tool management interfaceThe current implementation only supports binding tools, but a more comprehensive tool management interface might be needed for:
- Unbinding tools
- Listing bound tools
- Checking tool compatibility
- Tool validation and verification
Consider creating a dedicated tool management interface that this class can implement.
app/pipeline/chat/interaction_suggestion_pipeline.py (2)
119-122: Consider making the chat history limit configurable.The hardcoded limit of 4 messages could be made configurable to allow adjustment based on token limits or specific use cases.
+ MAX_HISTORY_MESSAGES = 4 # Class constant for easy configuration chat_history_messages = [ convert_iris_message_to_langchain_message(message) - for message in history[-4:] + for message in history[-self.MAX_HISTORY_MESSAGES:] ]
141-143: Consider validating the problem statement format.While a default value is provided for missing problem statements, there's no validation of the format or content when it exists.
- prob_st_val = dto.problem_statement or "No problem statement provided." + def validate_problem_statement(ps: Optional[str]) -> str: + if not ps: + return "No problem statement provided." + if len(ps.strip()) == 0: + return "No problem statement provided." + return ps + + prob_st_val = validate_problem_statement(dto.problem_statement)app/web/routers/pipelines.py (1)
34-45: Document the new parameters for ExerciseChatAgentPipelineThe implementation looks good, but please add docstrings to explain:
- The purpose and expected values for the
variantparameter- The significance and possible values for the optional
eventparameterExample docstring:
def run_exercise_chat_pipeline( variant: str, event: str | None = Query(None, description="Event query parameter"), dto: ExerciseChatPipelineExecutionDTO = Body( description="Exercise Chat Pipeline Execution DTO" ), ): """Execute the exercise chat pipeline with the specified variant and event. Args: variant: The pipeline variant to use (e.g., "default", "experimental") event: Optional event identifier to customize pipeline behavior dto: The pipeline execution configuration """Also applies to: 65-74
app/pipeline/prompts/iris_course_chat_prompts.py (1)
83-85: Add examples to clarify the expected format.The formatting requirements for resource links are clear, but adding examples would help ensure consistent implementation. Consider adding examples like:
If you link a resource, DO NOT FORGET to include a markdown link. Use markdown format: [Resource title](Resource URL). The resource title should be the title of the lecture, exercise, or any other course material and shoud be descriptive in case no title is provided. Do not use "here" as a link text -The resource URL should only be the relative path to the course website, not the full URL. +The resource URL should only be the relative path to the course website, not the full URL. For example: +- Good: [Introduction to Python](/lectures/intro-python) +- Bad: [click here](/lectures/intro-python) +- Bad: [Introduction to Python](https://example.com/lectures/intro-python)app/pipeline/chat/exercise_chat_pipeline.py (3)
Line range hint
60-75: Add type validation for variant parameter.Consider validating the
variantparameter against a predefined set of allowed values to prevent runtime errors from invalid variants.+from enum import Enum, auto + +class ChatVariant(Enum): + DEFAULT = auto() + PROGRESS_STALLED = auto() + BUILD_FAILED = auto() + class ExerciseChatPipeline(Pipeline): """Exercise chat pipeline that answers exercises related questions from students.""" llm: IrisLangchainChatModel pipeline: Runnable callback: ExerciseChatStatusCallback suggestion_pipeline: InteractionSuggestionPipeline code_feedback_pipeline: CodeFeedbackPipeline prompt: ChatPromptTemplate - variant: str + variant: ChatVariant - def __init__(self, callback: ExerciseChatStatusCallback, variant: str = "default"): + def __init__(self, callback: ExerciseChatStatusCallback, variant: str = "default"): super().__init__(implementation_id="exercise_chat_pipeline") + try: + self.variant = ChatVariant[variant.upper()] + except KeyError: + raise ValueError(f"Invalid variant: {variant}. Must be one of {[v.name.lower() for v in ChatVariant]}")
171-173: Improve clarity of query extraction logic.The current implementation could be more explicit about extracting the last message from chat history.
-query: PyrisMessage | None = None -if history: - query = dto.chat_history[-1] +query: PyrisMessage | None = dto.chat_history[-1] if history else None
354-362: Add documentation for chat history handling logic.The new chat history handling logic would benefit from docstring updates to explain the different scenarios and their implications.
def _add_conversation_to_prompt( self, chat_history: List[PyrisMessage], user_question: PyrisMessage, ): """ Adds the chat history and user question to the prompt - :param chat_history: The chat history - :param user_question: The user question - :return: The prompt with the chat history + :param chat_history: The chat history. If empty, a special no-history prompt is used + :param user_question: The user question. If None, no user question is added to the prompt + :return: None + + The method handles three scenarios: + 1. With chat history: Adds the last 4 messages from history + 2. Without chat history: Adds a special system prompt + 3. With user question: Adds it as the latest input to consider """app/llm/external/openai_chat.py (4)
20-20: Remove unused importResponseFormat.The import
ResponseFormatat line 20 is unused in the code and can be safely removed to clean up the imports.Apply this diff to remove the unused import:
-from openai.types.chat.completion_create_params import ResponseFormat🧰 Tools
🪛 Ruff
20-20:
openai.types.chat.completion_create_params.ResponseFormatimported but unusedRemove unused import:
openai.types.chat.completion_create_params.ResponseFormat(F401)
47-58: Handle unanticipated content types explicitly inconvert_to_open_ai_messages.In the
matchstatement for messages wheremessage.sender == "TOOL", the default case uses apassstatement. This could lead to silent failures if unexpected content types are encountered. It's advisable to handle unanticipated content types explicitly, possibly by raising an exception or logging a warning.Apply this diff to handle unexpected content types:
case _: - pass + logging.warning(f"Unhandled content type: {type(content)} in TOOL message.")
85-100: Refactor to reduce duplication in message construction.The construction of
openai_messagein both branches shares common keys"role"and"content". Consider initializingopenai_messagewith the common fields and conditionally adding the"tool_calls"key if it exists to reduce code duplication.Apply this diff to refactor the code:
openai_message = { "role": map_role_to_str(message.sender), "content": openai_content, } - if isinstance(message, PyrisAIMessage) and message.tool_calls: + if getattr(message, "tool_calls", None): openai_message["tool_calls"] = [ { "id": tool.id, "type": tool.type, "function": { "name": tool.function.name, "arguments": json.dumps(tool.function.arguments), }, } for tool in message.tool_calls ]
174-206: Simplify API call logic to reduce duplication.The
chatmethod has duplicated code for making API calls to OpenAI, differing only in the presence oftoolsandresponse_format. Consider refactoring to construct thekwargsdynamically based on conditions.Apply this diff as a starting point to refactor the API calls:
for attempt in range(retries): try: kwargs = { "model": self.model, "messages": messages, "temperature": arguments.temperature, "max_tokens": arguments.max_tokens, } if self.tools: kwargs["tools"] = self.tools if arguments.response_format == "JSON": - if self.tools: - response = self._client.chat.completions.create( - response_format=ResponseFormatJSONObject(type="json_object"), - **kwargs, - ) - else: - response = self._client.chat.completions.create( - response_format=ResponseFormatJSONObject(type="json_object"), - **kwargs, - ) + kwargs["response_format"] = ResponseFormatJSONObject(type="json_object") response = self._client.chat.completions.create(**kwargs) choice = response.choices[0] usage = response.usage model = response.model # Existing error handling...Ensure that after refactoring, all necessary parameters are included, and the logic remains correct.
🧰 Tools
🪛 Ruff
180-180: Undefined name
ResponseFormatJSONObject(F821)
189-189: Undefined name
ResponseFormatJSONObject(F821)
app/pipeline/chat/exercise_chat_agent_pipeline.py (1)
430-607: Catch specific exceptions instead of using a broadexcept ExceptionThroughout the
__call__method, general exceptions are caught usingexcept Exception as e:. Catching all exceptions can make debugging difficult and may suppress important error information. It's advisable to catch specific exceptions to improve error handling and code robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
.gitignore(1 hunks)app/common/pyris_message.py(2 hunks)app/llm/external/model.py(2 hunks)app/llm/external/ollama.py(3 hunks)app/llm/external/openai_chat.py(8 hunks)app/llm/langchain/iris_langchain_chat_model.py(2 hunks)app/llm/request_handler/basic_request_handler.py(2 hunks)app/llm/request_handler/capability_request_handler.py(2 hunks)app/llm/request_handler/request_handler_interface.py(2 hunks)app/pipeline/chat/course_chat_pipeline.py(7 hunks)app/pipeline/chat/exercise_chat_agent_pipeline.py(1 hunks)app/pipeline/chat/exercise_chat_pipeline.py(6 hunks)app/pipeline/chat/interaction_suggestion_pipeline.py(2 hunks)app/pipeline/prompts/iris_course_chat_prompts.py(1 hunks)app/web/routers/pipelines.py(4 hunks)app/web/status/status_update.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Ruff
app/llm/external/openai_chat.py
20-20: openai.types.chat.completion_create_params.ResponseFormat imported but unused
Remove unused import: openai.types.chat.completion_create_params.ResponseFormat
(F401)
180-180: Undefined name ResponseFormatJSONObject
(F821)
189-189: Undefined name ResponseFormatJSONObject
(F821)
app/web/routers/pipelines.py
68-70: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
108-110: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (24)
app/common/pyris_message.py (3)
3-3: LGTM: Import changes are appropriate.
The new imports support the tool-related functionality and follow proper import conventions.
Also applies to: 9-10
40-43: Add documentation and verify tool message handling.
The implementation is correct, but would benefit from documentation explaining its purpose in the tool calling workflow.
Let's verify the tool message handling implementation:
#!/bin/bash
# Check for ToolMessageContentDTO usage and implementation
ast-grep --pattern 'class $name implements ToolMessageContentDTO {
$$$
}'
# Look for tool message processing logic
rg -l 'PyrisToolMessage' --type py27-28: Verify compatibility with existing message handlers.
The change from List[MessageContentDTO] to Union[str, List[MessageContentDTO]] is a significant modification that could affect existing code. Please ensure all message handlers can properly handle both string and list content types.
Let's check for potential compatibility issues:
✅ Verification successful
Based on the search results, I can now provide a final response as there's clear evidence of how the contents field is used throughout the codebase:
The type change is safe but requires careful testing of message conversion logic
The change from List[MessageContentDTO] to Union[str, List[MessageContentDTO]] appears safe because:
-
All existing code consistently treats messages as having a list of contents and accesses the first element:
- Message converters validate non-empty contents:
if len(iris_message.contents) == 0 - Content access pattern is always:
message.contents[0].text_content - No direct list operations (append/extend/remove) are performed on contents
- Message converters validate non-empty contents:
-
The conversion logic in
message_converters.pyproperly handles the contents field:- Creates new messages with list contents:
contents=[TextMessageContentDTO(...)] - Validates content type before access:
isinstance(message, TextMessageContentDTO)
- Creates new messages with list contents:
However, recommend testing these specific areas:
- Message conversion functions in
app/common/message_converters.py - OpenAI and Ollama message conversion in their respective adapters
- Pipeline message handling in text/exercise/lecture chat pipelines
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct access to message contents that might assume list type
rg -l 'contents\[' --type py
# Search for iterations over contents that might break with string type
rg -l 'for .+ in .+contents' --type py
# Search for list-specific method calls on contents
rg -l 'contents\.(append|extend|pop|remove)' --type py
Length of output: 305
Script:
#!/bin/bash
# Let's try a more comprehensive search for contents usage patterns
# Search for any reference to contents with surrounding context
rg -A 3 -B 3 'contents' --type py
# Search for message handling or processing methods
rg -A 5 'def .*message' --type py
# Search for any class that might be handling PyrisMessage
rg -A 10 'class.*Message' --type py
Length of output: 29376
app/llm/request_handler/basic_request_handler.py (3)
1-4: LGTM: Import statements are well-organized and necessary.
The new imports are correctly added and properly used for type hints in the bind_tools method.
41-47: LGTM: Tool binding implementation is correct and type-safe.
The implementation properly integrates with the LLM manager and follows the established pattern for request handlers. The comprehensive type hints ensure type safety when binding various tool types.
41-47: Verify LLM compatibility with tool binding.
Since tool binding is provider-specific functionality, we should verify that the underlying LLM implementation supports it.
✅ Verification successful
Let me gather more information about the LLM implementations to verify tool binding support.
Let me check the actual implementation of bind_tools in OpenAI models since we found multiple declarations.
Tool binding is properly implemented in supported models
The verification shows that tool binding is well-implemented:
- OpenAI models (both Direct and Azure) properly implement
bind_toolswith tool conversion - Base models have appropriate NotImplementedError for unsupported cases
- The request handler correctly delegates tool binding to the underlying LLM implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if LLM implementations support tool binding
# Expected: Find implementations of bind_tools in LLM classes
# Look for LLM implementations with bind_tools method
echo "Searching for bind_tools implementations in LLM classes..."
ast-grep --pattern 'class $CLASS implements LanguageModel {
$$$
bind_tools($$$) {
$$$
}
$$$
}'
# Look for potential incompatibility markers
echo "Checking for potential incompatibility markers..."
rg -i "not(\s+)?(implemented|supported)|todo|fixme" -g "app/llm/**/*.py" -A 2 -B 2
Length of output: 4582
Script:
#!/bin/bash
# Search for concrete LLM implementations and their bind_tools methods
echo "Searching for LLM class implementations..."
rg "class.*\(.*LanguageModel.*\)" -g "app/llm/**/*.py" -A 10
# Search specifically for bind_tools implementations
echo "Searching for bind_tools method implementations..."
rg "def bind_tools" -g "app/llm/**/*.py" -A 5
Length of output: 6901
Script:
#!/bin/bash
# Check the OpenAI chat model implementations
echo "Checking OpenAI chat model bind_tools implementations..."
rg "def bind_tools" -g "app/llm/external/openai_chat.py" -A 15
Length of output: 2599
app/llm/request_handler/request_handler_interface.py (3)
2-4: LGTM: Imports are well-organized and necessary.
The new imports are properly organized and provide the required type hints for the tool binding functionality.
46-52: LGTM: Well-typed method signature for tool binding.
The method signature correctly handles various tool types through Union typing, which provides flexibility for different tool implementations.
46-52: Enhance the docstring as previously suggested.
The current docstring "Bind tools" is too minimal for an interface method.
app/llm/langchain/iris_langchain_chat_model.py (2)
2-2: LGTM: Import additions support tool binding functionality.
The new imports appropriately support the implementation of native function calling, with proper type hints and necessary tool-related classes.
Also applies to: 5-5, 10-14
43-49: Consider enhancing tool binding architecture.
Since this implementation is part of a larger function calling feature, consider these architectural improvements:
- Add model capability checking (e.g., does the model support function calling?)
- Add tool compatibility verification
- Consider adding configuration options for tool execution (e.g., temperature, tool choice strategy)
Let's verify if there are any model capability checks in the codebase:
app/llm/request_handler/capability_request_handler.py (1)
2-5: LGTM: Well-organized imports
The new imports are properly organized and necessary for the added functionality.
app/llm/external/model.py (2)
2-6: LGTM: Import changes are well-structured and necessary.
The new imports properly support tool binding functionality and chat completion types, while maintaining compatibility with both pydantic v1 and v2 through appropriate aliasing.
47-47: Verify the impact of return type change across implementations.
Changing the return type from PyrisMessage to ChatCompletionMessage is a breaking change that might affect existing implementations. While it aligns with OpenAI's types, it could limit flexibility with other providers.
Consider:
- Adding a conversion layer to maintain compatibility with different providers
- Documenting this breaking change in the changelog
app/llm/external/ollama.py (1)
3-8: LGTM: Imports are well-organized and support tool binding functionality.
The new imports from typing and langchain_core are appropriate for implementing the tool binding interface.
app/pipeline/chat/interaction_suggestion_pipeline.py (2)
145-150:
Improve error handling in pipeline execution.
The current implementation has good error handling for missing last message but the catch-all exception handler silently returns an empty list, which might hide issues.
- return []
+ logger.error(f"Pipeline execution failed: {str(e)}")
+ raise RuntimeError(f"Failed to generate interaction suggestions: {str(e)}") from eLikely invalid or redundant comment.
124-126: Consider handling message content validation.
The current implementation assumes the message content is valid and only handles brace escaping. Consider adding validation for message content and proper error handling.
app/web/routers/pipelines.py (1)
78-85: Verify intended CourseChatPipeline changes
The course chat pipeline has been updated with similar parameters (variant and event) as the exercise chat pipeline. Please confirm if this is intentional, as the PR objective only mentions changes to the exercise chat functionality.
Also applies to: 105-112
✅ Verification successful
Based on the git history and code changes shown, I can now generate a final response:
CourseChatPipeline changes are intentional and part of systematic updates
The changes to CourseChatPipeline are intentional and part of a larger refactoring effort. The git history shows:
- The
variantparameter was added to support different chat modes (e.g., "jol") - The
eventparameter was added to handle specific event types like "jol" (judgment of learning) - These changes align with the codebase's evolution, particularly:
- Addition of competency and JOL-related functionality
- Improved event handling for different chat scenarios
- Consistent parameter patterns across pipeline implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CourseChatPipeline implementation for event parameter usage
ast-grep --pattern 'class CourseChatPipeline {
$$$
def __init__($_, $event, $_) {
$$$
}
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Search for CourseChatPipeline class definition and its usage
rg "class CourseChatPipeline" -A 10
# Search for run_course_chat_pipeline_worker usage
rg "run_course_chat_pipeline_worker" -A 5
# Search for any recent changes related to CourseChatPipeline
git log -p | rg "CourseChatPipeline" -A 5 -B 5
Length of output: 101872
app/web/status/status_update.py (3)
136-136: LGTM: Proper cleanup of suggestions during error state.
The addition of self.status.suggestions = None ensures a clean error state by preventing stale suggestions from being displayed to users.
167-167: LGTM: Fixed suggestion cleanup scope in skip method.
The change correctly moves suggestion cleanup from stage scope to status scope, maintaining consistency with error handling and the status object being the source of truth.
Line range hint 209-214: Review stage configuration for tool calling agent.
The stage name change to "Checking available information" aligns with the new tool calling functionality. However, please consider:
- Should the stage weight (30) be adjusted based on the expected duration of tool calling operations?
- Does the "Creating suggestions" stage need updates to handle tool-specific suggestions?
#!/bin/bash
# Description: Check for any tool-related configurations or similar stage patterns
# that might inform the weight adjustment
echo "Checking for similar stage configurations..."
rg -B 2 -A 2 "weight.*=.*30.*state.*=.*StageStateEnum"app/pipeline/chat/exercise_chat_pipeline.py (1)
Line range hint 1-424: Verify variant handling across the codebase.
Let's verify that all variant values are consistently handled and that there are no unhandled variants in the codebase.
✅ Verification successful
Variant handling is properly implemented and consistent
The verification shows that variant handling is well-implemented across the codebase:
- All variants ("default", "progress_stalled", "build_failed") are consistently defined and used
- Each variant has its corresponding system prompt template defined in
iris_exercise_chat_prompts.py - The variant logic in
exercise_chat_pipeline.pycorrectly handles all cases:- Default case uses
final_system_prompt - "progress_stalled" case uses
progress_stalled_system_prompt - "build_failed" case uses
build_failed_system_prompt
- Default case uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in variant handling
# Search for variant-related string literals
echo "Checking for variant string literals:"
rg -i "variant.*=.*['\"](?:default|progress_stalled|build_failed)['\"]" --type py
# Search for direct string comparisons with variants
echo -e "\nChecking for variant string comparisons:"
rg -i "variant.*==.*['\"](?:default|progress_stalled|build_failed)['\"]" --type py
# Look for potential variant definitions or enums
echo -e "\nChecking for variant type definitions:"
rg -i "class.*variant" --type py
Length of output: 1241
Script:
#!/bin/bash
# Let's check for all the system prompts referenced in the variant handling
echo "Checking for system prompt definitions:"
rg -l "progress_stalled_system_prompt|build_failed_system_prompt|final_system_prompt" --type py
echo -e "\nChecking the content of the prompts file:"
fd "iris_exercise_chat_prompts.py" -x cat {}
Length of output: 12559
app/pipeline/chat/exercise_chat_agent_pipeline.py (2)
219-261: Add null checks for dto.exercise and dto.exercise.end_date
As previously noted, there is a potential AttributeError when dto.exercise or dto.exercise.end_date is None in the get_additional_exercise_details function. Ensure that both are not None before performing the comparison to prevent runtime errors.
336-353: Ensure latest_result is not None before accessing feedbacks
This issue was previously raised and is still applicable. In the get_feedbacks function, check if dto.submission.latest_result is not None before accessing its feedbacks attribute to avoid an AttributeError.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
app/domain/event/pyris_event_dto.py (1)
5-5: Consider adding documentation for the type variable.While the unconstrained type variable provides flexibility, adding a docstring explaining the expected types that T might represent would improve maintainability, especially given this DTO's role in the native function calling implementation.
T = TypeVar("T") +"""Type variable representing the event payload type. + Common types might include tool call responses, function results, etc."""requirements.txt (1)
18-23: Standardize version pinning format for consistency.The new dependencies use mixed version pinning styles (
==vs~=). Consider standardizing to==for better reproducibility, matching the style used in existing dependencies.Apply this diff to standardize version pinning:
-langchain_openai==0.1.19 -starlette~=0.37.2 -langsmith~=0.1.75 -langgraph~=0.1.17 -langchain-core~=0.2.41 -langchain-text-splitters~=0.2.1 +langchain_openai==0.1.19 +starlette==0.37.2 +langsmith==0.1.75 +langgraph==0.1.17 +langchain-core==0.2.41 +langchain-text-splitters==0.2.1app/llm/external/openai_chat.py (2)
45-56: Simplify tool message handling logic.The pattern matching for tool messages can be simplified since we're only handling
ToolMessageContentDTO.- if message.sender == "TOOL": - match content: - case ToolMessageContentDTO(): - openai_messages.append( - { - "role": "tool", - "content": content.tool_content, - "tool_call_id": content.tool_call_id, - } - ) - case _: - pass + if message.sender == "TOOL" and isinstance(content, ToolMessageContentDTO): + openai_messages.append( + { + "role": "tool", + "content": content.tool_content, + "tool_call_id": content.tool_call_id, + } + )
83-104: Reduce code duplication in message construction.The message construction logic has duplicated fields. Consider extracting common fields and conditionally adding tool calls.
- if isinstance(message, PyrisAIMessage) and message.tool_calls: - openai_message = { - "role": map_role_to_str(message.sender), - "content": openai_content, - "tool_calls": [ - { - "id": tool.id, - "type": tool.type, - "function": { - "name": tool.function.name, - "arguments": json.dumps(tool.function.arguments), - }, - } - for tool in message.tool_calls - ], - } - else: - openai_message = { - "role": map_role_to_str(message.sender), - "content": openai_content, - } + openai_message = { + "role": map_role_to_str(message.sender), + "content": openai_content, + } + + if isinstance(message, PyrisAIMessage) and message.tool_calls: + openai_message["tool_calls"] = [ + { + "id": tool.id, + "type": tool.type, + "function": { + "name": tool.function.name, + "arguments": json.dumps(tool.function.arguments), + }, + } + for tool in message.tool_calls + ]app/common/message_converters.py (1)
58-75: Refactor duplicate code by creating a helper function.The functions
convert_iris_message_to_langchain_human_messageandextract_text_from_iris_messagecontain duplicate code for extracting and validating the message content fromiris_message. Refactoring this repeated logic into a shared helper function enhances maintainability and reduces code duplication.Consider implementing a helper function as shown below:
def _extract_text_content(iris_message: PyrisMessage) -> str: if not iris_message.contents: raise ValueError("IrisMessage contents must not be empty") message = iris_message.contents[0] if not isinstance(message, TextMessageContentDTO): raise ValueError("Message must be of type TextMessageContentDTO") return message.text_content def convert_iris_message_to_langchain_human_message( iris_message: PyrisMessage, ) -> HumanMessage: content = _extract_text_content(iris_message) return HumanMessage(content=content) def extract_text_from_iris_message(iris_message: PyrisMessage) -> str: return _extract_text_content(iris_message)app/pipeline/chat/exercise_chat_agent_pipeline.py (4)
129-129: Updateimplementation_idto match the class name for consistencyThe
implementation_idin the superclass initializer is set to"exercise_chat_pipeline", whereas the class is namedExerciseChatAgentPipeline. Updating theimplementation_idto match the class name enhances clarity and consistency.Apply this diff to update the
implementation_id:- super().__init__(implementation_id="exercise_chat_pipeline") + super().__init__(implementation_id="exercise_chat_agent_pipeline")
121-121: Ensure type annotations are compatible with Python versionsThe type annotation
str | Noneis valid in Python 3.10 and above. If the codebase needs to support earlier versions of Python, consider usingOptional[str]from thetypingmodule for compatibility.Apply this diff for compatibility with earlier Python versions:
- event: str | None + from typing import Optional + event: Optional[str]
568-571: ReplaceUsing
Apply this diff to use
logger.infoinstead of- print("Response is ok and not rewritten!!!") + logger.info("Response is okay and not rewritten.") - print("Response is rewritten.") + logger.info("Response is rewritten.")
144-151: Remove unused attributes to improve maintainabilityThe attributes
self.db,self.retriever,self.reranker_pipeline,self.code_feedback_pipeline, andself.citation_pipelineare defined but not used in the class. Removing unused attributes cleans up the code and reduces potential confusion.Apply this diff to remove unused attributes:
# Create the pipelines - self.db = VectorDatabase() self.suggestion_pipeline = InteractionSuggestionPipeline(variant="exercise") - self.retriever = LectureRetrieval(self.db.client) - self.reranker_pipeline = RerankerPipeline() - self.code_feedback_pipeline = CodeFeedbackPipeline() - self.pipeline = self.llm | JsonOutputParser() - self.citation_pipeline = CitationPipeline()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
app/common/__init__.py(0 hunks)app/common/message_converters.py(3 hunks)app/common/pyris_message.py(2 hunks)app/domain/event/pyris_event_dto.py(1 hunks)app/llm/external/openai_chat.py(5 hunks)app/llm/langchain/iris_langchain_chat_model.py(2 hunks)app/pipeline/chat/course_chat_pipeline.py(7 hunks)app/pipeline/chat/exercise_chat_agent_pipeline.py(1 hunks)app/pipeline/chat/interaction_suggestion_pipeline.py(3 hunks)app/pipeline/chat/lecture_chat_pipeline.py(1 hunks)app/retrieval/lecture_retrieval.py(1 hunks)requirements.txt(1 hunks)
💤 Files with no reviewable changes (1)
- app/common/init.py
✅ Files skipped from review due to trivial changes (1)
- app/pipeline/chat/lecture_chat_pipeline.py
🚧 Files skipped from review as they are similar to previous changes (3)
- app/common/pyris_message.py
- app/llm/langchain/iris_langchain_chat_model.py
- app/pipeline/chat/interaction_suggestion_pipeline.py
🧰 Additional context used
📓 Learnings (1)
app/llm/external/openai_chat.py (1)
Learnt from: kaancayli
PR: ls1intum/Pyris#154
File: app/llm/external/openai_chat.py:124-140
Timestamp: 2024-11-11T22:18:36.596Z
Learning: In the codebase, it's acceptable for `PyrisMessage` and `PyrisAIMessage` instances to have `contents` arrays containing messages with empty string contents, as validation only checks if the `contents` array is empty.
🔇 Additional comments (8)
app/domain/event/pyris_event_dto.py (2)
1-3: LGTM! Well-organized imports following best practices.
The imports are properly organized with standard library imports before third-party imports, and using specific imports rather than wildcards.
8-10: Add class-level documentation and verify OpenAI compatibility.
While the implementation is solid, consider these improvements:
- Add class-level documentation to explain the DTO's role in the native function calling system.
- Verify that this structure aligns with OpenAI's function calling response format.
class PyrisEventDTO(BaseModel, Generic[T]):
+ """Data Transfer Object for handling events in the native function calling system.
+
+ This DTO encapsulates events that occur during tool/function execution,
+ providing a standardized way to handle both successful results and errors.
+
+ Attributes:
+ event_type: The type of event (e.g., "tool_response", "error")
+ event: The actual event payload, typed according to the event_type
+ """
event_type: Optional[str] = Field(default=None, alias="eventType")
event: Optional[T] = Field(default=None, alias="event")requirements.txt (1)
18-23: Verify alignment with PR objectives.
The added dependencies align well with the PR objective of implementing native function calling agent capabilities, particularly through the inclusion of langchain_openai and related packages.
app/pipeline/chat/course_chat_pipeline.py (4)
82-90: LGTM: Event handling support added correctly.
The addition of the event attribute and constructor parameter provides a clean way to support event-specific behavior in the pipeline.
246-249: LGTM: Clear documentation of competency metrics.
The docstring updates clearly explain the metrics, including the mastery percentage range and JOL values with their confidence computations.
335-357: LGTM: JOL event handling implemented correctly.
The implementation properly handles JOL events by:
- Validating the event payload
- Finding the relevant competency
- Calculating competency mastery using the standardized
get_masteryfunction - Preparing the response with both JOL value and mastery information
280-281: LGTM: Clear documentation of RAG retrieval behavior.
The docstring update clearly explains that the retrieval is based on chat history, which is important for understanding the context-aware nature of the retrieval.
app/pipeline/chat/exercise_chat_agent_pipeline.py (1)
550-552: Check for empty agent invocation parameters
The params dictionary passed to agent_executor.invoke(params) is empty. Verify whether this is intentional. If not required, you can omit it; otherwise, ensure that necessary parameters are included.
Run the following script to search for any usage of params within the agent_executor methods:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/llm/langchain/iris_langchain_chat_model.py (1)
43-68: Add additional validations for tool binding.While the implementation looks good and includes the basic validations, consider adding these safety checks:
- Validate individual tool types before binding
- Verify request handler's readiness
- Add error handling for the bind_tools call
def bind_tools( self, tools: Sequence[Union[Dict[str, Any], Type[BaseModel], Callable, BaseTool]], **kwargs: Any, ) -> Runnable[LanguageModelInput, BaseMessage]: """Bind a sequence of tools to the request handler for function calling support. Args: tools: Sequence of tools that can be one of: - Dict describing the tool - Pydantic BaseModel - Callable function - BaseTool instance **kwargs: Additional arguments passed to the request handler Returns: self: Returns this instance as a Runnable Raises: ValueError: If tools sequence is empty or contains invalid tool types + RuntimeError: If request handler is not ready """ if not tools: raise ValueError("At least one tool must be provided") + # Validate individual tool types + valid_types = (dict, BaseModel, Callable, BaseTool) + for tool in tools: + if not isinstance(tool, valid_types) and not ( + isinstance(tool, type) and issubclass(tool, BaseModel) + ): + raise ValueError(f"Invalid tool type: {type(tool)}") + + # Verify request handler readiness + if not hasattr(self.request_handler, 'bind_tools'): + raise RuntimeError("Request handler does not support tool binding") + + try: self.request_handler.bind_tools(tools) + except Exception as e: + raise RuntimeError(f"Failed to bind tools: {str(e)}") + return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/llm/langchain/iris_langchain_chat_model.py(2 hunks)
🔇 Additional comments (2)
app/llm/langchain/iris_langchain_chat_model.py (2)
2-2: LGTM! Import changes align with tool binding feature.
The new imports properly support the tool binding functionality and follow good organization practices.
Also applies to: 13-14
67-67: Verify request handler's tool binding implementation.
Let's ensure the request handler properly implements the tool binding feature.
✅ Verification successful
Tool binding implementation is properly supported across the request handler hierarchy
The verification shows that tool binding is correctly implemented:
RequestHandlerinterface defines thebind_toolscontract- Both concrete handlers (
BasicRequestHandlerandCapabilityRequestHandler) implement the method - The handlers delegate tool binding to the underlying LLM implementations
- The implementation in
IrisLangchainChatModelaligns with the interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if request handler implements tool binding support
# Search for request handler implementations with bind_tools method
ast-grep --pattern 'class $_ implements RequestHandler {
$$$
bind_tools($$$) {
$$$
}
$$$
}'
# Search for any existing tool binding tests
rg -l "test.*bind.*tools?" --type=python
Length of output: 179
Script:
#!/bin/bash
# Let's try different approaches to find the request handler implementation
# Search for RequestHandler class definition
rg -A 5 "class.*RequestHandler"
# Search for bind_tools method
rg -A 5 "def bind_tools"
# Search for files containing RequestHandler
fd RequestHandler
# Search for test files related to request handler
fd -e py test.*request.*handler -i
Length of output: 5339
Script:
#!/bin/bash
# Let's examine the implementation details of bind_tools in relevant files
# Check CapabilityRequestHandler bind_tools implementation
rg -A 10 "def bind_tools" app/llm/request_handler/capability_request_handler.py
# Check BasicRequestHandler bind_tools implementation
rg -A 10 "def bind_tools" app/llm/request_handler/basic_request_handler.py
# Check IrisLangchainChatModel bind_tools implementation
rg -A 10 "def bind_tools" app/llm/langchain/iris_langchain_chat_model.py
Length of output: 1203
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kaancayli
force-pushed
the
feature/exercise-chat/agent
branch
from
05fcc49
to
ec52f25
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
app/llm/llm_manager.py (2)
43-43: Consider version compatibility and error handling improvements
- The change to
model_validateindicates a Pydantic v2 upgrade. Ensure the project's dependencies are updated accordingly.- Consider adding more detailed error handling for YAML parsing and model validation failures.
- self.entries = LlmList.model_validate({"llms": loaded_llms}).llms + try: + self.entries = LlmList.model_validate({"llms": loaded_llms}).llms + except Exception as e: + raise ValueError(f"Failed to validate LLM configuration: {str(e)}") from e
Line range hint
1-58: Consider documenting tool calling architectureWhile these changes support polymorphic LLM handling, which is necessary for tool calling, the actual tool calling implementation isn't visible here. Consider:
- Adding documentation about how different LLM types implement tool calling
- Including examples in the LLM configuration for tool-enabled models
app/llm/request_handler/basic_request_handler.py (1)
21-23: Simplify initialization pattern.The current initialization pattern creates a brief window where
llm_managerisNone. Consider passing the initialized manager directly to super():def __init__(self, model_id: str): - super().__init__(model_id=model_id, llm_manager=None) - self.model_id = model_id - self.llm_manager = LlmManager() + llm_manager = LlmManager() + super().__init__(model_id=model_id, llm_manager=llm_manager) + self.model_id = model_idapp/llm/langchain/iris_langchain_chat_model.py (1)
44-69: Consider future extensibility for non-OpenAI tool calling.While the current implementation focuses on OpenAI tool calling, the flexible design with
BaseTooland the generic tool sequence type hints will make it easier to extend to other providers. Consider documenting the extension points and provider-specific requirements in the class docstring for future implementations.app/llm/request_handler/capability_request_handler.py (2)
33-34: Review the llm_manager initialization patternThe
llm_manageris marked as optional withNonedefault, but it's always initialized in__init__. This creates a misleading type hint. Consider either:
- Removing the
Nonedefault if it's always required- Making it truly optional if there are cases where it's not needed
- llm_manager: LlmManager | None = None + llm_manager: LlmManager
82-112: Enhance tool validation in bind_toolsWhile the basic validation is good, consider adding:
- Type validation for individual tools in the sequence
- Validation of tool configurations (for Dict type)
- More specific error messages indicating which tool failed validation
def bind_tools( self, tools: Sequence[Union[Dict[str, Any], Type[BaseModel], Callable, BaseTool]], ) -> LanguageModel: if not tools: raise ValueError("Tools sequence cannot be empty") + for i, tool in enumerate(tools): + if not isinstance(tool, (dict, type, Callable, BaseTool)): + raise TypeError( + f"Tool at index {i} has unsupported type: {type(tool).__name__}" + ) + if isinstance(tool, dict) and not all(k in tool for k in ['name', 'description']): + raise ValueError( + f"Tool configuration at index {i} missing required fields" + ) + llm = self._select_model(ChatModel) if not hasattr(llm, "bind_tools"): raise TypeError(app/pipeline/chat/course_chat_pipeline.py (2)
246-249: Documentation improvement: Add examples for metrics and JOL values.The documentation would benefit from concrete examples of the metrics and JOL values to help developers understand the expected ranges and formats.
The response may include metrics for each competency, such as progress and mastery (0%-100%). These are system-generated. - The judgment of learning (JOL) values indicate the self-reported confidence by the student (0-5, 5 star). - The object describing it also indicates the system-computed confidence at the time when the student + The judgment of learning (JOL) values indicate the self-reported confidence by the student (0-5, 5 star). + Example: {"value": 4, "timestamp": "2024-11-01T10:00:00Z", "system_confidence": 0.85} + The object also indicates the system-computed confidence (0-1) at the time when the student
276-281: Enhance lecture content retrieval documentation.The documentation could be more specific about the RAG (Retrieval-Augmented Generation) process and its parameters.
- This will run a RAG retrieval based on the chat history on the indexed lecture slides and return the - most relevant paragraphs. + This will run a RAG (Retrieval-Augmented Generation) retrieval on indexed lecture slides using: + - Recent chat history (last 5 messages) + - Student's latest query + - Course context (name and ID) + Returns the top 5 most relevant paragraphs with lecture metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
app/llm/external/__init__.py(2 hunks)app/llm/external/model.py(2 hunks)app/llm/external/ollama.py(3 hunks)app/llm/external/openai_chat.py(3 hunks)app/llm/langchain/iris_langchain_chat_model.py(2 hunks)app/llm/llm_manager.py(3 hunks)app/llm/request_handler/basic_request_handler.py(2 hunks)app/llm/request_handler/capability_request_handler.py(3 hunks)app/llm/request_handler/request_handler_interface.py(2 hunks)app/pipeline/chat/course_chat_pipeline.py(7 hunks)requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/llm/external/model.py
- app/llm/external/ollama.py
- app/llm/request_handler/request_handler_interface.py
- requirements.txt
🧰 Additional context used
🪛 Ruff
app/llm/langchain/iris_langchain_chat_model.py
37-37: Do not perform function call CompletionArguments in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (12)
app/llm/external/__init__.py (2)
1-1: LGTM! Clean import of Union type.
The explicit import of Union from typing module follows Python type hinting best practices.
15-23: Consider documenting tool binding support requirements.
Given that this PR implements native function calling capabilities, it would be helpful to document which of these LLM implementations support tool binding. Not all models (especially embedding models) would typically support function calling features.
Let's verify which models implement tool binding:
✅ Verification successful
Document Tool Binding Support for LLM Models
Document that the following models support tool binding:
- DirectOpenAICompletionModel
- AzureOpenAICompletionModel
- DirectOpenAIChatModel
- AzureOpenAIChatModel
- OllamaModel
The embedding models do not support tool binding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check which model classes implement tool binding functionality
# Search for classes that implement bind_tools method
ast-grep --pattern 'class $CLASS_NAME($_) {
$$$
def bind_tools($$$) {
$$$
}
$$$
}'
# Also search for abstract method declarations
ast-grep --pattern 'class $CLASS_NAME($_) {
$$$
@abstractmethod
def bind_tools($$$):
$$$
$$$
}'
Length of output: 206
Script:
#!/bin/bash
# Description: Improved search for classes implementing or declaring bind_tools method
# Search for classes that implement bind_tools method with any parameters and decorators
ast-grep --pattern 'class $_($_) {
$$$
@?[$$_decorator]?
def bind_tools($_args) {
$$$
}
$$$
}'
# Search for bind_tools method declarations with decorators like @abstractmethod
ast-grep --pattern 'class $_($_) {
$$$
@abstractmethod
def bind_tools($_args):
$$$
$$$
}'
Length of output: 214
Script:
#!/bin/bash
# Description: Search for all occurrences of bind_tools method definitions using ripgrep
# Search for method definitions named bind_tools with any decorators and parameters
rg -n --pretty 'def bind_tools\('
# Additionally, search for any class that inherits from a base class that might define bind_tools
rg -n --pretty 'class\s+\w+\s*\(.*\):' | rg -i 'bind_tools'
Length of output: 552
app/llm/llm_manager.py (1)
2-2: LGTM: Import changes align with modern type hinting practices
The switch from Field to Discriminator and addition of Annotated support better type discrimination for polymorphic models.
Also applies to: 4-4
app/llm/request_handler/basic_request_handler.py (2)
1-5: LGTM: Import statements are well-organized and necessary.
The new imports support the tool binding functionality and follow Python's import style guidelines.
17-18:
Consider the implications of nullable llm_manager.
While making llm_manager nullable aligns with the constructor change, all methods (complete, chat, embed, bind_tools) assume it's non-null and directly use it without checks. This could lead to NullPointerException.
Let's verify if there are any null checks in the codebase:
Consider either:
- Adding null checks in methods:
def bind_tools(self, tools: Sequence[Union[Dict[str, Any], Type[BaseModel], Callable, BaseTool]]) -> LanguageModel:
if self.llm_manager is None:
raise ValueError("LlmManager is not initialized")
llm = self.llm_manager.get_llm_by_id(self.model_id)
llm.bind_tools(tools)
return llm- Or ensuring llm_manager is always initialized:
llm_manager: LlmManagerapp/llm/langchain/iris_langchain_chat_model.py (3)
2-15: LGTM: Import changes are well-organized and support the new functionality.
The new imports are properly organized and necessary for implementing the tool calling feature.
32-32: LGTM: Logger type annotation improves code clarity.
The addition of type annotation for the logger follows Python typing best practices.
44-69: LGTM: Well-implemented tool binding with proper validation and documentation.
The implementation is clean, type-safe, and includes proper validation and comprehensive documentation. The method chaining pattern is a nice touch for API usability.
app/llm/request_handler/capability_request_handler.py (1)
105-105: Document OpenAI-specific tool binding support
The PR indicates this is specifically for OpenAI tool binding, but the code assumes all ChatModel instances support it. Consider:
- Adding a comment indicating OpenAI-specific support
- Adding a more specific model check beyond just
ChatModel
app/pipeline/chat/course_chat_pipeline.py (1)
256-264: 🛠️ Refactor suggestion
Improve mastery calculation configuration and consistency.
- The weight factor is hardcoded and lacks explanation
- The mastery calculation differs from the
get_masteryfunction
- weight = 2.0 / 3.0
+ # Weight factor: 2/3 for confidence and 1/3 for progress
+ CONFIDENCE_WEIGHT = 2.0 / 3.0
return [
{
"info": competency_metrics.competency_information.get(comp, None),
"exercise_ids": competency_metrics.exercises.get(comp, []),
"progress": competency_metrics.progress.get(comp, 0),
- "mastery": (
- (1 - weight) * competency_metrics.progress.get(comp, 0)
- + weight * competency_metrics.confidence.get(comp, 0)
- ),
+ "mastery": get_mastery(
+ competency_metrics.progress.get(comp, 0),
+ competency_metrics.confidence.get(comp, 0)
+ ),Likely invalid or redundant comment.
app/llm/external/openai_chat.py (2)
66-79: Validate the structure of tool calls for OpenAI API compatibility.
The create_openai_tool_calls function formats tool calls for the API:
{
"id": tool.id,
"type": tool.type,
"function": {
"name": tool.function.name,
"arguments": json.dumps(tool.function.arguments),
},
}Confirm that this structure aligns with OpenAI's function calling API specifications. Any discrepancies might result in API errors.
Run the following script to verify the tool call structure:
Alternatively, consult OpenAI's official documentation to ensure compliance.
181-190: Confirm handling of empty contents in PyrisAIMessage.
When message.tool_calls is present, the convert_to_iris_message function returns a PyrisAIMessage with empty contents:
contents=[TextMessageContentDTO(textContent="")],Verify that downstream components can handle PyrisAIMessage instances with empty content strings without issues.
Run the following script to check for potential problems:
Ensure that all parts of the codebase gracefully handle empty content strings.
✅ Verification successful
Downstream components handle empty contents correctly in PyrisAIMessage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of PyrisAIMessage and check handling of contents.
# Find all usages of PyrisAIMessage where contents are accessed.
rg 'PyrisAIMessage' -A 5
# Look for any assumptions that contents are non-empty.
rg 'message.contents\[' -A 2
Length of output: 5066
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/pipeline/chat/exercise_chat_agent_pipeline.py (4)
62-64: Consider using template strings for better maintainabilityThe current string replacement approach for curly braces is error-prone and harder to maintain.
Consider using a template string or format string:
- - **Exercise Title:** {exercise_title.replace("{", "{{").replace("}", "}}")} - - **Problem Statement:** {problem_statement.replace("{", "{{").replace("}", "}}")} - - **Programming Language:** {programming_language} + - **Exercise Title:** {{exercise_title}} + - **Problem Statement:** {{problem_statement}} + - **Programming Language:** {programming_language}
134-142: Document the rationale for temperature settingThe LLM temperature setting of 0.5 is a crucial parameter that affects response randomness, but its choice isn't documented.
Add a comment explaining why this specific temperature value was chosen:
- completion_args = CompletionArguments(temperature=0.5, max_tokens=2000) + # Temperature of 0.5 provides a balance between creative and deterministic responses + # while maintaining consistency in exercise-related explanations + completion_args = CompletionArguments(temperature=0.5, max_tokens=2000)
387-394: Consider making result_limit configurableThe result limit of 5 is hardcoded in the lecture content retrieval, which reduces flexibility.
Consider making it configurable:
+ def __init__(self, ..., lecture_result_limit: int = 5): + self.lecture_result_limit = lecture_result_limit + def lecture_content_retrieval(...): self.retrieved_paragraphs = self.retriever( chat_history=chat_history, student_query=query.contents[0].text_content, - result_limit=5, + result_limit=self.lecture_result_limit, course_name=dto.course.name,
556-560: Replace magic string with constantThe string "!ok!" is used as a magic string for response validation.
Define it as a class constant:
+ RESPONSE_APPROVAL_MARKER = "!ok!" + def __call__(self, dto: ExerciseChatPipelineExecutionDTO): # ... - if "!ok!" in guide_response: + if self.RESPONSE_APPROVAL_MARKER in guide_response: print("Response is ok and not rewritten!!!")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/common/PipelineEnum.py(1 hunks)app/pipeline/chat/exercise_chat_agent_pipeline.py(1 hunks)app/pipeline/chat/exercise_chat_pipeline.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pipeline/chat/exercise_chat_pipeline.py
🔇 Additional comments (3)
app/common/PipelineEnum.py (2)
8-8: LGTM! The new enum member follows conventions.
The addition of IRIS_CHAT_EXERCISE_AGENT_MESSAGE follows the established naming patterns and is appropriately placed near related exercise chat enums.
8-8: Verify enum handling in switch/match statements.
Since this is a new enum value, ensure all switch/match statements handling PipelineEnum are updated to handle this new case where necessary.
app/pipeline/chat/exercise_chat_agent_pipeline.py (1)
606-623: LGTM! Well-implemented utility method
The should_allow_lecture_tool method is well-implemented with proper error handling, type hints, and documentation.
❌ Unable to deploy to test server ❌Pyris Testserver is already in use by PR #142. |
This PR implements the exercise chat as a native tool calling agent. For now only OpenAI tool calling is supported.
More details will be added.
Summary by CodeRabbit
New Features
ExerciseChatAgentPipelinefor improved interactions regarding exercise-related queries.PyrisEventDTOfor flexible event handling.IRIS_CHAT_EXERCISE_AGENT_MESSAGEenum member to represent pipeline states.Bug Fixes
Documentation
Chores