-
Notifications
You must be signed in to change notification settings - Fork 3
Enhance chat functionality, support conversation (no-RAG) use case #100
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
Conversation
… improved conversation chains
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
Enhance chat functionality by making document collections optional and introducing separate chains for RAG-based and simple chats.
- Make
collectionoptional in the/chatendpoint and adjust routing logic to pick between RAG and simple chains. - Rename the existing
conversation_chaintoconversation_rag_chainand add a newconversation_chainfor non-collection chats. - Update chat history persistence to allow
collection_idto beNoneif no collection is provided.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| brevia/routers/qa_router.py | Made collection optional, added chain-selection logic, updated docstring |
| brevia/query.py | Renamed RAG chain, added a standalone simple conversation_chain |
| brevia/chat_history.py | Allow collection_id to be None and removed error on missing collection |
Comments suppressed due to low confidence (4)
brevia/routers/qa_router.py:61
- The variable
conversation_handleris instantiated but never used; consider passing it to the chain callbacks or removing it to avoid dead code.
conversation_handler = ConversationCallbackHandler()
brevia/query.py:286
- Consider adding unit tests to cover the new
conversation_chainfunction to verify its output formatting and behavior without a collection.
def conversation_chain(
brevia/chat_history.py:119
- Removing the error when a provided collection isn't found may lead to silent failures; consider reintroducing error handling or at least logging a warning when
CollectionStore.get_by_namereturnsNone.
if collection:
brevia/query.py:5
- Double-check that all symbols used in the new
conversation_chain(e.g.,get_settings,StrOutputParser,RunnablePassthrough,load_condense_prompt) are imported; missing imports will cause runtime errors.
from langchain_core.output_parsers import JsonOutputParser
…s for invalid cases
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
Enhances chat functionality by introducing optional collection support, a new RAG-based chain, and a simple test mode; also updates history handling to allow missing collections without errors.
- Add
conversation_rag_chainalongside a streamlinedconversation_chainfor test mode - Update
/chatendpoint to select between RAG and test chains via a newmodefield - Change
add_historyto accept non-existent collections and adjust related tests
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_query.py | Added tests for simple and RAG conversation chains and output format |
| tests/test_chat_history.py | Updated test to expect successful history addition with no collection |
| tests/routers/test_qa_router.py | Added tests for invalid mode and missing collection in /chat route |
| brevia/routers/qa_router.py | Introduced mode to ChatBody, routing logic for RAG/test modes |
| brevia/query.py | Refactored conversation_chain and introduced conversation_rag_chain |
| brevia/models.py | Changed LOREM_IPSUM to a JSON-formatted string literal |
| brevia/chat_history.py | Modified add_history to handle missing collections without raising |
Comments suppressed due to low confidence (3)
tests/test_chat_history.py:34
- [nitpick] The test name
test_add_history_failureno longer matches its behavior (it no longer checks for a failure). Consider renaming it to something liketest_add_history_no_collectionfor clarity.
def test_add_history_failure():
brevia/routers/qa_router.py:39
- Using a mutable default value for
chat_historycan lead to shared state across requests. Consider usingField(default_factory=list)to provide a new list per instance.
chat_history: list = []
brevia/models.py:27
- The multiline string for
LOREM_IPSUMincludes an unescaped newline inside the JSON string, which is invalid JSON and may cause parsing errors. Either escape the newline or adjust the format so the JSON remains valid.
"answer": "Lorem ipsum dolor sit amet, consectetur adipisici elit,
brevia/query.py
Outdated
| """ | ||
|
|
||
| # Define your desired data structure. | ||
| class Result(BaseModel): |
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.
can we remove this Result class? seems unused now
This PR enhances chat functionality by introducing optional collection support, a new chain, and a simple conversation mode to chat with a model using history, but without a reference document collections.
Also history handling has been updated to allow missing collections without errors.
conversation_rag_chainalongside a streamlinedconversation_chainfor test mode/chatendpoint to select between RAG and conversation chains via a newmodefieldadd_historyto accept non-existent collections and adjust related testsChanges
Show a summary per file
modetoChatBody, routing logic for RAG/conversation modesconversation_chainand introducedconversation_rag_chainLOREM_IPSUMto a JSON-formatted string literaladd_historyto handle missing collections without raising/chatroute