-
Notifications
You must be signed in to change notification settings - Fork 126
Add instruction query generator #226
Add instruction query generator #226
Conversation
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.
@izellevy please see a few suggestions
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.
LGTM overall! left some comments for your decision, and some for a discussion
|
||
class CondensedQueryGenerator(QueryGenerator): | ||
_DEFAULT_COMPONENTS = { | ||
"llm": AnyscaleLLM, |
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.
Not sure if it not make more sense to use OpenAI also here as default. It seems more consistent for me, especially when we add more LLMs
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 also think so but @igiloh-pinecone had a comment on this. I agree that our default should be OpenAI
pass | ||
|
||
|
||
class CondensedQueryGenerator(QueryGenerator): |
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'm not sure condensed describes well enough what this class is doing. How about InstructionQueryGenerator
? I think instruction prompting is the most common terminology for this type of prompt and parsing
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.
@igiloh-pinecone what do you think about the name?
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 tend to agree with @acatav , the name Instruction
or even Prompt
is more common.
The "condensed" naming is a term that LLamaIndex invented, and I haven't seen it used otherwise.
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.
Problem
Currently, we only support LLMs that have function calling functionality for query generation. We need to support LLMs that do not have this functionality.
Solution
Added a new query generator that is able to generate a question based on the chat history without using function calling
Type of Change
Test Plan
Describe specific steps for validating this change.