Skip to content
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

Show current LLM model used #2001

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

FadhlanR
Copy link
Contributor

@FadhlanR FadhlanR commented Jan 6, 2025

Screenshot 2025-01-08 at 14 52 26

Copy link

github-actions bot commented Jan 6, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   20m 42s ⏱️ +17s
726 tests +1  724 ✔️ +1  2 💤 ±0  0 ±0 
731 runs  +1  729 ✔️ +1  2 💤 ±0  0 ±0 

Results for commit 7421df3. ± Comparison against base commit b650668.

♻️ This comment has been updated with latest results.

@IanCal
Copy link
Contributor

IanCal commented Jan 7, 2025

For an initial list, I've used a filtered set here https://openrouter.ai/models?fmt=cards&order=newest&supported_parameters=tools

openai/gpt-4o

openai/gpt-4o-mini

anthropic/claude-3.5-sonnet

google/gemini-pro-1.5

We can try out other models and add them if they seem to be working, this gives a reasonable list of some popular models right now, which have

O1 will be good to add, but it is not currently generally available via openrouter and the newer google models are all marked experimental (which would be fine, but they make them free then heavily rate limit them so they quickly break).

@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 7, 2025

For an initial list, I've used a filtered set here https://openrouter.ai/models?fmt=cards&order=newest&supported_parameters=tools

openai/gpt-4o

openai/gpt-4o-mini

anthropic/claude-3.5-sonnet

google/gemini-pro-1.5

We can try out other models and add them if they seem to be working, this gives a reasonable list of some popular models right now, which have

O1 will be good to add, but it is not currently generally available via openrouter and the newer google models are all marked experimental (which would be fine, but they make them free then heavily rate limit them so they quickly break).

Currently, I obtain the list by calling openai.models.list(). With the list you provided, does this mean we no longer need to call openai.models.list()? Instead, we can set the list in the runtime common.

@IanCal
Copy link
Contributor

IanCal commented Jan 7, 2025

Currently, I obtain the list by calling openai.models.list(). With the list you provided, does this mean we no longer need to call openai.models.list()? Instead, we can set the list in the runtime common.

Yep, this can be how we have the default list.

@FadhlanR FadhlanR force-pushed the cs-7714-show-current-llm-model-used branch from 5c159cd to 3e66444 Compare January 8, 2025 06:00
'google/gemini-pro-1.5',
'openai/gpt-4o',
'openai/gpt-4o-mini',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the right place to put DEFAULT_LLM and DEFAULT_LLM_LIST constants but I put them here to make it available for host and ai-bot.

@FadhlanR FadhlanR marked this pull request as ready for review January 8, 2025 08:15
@FadhlanR FadhlanR requested review from lukemelia and IanCal January 8, 2025 08:16
@jurgenwerk
Copy link
Contributor

I chose Claude but bot says:
image

@jurgenwerk jurgenwerk requested a review from a team January 9, 2025 08:37
@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 9, 2025

I chose Claude but bot says:

image

Were you trying it locally and running the latest ai-bot code?

@jurgenwerk
Copy link
Contributor

jurgenwerk commented Jan 9, 2025

I am running cs-7714-show-current-llm-model-used branch locally, with a restarted bot

@jurgenwerk
Copy link
Contributor

image

For this particular model, I get the following error in the ai bot:

Request used 884 prompt tokens and 0
Attempt 1 failed: TypeError: response.error.includes is not a function
    at fetchGenerationCost (/Users/jurgen/development/boxel/packages/ai-bot/lib/ai-billing.ts:92:40)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async retry (/Users/jurgen/development/boxel/packages/runtime-common/utils.ts:7:22)
    at async saveUsageCost (/Users/jurgen/development/boxel/packages/ai-bot/lib/ai-billing.ts:22:21)
Attempt 2 failed: TypeError: response.error.includes is not a function
    at fetchGenerationCost (/Users/jurgen/development/boxel/packages/ai-bot/lib/ai-billing.ts:92:40)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async retry (/Users/jurgen/development/boxel/packages/runtime-common/utils.ts:7:22)
    at async saveUsageCost (/Users/jurgen/development/boxel/packages/ai-bot/lib/ai-billing.ts:22:21)
    ```

@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 9, 2025

@jurgenwerk I just tested it again in my local with two models and it worked. I couldn't reproduce it. It seems the value of response.error is not a string, I guess we need to capture the error structure so we can update how we handle the error in fetchGenerationCost.

Screenshot 2025-01-09 at 19 25 33 Screenshot 2025-01-09 at 19 24 33

@jurgenwerk
Copy link
Contributor

I don't know what went wrong previously bot for the purposes of some other task I nuked all my local dbs and started anew in the meanwhile, and now this feature works for me 🥂 However I noticed a bug - switching doesn't seem to work ok:

image

@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 9, 2025

I don't know what went wrong previously bot for the purposes of some other task I nuked all my local dbs and started anew in the meanwhile, and now this feature works for me 🥂 However I noticed a bug - switching doesn't seem to work ok:

image

I noticed that too, it seems the ai bot will answer based on the previous conversation. If you ask the ai to forget the previous conversation, it will give you the right answer.

@@ -570,6 +574,16 @@ export function isCommandEvent(
);
}

function getModel(eventlist: DiscreteMatrixEvent[]): string {
let activeLLMEvent = eventlist.findLast(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be reading this from room state instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could read this from the room state, but I chose to read it from the event list to make testing easier. Additionally, this approach is consistent with how we read skills from the room.

@IanCal
Copy link
Contributor

IanCal commented Jan 9, 2025

I tried with a local key and can see the requests going to the right providers even if they respond with what has been said before.

I found that when changing the room however the model didn't change in the dropdown

Screencast.from.09-01-25.15.49.19.webm

@IanCal
Copy link
Contributor

IanCal commented Jan 9, 2025

I've not been able to get sonnet working either, not sure why but I think it's unrelated to this work

edit - found the issue. If the last message is from the assistant, claude breaks. I'll create a test and a fix in a different PR, it's a small change.

@FadhlanR
Copy link
Contributor Author

FadhlanR commented Jan 10, 2025

I found that when changing the room however the model didn't change in the dropdown

Fixed! please check it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants