-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Merge ChatReadRetrieveReadApproach into Approach class #2860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Merge ChatReadRetrieveReadApproach into Approach class #2860
Conversation
Remove the Ask/Q&A tab from the application as it was not widely used and duplicated functionality available in the Chat tab. Changes: - Remove Ask page frontend components (Ask.tsx, Ask.module.css) - Remove RetrieveThenReadApproach backend class - Remove /ask API endpoint - Remove Ask-related tests and snapshots - Update documentation to reflect removal - Update all translation files - Update evaluation configs Fixes Azure-Samples#2834
Deprecate Ask tab
Since the Ask tab was removed, there is now only one approach implementation. This refactor merges ChatReadRetrieveReadApproach into the base Approach class, simplifying the codebase by removing the unnecessary inheritance hierarchy. Changes: - Merge all ChatReadRetrieveReadApproach code into approach.py - Remove ABC base class since there's only one implementation - Delete chatreadretrieveread.py - Update app.py to use Approach directly - Update tests to use new import paths - Update documentation to reflect simplified architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR simplifies the codebase by merging ChatReadRetrieveReadApproach into the base Approach class following the removal of the Ask tab (#2834). Since only one approach implementation remains (the Chat approach), the inheritance hierarchy is no longer necessary. The refactor consolidates all RAG functionality into a single class while maintaining the same behavior.
Key changes:
- Merged
ChatReadRetrieveReadApproachimplementation intoApproachclass - Removed unused Ask-related code (routes, components, prompts, snapshots)
- Updated all imports and references throughout tests and application code
- Simplified architecture documentation and developer guides
Reviewed changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/backend/approaches/approach.py |
Merged chat implementation from ChatReadRetrieveReadApproach; added run(), run_stream(), and retrieval methods; removed ABC inheritance |
app/backend/approaches/chatreadretrieveread.py |
Deleted (merged into Approach) |
app/backend/approaches/retrievethenread.py |
Deleted (Ask approach no longer needed) |
app/backend/approaches/prompts/ask_answer_question.prompty |
Deleted (Ask prompt no longer needed) |
app/backend/app.py |
Removed /ask endpoint; removed CONFIG_ASK_APPROACH; updated to instantiate Approach directly |
app/backend/config.py |
Removed CONFIG_ASK_APPROACH constant |
tests/test_chatapproach.py |
Updated imports to use Approach instead of ChatReadRetrieveReadApproach; fixed constant reference |
tests/test_app.py |
Removed Ask-related tests; updated monkeypatch paths from chatreadretrieveread to approach |
tests/conftest.py |
Updated chat_approach fixture to use Approach class |
tests/e2e.py |
Removed test_ask end-to-end test |
app/frontend/src/pages/ask/Ask.tsx |
Deleted Ask page component |
app/frontend/src/index.tsx |
Removed /qa route |
app/frontend/src/api/api.ts |
Removed askApi function |
app/frontend/src/pages/layout/Layout.tsx |
Simplified header; removed navigation menu |
app/frontend/vite.config.ts |
Removed /ask proxy endpoint |
app/frontend/src/locales/*/translation.json |
Removed "qa" translation key from all language files |
docs/architecture.md |
Updated diagram and descriptions to reflect single approach |
docs/customization.md |
Removed Ask-specific documentation; updated file references |
AGENTS.md |
Updated approach file references |
README.md |
Removed reference to Q&A interface |
evals/evaluate_config.json |
Updated target URL from /ask to /chat |
evals/results*/baseline-ask/* |
Deleted Ask-specific evaluation results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Purpose
Since the Ask tab was removed (#2834), there is now only one approach implementation. This refactor merges
ChatReadRetrieveReadApproachinto the baseApproachclass, simplifying the codebase by removing the unnecessary inheritance hierarchy.Does this introduce a breaking change?
Type of change
What is the current behavior?
Approachclass was an abstract base class (ABC) inapproach.pyChatReadRetrieveReadApproachextended it in a separate filechatreadretrieveread.pyWhat is the new behavior?
Approachclass now contains the full implementation (no longer ABC)chatreadretrieveread.pyhas been deletedapp.pyimports and usesApproachdirectlyOther information
Files changed:
app/backend/approaches/approach.py: Merged all chat-specific code, removed ABCapp/backend/approaches/chatreadretrieveread.py: Deletedapp/backend/app.py: Updated imports to useApproachdirectlytests/conftest.py: Updatedchat_approachfixturetests/test_chatapproach.py: Updated importstests/test_app.py: Updated monkeypatch pathsdocs/architecture.md: Simplified architecture diagram and descriptionsdocs/customization.md: Updated file referencesAGENTS.md: Updated approach file referenceAll 457 tests pass ✅