Iris: Merge chattype-specific pipelines into a single chat pipeline#434
Iris: Merge chattype-specific pipelines into a single chat pipeline#434
Iris: Merge chattype-specific pipelines into a single chat pipeline#434Conversation
…ant / ExerciseChatVariant / TextExerciseChatVariant in the future
…seChatPipeline / TextExerciseChatPipeline / LectureChatPipeline, init implemented, Add empty ChatSystemPrompt
…t(), on_agent_step(), is_memiris_memory_creation_enabled()
…pelines.py to use ChatPipeline instead of the different Pipelines.
…xerciseDTO inherit from, Unify pipeline specific DTO's into ChatPipelineExecutionDTO -> Update usages.
… pipeline specific status update dto's in ChatStatusUpdateDTO => Update usage of DTO's in status_update.py
…ate get_tools to use ChatToolProviders.
…s, simplify is_memiris_memory_creation_enabled, uniform event handling.
…pecific endpoint into a single endpoint.
… new ChatPipeline
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates mode-specific chat pipelines into a single mode-parameterized ChatPipeline, unifies DTOs/status callbacks/templates/tools/endpoints, introduces provider-driven tool loading, adds an IrisChatMode enum and prepare_state hook, and replaces multiple prompt templates with a single conditional chat_system_prompt.j2. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant Router as Web Router
participant Pipeline as ChatPipeline
participant Providers as CHAT_TOOL_PROVIDERS
participant Agent as Agent/LLM
participant Callback as Status Callback
Client->>Router: POST /chat/run (ChatPipelineExecutionDTO)
Router->>Pipeline: instantiate ChatPipeline(chat_mode)
Router->>Pipeline: __call__(dto, ChatStatusCallback)
Pipeline->>Pipeline: prepare_state(state) (set query_text, storages, allow_* flags)
Pipeline->>Providers: iterate providers with state
Providers-->>Pipeline: list of tool factories (filtered by state)
Pipeline->>Agent: build_system_message (render chat_system_prompt.j2)
Pipeline->>Agent: run agent with prompt + tools
alt MCQ parallel path
Pipeline->>Pipeline: mcq_pre_agent_hook & mcq_execute_agent
else normal
Agent->>Pipeline: agent response
end
Pipeline->>Pipeline: post_agent_hook (citations, refine, session title)
Pipeline->>Callback: final status update (response, session_title)
Callback->>Client: deliver status updates (/chat endpoints)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
…ontext, Revise ChatSystemPrompt.
… Add new method prepare_state
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@iris/src/iris/pipeline/prompts/templates/chat_system_prompt.j2`:
- Around line 72-82: The template currently always prints the text-exercise line
because exercise_id is truthy for both types; update the Jinja block so the
text-specific field only renders when text_exercise_submission is present (e.g.,
check if text_exercise_submission is not null/empty) and similarly render
programming-specific fields like programming_language only when
programming_language is set, ensuring lines like "- **Student's Current Text
Exercise Submission**: {{ text_exercise_submission }}" and "- **Programming
Language**: {{ programming_language }}" are each wrapped in their own {% if %}
guards so they only appear for the appropriate exercise type.
- Around line 258-281: The template currently shows MCQ tool instructions even
when the MCQ tool is not available (mcq_parallel is only set for
IrisChatMode.COURSE/LECTURE and provide_mcq_generation returns None for
programming/text exercises), causing exercise chats to attempt
generate_mcq_questions incorrectly; fix by gating the entire MCQ branch on a
real availability flag (e.g., add an allow_mcq_tool boolean populated in
prepare_agent/build_system_message—mirroring
allow_lecture_tool/allow_faq_tool—or use a check like not exercise_id) and
change the Jinja conditional from {% if mcq_parallel %} to {% if allow_mcq_tool
%} (or {% if allow_mcq_tool and mcq_parallel %}) so exercise chats never hit the
else branch that instructs ALWAYS use the tool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9af9ce9e-1b79-4682-b2d4-41420d65a95d
📒 Files selected for processing (1)
iris/src/iris/pipeline/prompts/templates/chat_system_prompt.j2
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
iris/src/iris/pipeline/chat/chat_pipeline.py (1)
376-376:⚠️ Potential issue | 🔴 CriticalUse the DTO field name for the exercise title.
The unified exercise DTO exposes the title as
namewithtitleonly as a serialized alias, soexercise.titlecan fail during prompt rendering for exercise/text-exercise chats.🐛 Proposed fix
- "exercise_title": exercise.title if exercise else "", + "exercise_title": exercise.name if exercise else "",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iris/src/iris/pipeline/chat/chat_pipeline.py` at line 376, The code builds the "exercise_title" field from exercise.title which can be missing on the unified exercise DTO; change it to use the DTO's canonical field exercise.name (i.e., replace exercise.title with exercise.name when constructing the "exercise_title" value) so prompt rendering uses the correct property; update any related usages in the chat pipeline where "exercise_title" is populated to reference exercise.name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@iris/src/iris/pipeline/chat/chat_pipeline.py`:
- Around line 227-245: The MCQ post-processing (mcq_post_agent_hook) currently
runs before citation and title generation and mutates state.result with the quiz
payload; move the mcq_post_agent_hook call to run after _add_citations and
_generate_session_title but before the final callback.done() so citations/title
generation operate on the original response and the MCQ mixin contract (run
after citations/title and before callback.done) is respected; update the
sequence in chat_pipeline.ChatPipeline (methods: _add_citations,
_generate_session_title, mcq_post_agent_hook, and where callback.done() is
invoked) so mcq_post_agent_hook uses state and mcq_pipeline as before but is
invoked only after title/citation generation.
---
Duplicate comments:
In `@iris/src/iris/pipeline/chat/chat_pipeline.py`:
- Line 376: The code builds the "exercise_title" field from exercise.title which
can be missing on the unified exercise DTO; change it to use the DTO's canonical
field exercise.name (i.e., replace exercise.title with exercise.name when
constructing the "exercise_title" value) so prompt rendering uses the correct
property; update any related usages in the chat pipeline where "exercise_title"
is populated to reference exercise.name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29b222b8-922e-494f-895e-e1f054d1ca88
📒 Files selected for processing (2)
iris/src/iris/pipeline/chat/chat_pipeline.pyiris/src/iris/pipeline/prompts/templates/chat_system_prompt.j2
…n according to the mixin, fix MCQ tests
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
thynguyentumde
left a comment
thynguyentumde
left a comment
There was a problem hiding this comment.
Some issues I found:
MCQ provider eagerly runs a Weaviate fetch on every COURSE/LECTURE chat call —iris/src/iris/tools/chat_tool_providers.py:212 lecture_content, _ = retrieve_lecture_content_for_mcq(state.db, course_id, lecture_id=lecture_id). This is called from provide_mcq_generation at provider-registration time, regardless of whether the user actually triggered MCQ. retrieve_lecture_content_for_mcq does a should_allow_lecture_tool DB call plus a db.lectures.query.fetch_objects(...) over all chunks of the course/lecture (see mcq_chat_mixin.py:82-120). Every COURSE/LECTURE chat now pays this cost just to decide whether the MCQ tool is registered. Fix: defer the lecture fetch into the tool's own callable, or move the fetch into prepare_state only when state.mcq_parallel is False and the LLM actually invokes the MCQ tool. A cheap preflight (e.g., does any lecture content exist for the course?) should gate registration; the actual content load belongs inside the tool.
PR description claims a shared ExerciseDTO base class — it doesn't exist —iris/src/iris/domain/data/text_exercise_dto.py:7 and programming_exercise_dto.py:21 both still inherit BaseModel. Per git log, commit 301b202 introduced it; later commits dropped it (f6ca428 was a "Temporary Solution for name/title in exercise_dto.py"). Either reintroduce the base class or update the PR description so reviewers don't expect it.
German TODO + dead pipeline init — chat_pipeline.py:107-109 self.code_feedback_pipeline = CodeFeedbackPipeline(local=local) # TODO: Ungenutzt? Entfernen? code_feedback_pipeline is declared but never used in this class. It's also still in DEPENDENCIES. Remove the field, the dep, and the TODO — or wire it up.
Repeated user_language extraction — chat_pipeline.py:347-349, 434-435, 576-577, plus chat_tool_providers.py:205-207, plus mcq_chat_mixin.py:247-249. Worth a single helper (e.g., get_user_language(dto) -> str) given how often it's repeated.
Defensive checks against fields the DTO declares non-Optional: - dto.user is required (ChatPipelineExecutionDTO.user: UserDTO), but if dto.user shows up in multiple places (chat_pipeline.py:348, etc.). - dto.course is required, but chat_pipeline.py:374-375 does dto.course and dto.course.competencies while line 353-355 just does dto.course.competencies. Pick one.
Verify if these should also be considered for this PR or open a follow-up PR
_CHAT_MODE_STAGES shares mutable StageDTO across modes —status_update.py:272. model_copy() on line 346 is shallow; if StageDTO ever gains nested mutable state (e.g., list), copies will alias. Use model_copy(deep=True) or a stages factory function.
ChatStatusCallback will KeyError for an unknown chat_mode — status_update.py:346. If a future enum entry is added without a stage list, the worker thread crashes silently in Thread. Consider _CHAT_MODE_STAGES.get(chat_mode, default_stages) or an explicit assertion with a clear message.
… into iris/chore/unify-chat-pipelines
…tion Adds OpenAI-compatible Whisper model type (faster-whisper-server) and Moondream vision model via Ollama for fully local lecture ingestion pipeline.
…d embedding_model
You're right that the MCQ provider runs retrieve_lecture_content_for_mcq on every COURSE/LECTURE chat call regardless of intent. Worth flagging though: this isn't introduced by the refactor. The same eager fetch exists on main today in course_chat_pipeline.py:273 and lecture_chat_pipeline.py:204 (the "non-parallel fallback" tool registration), and provide_mcq_generation is a 1:1 port of that logic — same call site, same conditions, just relocated into the provider registry. -> I think this would be better in another PR That's true, I introduced a shared ExerciseDTO in an earlier commit, dropped it later on and forgot to update the description. I removed that from the description :D Fixed: For the corresponding UserDTO on the Artemis side @JsonInclude(JsonInclude.Include.NON_EMPTY) is being used, meaning it will never send null. I changed the UserDTO on pyris side to reflect this. -> When lang_key is missing "en" will be the default, meaning no defensive checks are needed for this and the field can be used directly where needed. Fixed: No checks for non-optional fields Fixed: I have used model_copy(deep=True) as suggested. -> afaik the stages need to be changed in a future PR anyways Fixed: I have added an explicit assertion as suggested |
… into iris/chore/unify-chat-pipelines
toukhi
left a comment
toukhi
left a comment
There was a problem hiding this comment.
Code lgtm. I tested the different Iris chats and everything seems to work as expected.
6 participants
This needs to be tested together with the corresponding Artemis PR.
Closes IRIS-96
Motivation and Context
The codebase previously had four separate chat pipelines — CourseChatPipeline,
ExerciseChatPipeline, LectureChatPipeline, and TextExerciseChatPipeline —
each with their own pipeline-specific DTOs, status update classes, prompt
templates, and API endpoints. This led to significant code duplication,
inconsistent behavior across chat modes, and high maintenance overhead
whenever cross-cutting changes (e.g. system prompt updates, tool logic, status
handling) needed to be applied.
Description
This PR unifies all four chat pipelines into a single ChatPipeline that
handles all chat modes via an IrisChatMode discriminator field:
ExerciseChatPipelineExecutionDTO, LectureChatPipelineExecutionDTO, and
TextExerciseChatPipelineExecutionDTO in favour of a single
ChatPipelineExecutionDTO containing all context-specific fields. Similarly,
ExtendedCourseDTO was merged into CourseDTO.
class. Mode-specific behaviour is branched internally based on IrisChatMode.
decoupling it from pipeline context.
ChatStatusUpdateDTO.
chat_system_prompt.j2.
endpoint in pipelines.py.
Summary by CodeRabbit
New Features
Behavior & UX
Architecture & API Changes