-
Couldn't load subscription status.
- Fork 23
Implemented OpenRouter API key usage with any model, set Gemini 2.5 Flash default, created natural-language agent to parse chat. #33
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?
Conversation
|
After the most recent commit, everything is working smoothly and the default mode is implemented properly throughout the codebase. I am able to switch easily between heatseek and collections, and the collection summary is only fetched once. All lingering issues with the distinction between OpenAI and OpenRouter API keys have been resolved. |
|
|
||
| # Get details on the current collection | ||
| print("Getting details on", chat_state.collection_name) | ||
| if chat_state.collection_name not in coll_summary_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.
coll_summary_query is initialized as empty above. More importantly, why waste time and money summarizing when we haven't yet checked if the query already starts with a command string?
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.
It should only check for the summary once for each collection and store it in the coll_summary_query dictionary. Whether it starts with a command string or not, each collection should be summarized at least once.
I have modified the code so it only initializes coll_summary_query if it doesn't exist here: 11d1441
agents/command_chooser.py
Outdated
| if chat_state.collection_name not in coll_summary_query: | ||
| coll_summary_query[chat_state.collection_name] = "" | ||
| summary_prompt = "/kb Can you summarize in one sentence the contents of the current collection?" | ||
| summary_llm = get_llm(chat_state.bot_settings, chat_state,chat_state.openrouter_api_key,embeddings_needed=False) |
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.
Ideally, the agent shouldn't waste resources on this unless it's the first time in the conversation when it needs to know what the collection is about. Most of the time the description could be cached.
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 think every collection should be summarized at least once, I think it's essential for the new natural language parsing feature of this app. It can't decide which command to use if it doesn't know anything about the collection. I don't think it's a waste of resources to have that summary stored in the variable. When you say cached, do you think this variable works as a cache or is something more involved necessary?
| else: | ||
| research_params.num_iterations_left = 1 |
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 else clause makes no sense
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.
research_params was sometimes not getting initialized, and it was looking for num_iterations_left and causing an exception. This ensures that this variable is set even if research_params doesn't get initialized for some reason.
| return ChatResponseData( | ||
| content="Apologies, you can customize your model settings (e.g. model name, " | ||
| "temperature) only when using your own OpenAI API key." | ||
| "temperature) only when using your own OpenRouter API key." |
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.
Way more people have an openai key than an openrouter key, so if I understand correctly this is a regression. I thought we would allow someone to provide just an openai key and use the app fully, with any openai model. And if they provide an openrouter key then they can use any model.
But it looks like the openrouter key is now king - the user id is tied to it.
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.
In that case, I completely misunderstood your intent. I thought the idea was to make gemini 2.5 flash the default model via openrouter, phase out openai models, and use openrouter for any other model. I thought the only reason to retain openai functionality was because you can't use embeddings models via openrouter. Would you like me to change this and allow any openai model if an openai key is provided?
…ore into dev-command-chooser
Co-authored-by: Dmitriy Vasilyuk <[email protected]>
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.
In that case, I completely misunderstood your intent. I thought the idea was to make gemini 2.5 flash the default model via openrouter, phase out openai models, and use openrouter for any other model. I thought the only reason to retain openai functionality was because you can't use embeddings models via openrouter. Would you like me to change this and allow any openai model if an openai key is provided?
Yes, otherwise it would be a big regression in UX. That would prevent the necessity for the user to provide two separate keys, which the majority of users would find unnecessarily burdensome. Since an OpenAI key is required in any case, it should continue being "king" (the user id should be tied to it) and the OpenRouter key can just be optional.
Many changes have been made.
Regarding OpenRouter, the embeddings model still requires OpenAI, so I had to implement both API keys, with the possibility for the user to provide their own for either one.
I implemented logic including an
embeddings_neededBoolean parameter to determine whether a command requires the embeddings model or not, and the function will provide the correct API key to the langchain function call based on the nature of the command.I edited the Streamlit UI so that you can enter an OpenRouter key as well as a custom model.
I set the default model to
google/gemini-2.5-flashand tested with it, it was able to complete all DDG functions. I also tried with an alternate model from Mistral.I created the command chooser utility in
command_chooser.pywith the functionget_raw_commandthat accurately determines the proper command to use given any user input. It is quite robust and selected an appropriate command with a variety of different scenarios. It uses the/kbcommand to check the topic of the selected collection before choosing a command so it knows whether to create a new collection or not.I tested thoroughly with many different prompts of differing topics and specificity.
I did encounter a bug near the end that appears to be unrelated to any of the changes I made, after running
/re hs 4:If you would like to employ my services to troubleshoot this bug as well I would be glad to assist.
Since this PR encompasses the content of the previous PR, I will close that one and leave a single PR for your approval. Thank you!