-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
feat: Context Template from settings for QueryDocs Allowing the text … #1398
base: main
Are you sure you want to change the base?
Changes from 4 commits
47b3713
bbb9d2c
cfbba71
4c51eeb
f06a8b4
7463efc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
) | ||
from private_gpt.open_ai.extensions.context_filter import ContextFilter | ||
from private_gpt.server.chunks.chunks_service import Chunk | ||
from private_gpt.settings.settings import settings | ||
|
||
|
||
class Completion(BaseModel): | ||
|
@@ -97,8 +98,11 @@ def _chat_engine( | |
system_prompt: str | None = None, | ||
use_context: bool = False, | ||
context_filter: ContextFilter | None = None, | ||
context_template: str | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we are not making use of this context_template argument anywhere, I'd suggest to remove it for the moment. |
||
) -> BaseChatEngine: | ||
if use_context: | ||
if context_template is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this, why not checking if settings.rag.default_cotnext_template is not None and then apply it? |
||
context_template = settings().rag.default_context_template | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should avoid calling settings() directly, and instead inject settings as a dependency in the ChatService constructor. Check how settings are injected in EmbeddingComponent or LLMComponent for example. |
||
vector_index_retriever = self.vector_store_component.get_retriever( | ||
index=self.index, context_filter=context_filter | ||
) | ||
|
@@ -109,6 +113,7 @@ def _chat_engine( | |
node_postprocessors=[ | ||
MetadataReplacementPostProcessor(target_metadata_key="window"), | ||
], | ||
context_template=context_template, | ||
) | ||
else: | ||
return SimpleChatEngine.from_defaults( | ||
|
@@ -121,6 +126,7 @@ def stream_chat( | |
messages: list[ChatMessage], | ||
use_context: bool = False, | ||
context_filter: ContextFilter | None = None, | ||
context_template: str | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we are not making use of this context_template argument anywhere, I'd suggest to remove it for the moment. |
||
) -> CompletionGen: | ||
chat_engine_input = ChatEngineInput.from_messages(messages) | ||
last_message = ( | ||
|
@@ -141,6 +147,7 @@ def stream_chat( | |
system_prompt=system_prompt, | ||
use_context=use_context, | ||
context_filter=context_filter, | ||
context_template=context_template, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we are not making use of this context_template argument anywhere, I'd suggest to remove it for the moment. |
||
) | ||
streaming_response = chat_engine.stream_chat( | ||
message=last_message if last_message is not None else "", | ||
|
@@ -157,6 +164,7 @@ def chat( | |
messages: list[ChatMessage], | ||
use_context: bool = False, | ||
context_filter: ContextFilter | None = None, | ||
context_template: str | None = None, | ||
) -> Completion: | ||
chat_engine_input = ChatEngineInput.from_messages(messages) | ||
last_message = ( | ||
|
@@ -177,6 +185,7 @@ def chat( | |
system_prompt=system_prompt, | ||
use_context=use_context, | ||
context_filter=context_filter, | ||
context_template=context_template, | ||
) | ||
wrapped_response = chat_engine.chat( | ||
message=last_message if last_message is not None else "", | ||
|
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.
Were is set (at usage) this new
context_template
?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.
I am not sure if I understand.
If "query docs" is being used, it creates a ContextChatEngine (from llama_index) that accepts this parameter. Currently it's never passed, so it defaults to a hardcoded value. With this change, if there is a value defined in the settings file, the value in settings will be used instead. This should work both from the Gradio UI or from the API.
As per the Discord discussion, I didn't expose this configuration to the API