Add ChatGPT wrapper pipeline #196
Conversation
…sometimes otherwise
|
Followup to #185, #194 and #195 as I was not able to make push any more changes to the existing branch due to rule: |
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis pull request introduces several enhancements across the application, focusing on documentation improvements, pipeline modifications, and error handling. The changes include updating the README documentation with consistent file naming conventions, modifying the OpenAPI schema to add bearer authentication, introducing a new ChatGPT wrapper pipeline, and improving error logging with Sentry integration. The modifications span multiple components of the application, including web routers, pipelines, and domain models. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIRouter
participant ChatGPTWrapperPipeline
participant LLM
participant Callback
Client->>APIRouter: Send chat request
APIRouter->>ChatGPTWrapperPipeline: Initialize pipeline
ChatGPTWrapperPipeline->>LLM: Generate response
LLM-->>ChatGPTWrapperPipeline: Return response
ChatGPTWrapperPipeline->>Callback: Trigger status updates
Callback-->>Client: Provide final response
Suggested Labels
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
🧹 Nitpick comments (4)
app/pipeline/chat_gpt_wrapper_pipeline.py (2)
42-43: Simplify the f-string formatting for improved readabilityThe nested f-strings in the return statement are hard to read and maintain. Consider simplifying the formatting to enhance readability.
Here's a suggested refactor:
- f"{map_message_role(message.sender)} {"" if not message.sent_at else f"at {message.sent_at.strftime( - "%Y-%m-%d %H:%M:%S")}"}: {message.contents[0].text_content}" + timestamp = f" at {message.sent_at.strftime('%Y-%m-%d %H:%M:%S')}" if message.sent_at else "" + f"{map_message_role(message.sender)}{timestamp}: {message.contents[0].text_content}"
51-52: Remove unused class attributesllmandpipelineThe class attributes
llmandpipelineare declared but not initialized or used within the class. Consider removing them if they are unnecessary, or ensure they are properly initialized and utilized.app/web/routers/pipelines.py (2)
239-261: Consider adding variant-specific error messages.The error handling is implemented well, but the error messages could be more specific to help with debugging wrapper-specific issues.
- logger.error(f"Error preparing ChatGPT wrapper pipeline: {e}") + logger.error(f"Error preparing ChatGPT wrapper pipeline (variant: {_variant}): {e}") - logger.error(f"Error running ChatGPT wrapper pipeline: {e}") + logger.error(f"Error running ChatGPT wrapper pipeline (variant: {_variant}): {e}")
325-332: Enhance feature description for the ChatGPT wrapper.The feature description could be more informative about the experimental nature mentioned in the PR description.
id="default", name="Default Variant", - description="Default ChatGPT wrapper variant.", + description="Experimental ChatGPT wrapper variant for direct model interaction.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
application.example.ymlis excluded by!**/*.ymlexample_application.ymlis excluded by!**/*.yml
📒 Files selected for processing (10)
README.MD(6 hunks)app/domain/status/text_exercise_chat_status_update_dto.py(1 hunks)app/main.py(1 hunks)app/pipeline/chat/course_chat_pipeline.py(1 hunks)app/pipeline/chat/exercise_chat_agent_pipeline.py(1 hunks)app/pipeline/chat_gpt_wrapper_pipeline.py(1 hunks)app/pipeline/prompts/chat_gpt_wrapper_prompts.py(1 hunks)app/pipeline/shared/citation_pipeline.py(1 hunks)app/web/routers/pipelines.py(4 hunks)app/web/status/status_update.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- app/pipeline/prompts/chat_gpt_wrapper_prompts.py
- app/pipeline/shared/citation_pipeline.py
- app/pipeline/chat/exercise_chat_agent_pipeline.py
- app/pipeline/chat/course_chat_pipeline.py
🧰 Additional context used
🪛 LanguageTool
README.MD
[uncategorized] ~323-~323: Loose punctuation mark.
Context: ...lfile. -PYRIS_LLM_CONFIG_YML_FILE: Path to your llm_config.yml` file. -...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (7)
app/pipeline/chat_gpt_wrapper_pipeline.py (1)
87-88: Verify the necessity of callingself.callback.done()twiceThe method
self.callback.done()is called twice consecutively—once without arguments and once withfinal_result. Confirm if both calls are necessary or if they can be consolidated into a single call with thefinal_resultparameter.app/domain/status/text_exercise_chat_status_update_dto.py (1)
7-7: Adjustment toresultbeing optional looks goodChanging
resulttoOptional[str]appropriately allows for cases where a result may not be available, aligning with the updated pipeline behavior.app/main.py (1)
32-34: Ensure security requirements are correctly applied to all pathsWhen applying the security scheme globally, verify that it doesn't unintentionally restrict access to endpoints intended to be public. The current implementation adds the
bearerAuthrequirement to all methods, which might override existing security configurations.app/web/routers/pipelines.py (1)
78-83: LGTM! Clean implementation of variant routing.The conditional routing for the ChatGPT wrapper variant is implemented correctly, maintaining consistency with the existing pattern.
app/web/status/status_update.py (1)
3-3: LGTM! Good addition of error tracking.The addition of Sentry SDK for error tracking will improve observability.
README.MD (2)
187-187: LGTM! Important warning about model requirements.Clear warning about the GPT version requirements helps prevent configuration issues.
98-101: Verify the consistency of configuration file names.The change from hyphen to underscore in configuration file names needs to be verified across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/pipeline/chat_gpt_wrapper_pipeline.py (3)
23-46: Consider extracting the map_message_role function and adding error handling.The chat history conversion looks good, but could be improved in two ways:
- Extract
map_message_roleas a standalone function for reusability- Add validation for empty message contents
+def map_message_role(role: IrisMessageRole) -> str: + if role == IrisMessageRole.SYSTEM: + return "System" + elif role == IrisMessageRole.ASSISTANT: + return "AI Tutor" + elif role == IrisMessageRole.USER: + return "Student" + else: + return "Unknown" + def convert_chat_history_to_str(chat_history: List[PyrisMessage]) -> str: """ Converts the chat history to a string :param chat_history: The chat history :return: The chat history as a string """ - def map_message_role(role: IrisMessageRole) -> str: - if role == IrisMessageRole.SYSTEM: - return "System" - elif role == IrisMessageRole.ASSISTANT: - return "AI Tutor" - elif role == IrisMessageRole.USER: - return "Student" - else: - return "Unknown" + if not chat_history: + return "" return "\n\n".join( [ f"{map_message_role(message.sender)} {"" if not message.sent_at else f"at {message.sent_at.strftime( "%Y-%m-%d %H:%M:%S")}"}: {message.contents[0].text_content}" - for message in chat_history + for message in chat_history + if message.contents and message.contents[0].text_content ] )
49-53: Add type hints and class documentation.The class attributes should have proper type hints, and the class should have a docstring explaining its purpose.
class ChatGPTWrapperPipeline(Pipeline): - callback: ChatGPTWrapperStatusCallback - llm: IrisLangchainChatModel - pipeline: Runnable + """ + A pipeline that wraps ChatGPT model interactions. + Handles chat history formatting and response generation using GPT-4.5. + """ + callback: Optional[ChatGPTWrapperStatusCallback] + llm: Optional[IrisLangchainChatModel] + pipeline: Optional[Runnable]
85-87: Consider making completion arguments configurable.The temperature and max_tokens values are hardcoded. Consider making these configurable through the DTO or pipeline settings.
response = self.request_handler.chat( - prompts, CompletionArguments(temperature=0.5, max_tokens=2000) + prompts, + CompletionArguments( + temperature=dto.settings.temperature if hasattr(dto.settings, 'temperature') else 0.5, + max_tokens=dto.settings.max_tokens if hasattr(dto.settings, 'max_tokens') else 2000 + ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/pipeline/chat_gpt_wrapper_pipeline.py(1 hunks)app/web/routers/pipelines.py(5 hunks)app/web/status/status_update.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run linters
app/web/status/status_update.py
[warning] File needs to be reformatted using Black code formatter
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (1)
app/web/routers/pipelines.py (1)
79-84: Handle the event parameter consistently.The
eventparameter is ignored when using the chat-gpt-wrapper variant but is passed to other variants. Either pass it consistently or document why it's not needed for the wrapper.if variant == "chat-gpt-wrapper": - thread = Thread(target=run_chatgpt_wrapper_pipeline_worker, args=(dto, variant)) + thread = Thread(target=run_chatgpt_wrapper_pipeline_worker, args=(dto, variant, event)) else: thread = Thread( target=run_exercise_chat_pipeline_worker, args=(dto, variant, event) )
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/pipeline/chat_gpt_wrapper_pipeline.py (3)
31-40: Consider moving the role mapping function to a separate utility module.The
map_message_rolefunction could be useful in other parts of the codebase. Consider moving it to a utility module for better reusability.🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
84-84: Improve chat history filtering readability.The current filtering condition is complex and hard to read. Consider breaking it down into a separate function.
+ def is_valid_message(msg: PyrisMessage) -> bool: + return ( + msg.contents is not None + and len(msg.contents) > 0 + and msg.contents[0].text_content + and len(msg.contents[0].text_content) > 0 + ) + - prompts = [pyris_system_prompt] + [msg for msg in dto.chat_history if msg.contents is not None and len(msg.contents) > 0 and msg.contents[0].text_content and len(msg.contents[0].text_content) > 0] + prompts = [pyris_system_prompt] + [msg for msg in dto.chat_history if is_valid_message(msg)]🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
87-87: Consider moving completion arguments to configuration.The temperature and max_tokens values are hard-coded. Consider moving these to configuration for better flexibility.
+ # Consider moving these to configuration + temperature = 0.5 # from config + max_tokens = 2000 # from config response = self.request_handler.chat( - prompts, CompletionArguments(temperature=0.5, max_tokens=2000) + prompts, CompletionArguments(temperature=temperature, max_tokens=max_tokens) )🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/pipeline/chat_gpt_wrapper_pipeline.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/pipeline/chat_gpt_wrapper_pipeline.py
1-1: json imported but unused
Remove unused import: json
(F401)
🪛 GitHub Actions: Run linters
app/pipeline/chat_gpt_wrapper_pipeline.py
[warning] File needs to be reformatted using Black code formatter
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (1)
app/pipeline/chat_gpt_wrapper_pipeline.py (1)
55-63: 🛠️ Refactor suggestionAdd callback validation and consider configuration for model parameters.
Two suggestions for improvement:
- Add validation for the callback parameter to prevent NoneType errors.
- Consider moving the GPT version and context length to configuration.
Consider this implementation:
def __init__(self, callback: Optional[ChatGPTWrapperStatusCallback] = None): super().__init__(implementation_id="chat_gpt_wrapper_pipeline_reference_impl") + if callback is None: + raise ValueError("ChatGPTWrapperStatusCallback is required") self.callback = callback + + # Consider moving these to configuration + gpt_version = 4.5 # from config + context_length = 16385 # from config self.request_handler = CapabilityRequestHandler( requirements=RequirementList( - gpt_version_equivalent=4.5, - context_length=16385, + gpt_version_equivalent=gpt_version, + context_length=context_length, ) )Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/pipeline/chat_gpt_wrapper_pipeline.py (2)
41-47: 🛠️ Refactor suggestionImprove readability and add error handling.
The current implementation needs defensive programming and better readability.
- return "\n\n".join( - [ - f"{map_message_role(message.sender)} {"" if not message.sent_at else f"at {message.sent_at.strftime( - "%Y-%m-%d %H:%M:%S")}"}: {message.contents[0].text_content}" - for message in chat_history - ] - ) + formatted_messages = [] + for message in chat_history: + if not message.contents: + continue + + sender = map_message_role(message.sender) + timestamp = f"at {message.sent_at.strftime('%Y-%m-%d %H:%M:%S')}" if message.sent_at else "" + content = message.contents[0].text_content if message.contents else "" + + formatted_message = f"{sender} {timestamp}: {content}".strip() + formatted_messages.append(formatted_message) + + return "\n\n".join(formatted_messages)🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
86-88:⚠️ Potential issueAdd error handling for API request.
The chat request should be wrapped in a try-catch block to handle potential API errors.
+ try: response = self.request_handler.chat( prompts, CompletionArguments(temperature=0.5, max_tokens=2000) ) + except Exception as e: + logger.error(f"Error calling ChatGPT API: {str(e)}") + self.callback.error(f"Failed to get response from ChatGPT: {str(e)}") + return🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
🧹 Nitpick comments (3)
app/llm/external/openai_chat.py (1)
246-249: Enhance error logging for empty messages.While the empty message check is good, the log message could be more informative by including context about the request.
if len(choice.message) == 0: - logging.error("Model returned an empty message") + logging.error( + "Model returned an empty message", + extra={ + "model": model, + "finish_reason": choice.finish_reason, + "usage": usage._asdict() if usage else None + } + )app/pipeline/chat_gpt_wrapper_pipeline.py (2)
84-84: Improve readability of prompt filtering.The current line is too long and contains multiple conditions. Consider breaking it down into a separate function or using intermediate variables.
- prompts = [pyris_system_prompt] + [msg for msg in dto.chat_history if msg.contents is not None and len(msg.contents) > 0 and msg.contents[0].text_content and len(msg.contents[0].text_content) > 0] + def is_valid_message(msg: PyrisMessage) -> bool: + return (msg.contents is not None and + len(msg.contents) > 0 and + msg.contents[0].text_content and + len(msg.contents[0].text_content) > 0) + + prompts = [pyris_system_prompt] + [ + msg for msg in dto.chat_history if is_valid_message(msg) + ]🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
92-92: Simplify response validation condition.The current condition is complex and hard to read. Consider breaking it down into a helper function.
+ def is_empty_response(response: PyrisMessage) -> bool: + return (response.contents is None or + len(response.contents) == 0 or + response.contents[0].text_content is None or + len(response.contents[0].text_content) == 0) + - if response.contents is None or len(response.contents) == 0 or response.contents[0].text_content is None or len(response.contents[0].text_content) == 0: + if is_empty_response(response):🧰 Tools
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/llm/external/openai_chat.py(1 hunks)app/pipeline/chat_gpt_wrapper_pipeline.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/pipeline/chat_gpt_wrapper_pipeline.py
1-1: json imported but unused
Remove unused import: json
(F401)
🪛 GitHub Actions: Run linters
app/pipeline/chat_gpt_wrapper_pipeline.py
[warning] File needs to be reformatted using Black code formatter
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (1)
app/pipeline/chat_gpt_wrapper_pipeline.py (1)
1-1: Remove unused import.The
jsonmodule is imported but never used in this file.-import json🧰 Tools
🪛 Ruff (0.8.2)
1-1:
jsonimported but unusedRemove unused import:
json(F401)
🪛 GitHub Actions: Run linters
[warning] File needs to be reformatted using Black code formatter
|
@coderabbitai ignore |
Only to be added temporarily as an experiment.
Summary by CodeRabbit
Documentation
llm-configtollm_configNew Features
Improvements