-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add RetrieveChat #1158
Add RetrieveChat #1158
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.
I reviewed all except the notebook. The change to the notebook can be made in the next round.
Overall, the application looks interesting and general. The implementation is creative. It's worth considering writing a blog post about it.
flaml/autogen/retrieval_utils.py
Outdated
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.
nice. could be useful for other applications too.
@QingYun
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.
Could you add a test?
Could you review #1165 ? It provides some utility for this PR.
if "exitcode: 0 (execution succeeded)" in message.get("content", ""): | ||
return "TERMINATE" |
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.
Sometimes even when the execution succeeds, the task is still not finished.
The result might indicate logic error or requires further steps.
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.
Although sometimes even when the code execution succeeds, the task is not solved, but it's hard to tell. If the human_input_mode of RetrieveUserProxyAgent is "TERMINATE" or "ALWAYS", user can still continue the conversation.
# return exitcode, log, None | ||
|
||
result = self._ipython.run_cell(code) | ||
log = str(result.result) |
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.
The problem with this is that the result.result is often None because the stdout + stderr are not part of the result.result. This is true for the example you had in the notebook.
If capture doesn't work, then we need to find some other way to get the stdout + stderr.
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 whether we should capture all of the stdout and stderr outputs and include them in the message. In the case of FLAML, the training logs can often be too long to fit into the LLM tokens. The result.result can only capture concise error information, which may not be sufficient for debugging problems. Therefore, I believe that for complex problems, it's essential to include human input as well.
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.
We can work on this in future PR.
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.
Most comments are for changes required in future PR. A few comments need to be addressed in this PR. If it's unclear which are required for this PR, please let me know.
self.register_auto_reply(Agent, RetrieveUserProxyAgent._generate_retrieve_user_reply) | ||
|
||
@staticmethod | ||
def get_max_tokens(model="gpt-3.5-turbo"): |
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.
This method shouldn't be in this class. Move it to oai.openai_utils? Can be done in next PR.
self._client = self._retrieve_config.get("client", chromadb.Client()) | ||
self._docs_path = self._retrieve_config.get("docs_path", "./docs") | ||
self._collection_name = self._retrieve_config.get("collection_name", "flaml-docs") | ||
self._model = self._retrieve_config.get("model", "gpt-4") |
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.
This info is supposed to be in the sender's llm_config, not here. Refactoring will be needed later.
self._docs_path = self._retrieve_config.get("docs_path", "./docs") | ||
self._collection_name = self._retrieve_config.get("collection_name", "flaml-docs") | ||
self._model = self._retrieve_config.get("model", "gpt-4") | ||
self._max_tokens = self.get_max_tokens(self._model) |
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.
As discussed in #1160 , where to get the token limit info may change in future.
self._doc_idx = -1 # the index of the current used doc | ||
self._results = {} # the results of the current query |
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.
This assumes a single agent to work with. Different agents will need different context. Need to change later.
self._ipython = get_ipython() | ||
self._doc_idx = -1 # the index of the current used doc | ||
self._results = {} # the results of the current query | ||
self.register_auto_reply(Agent, RetrieveUserProxyAgent._generate_retrieve_user_reply) |
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.
A more recommended way is to register the context needed by this function through context
instead of class variables. Can refactor later.
# return exitcode, log, None | ||
|
||
result = self._ipython.run_cell(code) | ||
log = str(result.result) |
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.
We can work on this in future PR.
TEXT_FORMATS = ["txt", "json", "csv", "tsv", "md", "html", "htm", "rtf", "rst", "jsonl", "log", "xml", "yaml", "yml"] | ||
|
||
|
||
def num_tokens_from_text( |
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.
oai.openai_utils is a better fit? Can change later.
For future PRs that requires OpenAI test, please use a branch in microsoft/FLAML |
Since the notebook is very long, can you add a table of contents for the examples at the beginning? That is to show what the example demonstrates and unique features of retrieve chat, and the user can go to the corresponding examples. For example, Example 5: we demonstrate the unique feature "UPDATE CONTEXT"... |
Thank you @kevin666aa for the suggestion. I've added the ToC, it works in VSCode and local jupyter server, but doesn't work as expected in GitHub preview. Looks like github has limited preview support on the links. |
We can just put the contents without the links. I think people can easily go to the example by searching the keywords. I have no more questions. |
Why are these changes needed?
RetrieveChat is a convesational framework for retrieve augmented code generation and question answering. Essentially,
RetrieveAssistantAgent
andRetrieveUserProxyAgent
implements a different auto reply mechanism corresponding to the RetrieveChat prompts.Related issue number
Closes #1107
Checks