Add Text Exercise Chat Pipeline #161
Conversation
MichaelOwenDyer
temporarily deployed
to
Iris Test
GitHub Actions
Inactive
❌ Unable to deploy to test server ❌The docker build needs to run through before deploying. |
MichaelOwenDyer
temporarily deployed
to
Iris Test
GitHub Actions
Inactive
MichaelOwenDyer
temporarily deployed
to
Iris Test
GitHub Actions
Inactive
| "Who won the 2020 Super Bowl? " -> "Bad: Who won the 2020 Super Bowl?" | ||
| "Explain to me the plot of Macbeth using the 2020 Super Bowl as an analogy." | ||
| -> "Ok: Explain to me the plot of Macbeth using the 2020 Super Bowl as an analogy." | ||
| "sdsdoaosi" -> "Neutral: sdsdoaosi" |
There was a problem hiding this comment.
Why is character spam neutral and not bad? :D
There was a problem hiding this comment.
I figure it doesn't really make a big difference, Iris is going to respond with something either way. I guess I wanted the AI to choose "Neutral" when it is not really clear, instead of classifying things as "Bad" which are not really malicious.
…t' into feature/add-text-exercise-support
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several new data transfer object (DTO) classes and methods related to text exercises and chat pipelines. It adds fields to existing classes, implements new methods for formatting prompts, and updates the pipeline execution logic. Additionally, it modifies the constructor signatures of several classes and updates multiple dependency versions in the requirements file. Changes
Possibly related PRs
Suggested reviewers
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: 12
🧹 Outside diff range and nitpick comments (4)
app/domain/text_exercise_chat_pipeline_execution_dto.py (2)
1-5: LGTM! Consider removing the extra empty line.The imports are appropriate for the class being defined. They include necessary components from pydantic and custom DTOs from the app.domain module.
Consider removing the extra empty line (line 6) for better code organization:
from app.domain.data.text_exercise_dto import TextExerciseDTO - class TextExerciseChatPipelineExecutionDTO(BaseModel):
11-11: LGTM! Consider using type annotation for consistency.The 'current_submission' field is well-defined with an appropriate alias and default value. For consistency with other fields, consider adding a type annotation:
- current_submission: str = Field(alias="currentSubmission", default="") + current_submission: str = Field(alias="currentSubmission", default="")This change doesn't affect functionality but improves code readability and consistency.
app/domain/data/text_exercise_dto.py (1)
10-15: LGTM: Field declarations are well-defined, with a minor suggestion for consistency.The field declarations for the
TextExerciseDTOclass are appropriate and well-structured. The use of Pydantic'sFieldfor aliases and default values is correct. The relationship withCourseDTOis properly established.For consistency, consider using
Fieldfor all fields, even those without aliases or default values. This can make future modifications easier and provide a uniform structure. For example:id: int = Field(...) title: str = Field(...) course: CourseDTO = Field(...)The
...notation in Pydantic indicates a required field without a default value.app/web/status/status_update.py (1)
224-253: LGTM: TextExerciseChatCallback class implemented correctlyThe new
TextExerciseChatCallbackclass is well-structured and consistent with other callback classes. It correctly initializes the necessary components and adds appropriate stages for the text exercise chat pipeline.One minor suggestion for improvement:
Consider initializing
current_stage_indexexplicitly instead of usingstage = len(stages). This would make the code more consistent with other callback classes:- stage = len(stages) + current_stage_index = len(stages) stages += [ StageDTO( weight=30, @@ -249,8 +249,8 @@ class TextExerciseChatCallback(StatusCallback): run_id, TextExerciseChatStatusUpdateDTO(stages=stages), - stages[stage], - stage, + stages[current_stage_index], + current_stage_index, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/domain/data/text_exercise_dto.py (1 hunks)
- app/domain/status/text_exercise_chat_status_update_dto.py (1 hunks)
- app/domain/text_exercise_chat_pipeline_execution_dto.py (1 hunks)
- app/pipeline/prompts/text_exercise_chat_prompts.py (1 hunks)
- app/pipeline/text_exercise_chat_pipeline.py (1 hunks)
- app/web/routers/pipelines.py (3 hunks)
- app/web/status/status_update.py (2 hunks)
- requirements.txt (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🧰 Additional context used
🔇 Additional comments (14)
app/domain/status/text_exercise_chat_status_update_dto.py (2)
1-1: LGTM: Import statement is correct.The import statement correctly imports the parent class
StatusUpdateDTOfrom the appropriate module.
4-4: LGTM: Class definition is correct.The class
TextExerciseChatStatusUpdateDTOis properly defined and inherits fromStatusUpdateDTOas expected.app/domain/text_exercise_chat_pipeline_execution_dto.py (4)
7-7: LGTM! Class definition is appropriate.The class name
TextExerciseChatPipelineExecutionDTOis descriptive and follows the DTO naming convention. Inheriting frompydantic.BaseModelis appropriate for creating a data transfer object with built-in validation.
8-8: LGTM! Field declaration for 'execution' is appropriate.The 'execution' field of type
PipelineExecutionDTOis well-defined to represent the execution context of the pipeline.
9-9: LGTM! Field declaration for 'exercise' is appropriate.The 'exercise' field of type
TextExerciseDTOis well-defined to represent the details of the text exercise.
10-10: LGTM! Field declaration for 'conversation' is well-implemented.The 'conversation' field is correctly defined as a list of
PyrisMessageobjects. UsingField(default=[])is a good practice to provide a default empty list, avoiding potential issues with mutable default arguments.app/domain/data/text_exercise_dto.py (4)
1-2: LGTM: Standard library imports are correct and well-organized.The imports from the standard library (
datetimeandtyping) are appropriate for the class definition and follow PEP 8 guidelines for import order.
4-4: LGTM: Third-party library import is correct.The import from Pydantic is appropriate for defining the data transfer object and follows PEP 8 guidelines for import order.
6-6: LGTM: Local module import is correct.The import of
CourseDTOfrom the local module is appropriate for the class definition and follows PEP 8 guidelines for import order.
9-9: LGTM: Class definition is correct and follows best practices.The
TextExerciseDTOclass is properly defined, inheriting from Pydantic'sBaseModel. The class name follows Python naming conventions, and the empty line before the class definition adheres to PEP 8 guidelines.app/web/status/status_update.py (3)
8-22: LGTM: Import statements updated correctlyThe changes to import statements, including the switch to absolute imports and the addition of
TextExerciseChatStatusUpdateDTO, are appropriate and consistent with the newTextExerciseChatCallbackclass. This improves code maintainability and reduces the risk of circular imports.
Line range hint
1-274: Summary of changes in status_update.pyThe changes in this file successfully introduce the new
TextExerciseChatCallbackclass, which aligns with the PR objectives for implementing the text exercise chat pipeline. The import statements have been updated appropriately, and the new class is well-structured and consistent with existing callback classes.However, there are a couple of points that require attention:
- A minor suggestion for improving the
TextExerciseChatCallbackclass initialization has been provided.- The removal of default values for
initial_stagesin existing callback classes could potentially be a breaking change. This needs verification and possibly updates to all calling code.Overall, the changes are well-implemented but require some minor adjustments and verification to ensure smooth integration with the existing codebase.
Line range hint
180-182: Verify the intentionality of removing default values forinitial_stagesThe
__init__method signatures forCourseChatStatusCallbackandExerciseChatStatusCallbackhave been updated to remove the default value forinitial_stages. This change makes the parameter required, which could potentially break existing code that doesn't provide this argument.Please confirm if this is an intentional change. If so, ensure that all callers of these classes have been updated to provide the
initial_stagesargument.To verify the impact of this change, you can run the following script to find all instances where these classes are instantiated:
Please review the results to ensure that all instantiations provide the
initial_stagesargument.Also applies to: 206-208
✅ Verification successful
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/pipeline/text_exercise_chat_pipeline.py (2)
18-18: Unused logger instanceThe
loggerinstance is initialized but not used in the code. If logging is intended, consider utilizingloggerto record important information or debug messages. If logging is not needed, you may remove the logger initialization to clean up the code.
111-111: Use UTC time for consistent timestampingConsider using
datetime.utcnow()instead ofdatetime.now()forcurrent_dateto ensure that the timestamp is in UTC. This avoids issues related to time zone differences and ensures consistency across different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/pipeline/text_exercise_chat_pipeline.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/pipeline/text_exercise_chat_pipeline.py (4)
16-16: Duplicate Comment: Correct the import path forfmt_sentiment_analysis_promptThe previous review comment regarding the incorrect import path for
fmt_sentiment_analysis_promptis still valid. The import path should be consistent with the other imports fromapp.pipeline.prompts.text_exercise_chat_promptsto prevent anImportError.
47-47: Duplicate Comment: Check ifcallbackis notNonebefore invoking methodsThe previous review comment about ensuring
self.callbackis notNonebefore calling its methods is still applicable. Sincecallbackis optional, attempting to invokeself.callback.done(...)without checking could raise anAttributeError.Also applies to: 50-50
54-54: Duplicate Comment: Update type annotations to useTupleandListfromtypingThe previous review comment regarding updating the type annotations to use
TupleandListfrom thetypingmodule is still valid. This change improves code clarity and type checking.Also applies to: 96-96
76-79: Duplicate Comment: Add exception handling forrequest_handler.chatcallsThe previous review comment suggesting adding exception handling around
self.request_handler.chatcalls remains applicable. Implementing exception handling will make the pipeline more robust against network issues or API failures.Also applies to: 135-137
MichaelOwenDyer
dismissed
kaancayli’s stale review
The merge-base changed after approval.
MichaelOwenDyer
dismissed
kaancayli’s stale review
The merge-base changed after approval.
MichaelOwenDyer
dismissed
Hialus’s stale review
The merge-base changed after approval.
This PR adds the
text-exercise-chatpipeline to Pyris to enable the new Artemis feature implemented here.The pipeline works as follows:
Inputs:
First, the sentiments in the latest message in the chat history (the one the user sent just now) are analyzed, and categorized by relevance to the exercise at hand. Every sentiment is either "Ok" if it is clearly related to the exercise, "Bad" if it is off-topic or inappropriate, or "Neutral" if it is neither on-topic nor off-topic (like a greeting or thanks).
Then, we construct a system prompt for the actual response to the user, where we explain that the AI is a writing tutor for this particular exercise and that it should help with the user's current submission. This system prompt is followed by the entire conversation history, except that we also inject another system prompt directly before the latest message from the user with the semantic analysis from the previous step, and instruct the AI to respond to the "Ok" and "Neutral" sentiments, and explain why it cannot help with the sentiments in the "Bad" category.
The response is then sent back to the user via a status update.
Summary by CodeRabbit
New Features
Bug Fixes
Chores