Fix Moonshot Chat model toolcalling token usage #1927
Conversation
|
@mxsl-gr Could you check this PR? |
no problem, I'll check and test this PR later |
@mxsl-gr thank you |
hi, @ilayaperumalg |
|
@mxsl-gr I don't seem to find the suggestions on this PR. Could you check? |
|
|
||
| ChatResponse chatResponse = new ChatResponse(generations, from(completionEntity.getBody())); | ||
| MoonshotApi.Usage usage = completionEntity.getBody().usage(); | ||
| Usage currentUsage = (usage != null) ? MoonshotUsage.from(usage) : new EmptyUsage(); |
There was a problem hiding this comment.
I think this line of code seems unnecessary. MoonshotApi.Usage can directly implement the Usage interface, and UsageUtils.getCumulativeUsage already handles null checks for the input. This approach ensures that callers don’t need to repeatedly check for currentUsage in every call, avoiding redundant code and improving cohesion.
If an EmptyUsage is truly needed, it could simply be a static constant like Usage.empty() or EmptyUsage.instance().
There was a problem hiding this comment.
I understand your perspective but we wanted to address this as part of #1487 - not just for OpenAI usage but across the models. But for now, I think we can go with the consistent approach across the models by passing the EmptyUsage object when the ChatModel API usage is null. I hope that is ok with you.
| @@ -286,8 +300,11 @@ public Flux<ChatResponse> stream(Prompt prompt) { | |||
| // @formatter:on | |||
| return buildGeneration(choice, metadata); | |||
| }).toList(); | |||
| MoonshotApi.Usage usage = chatCompletion2.usage(); | |||
| Usage currentUsage = (usage != null) ? MoonshotUsage.from(usage) : new EmptyUsage(); | |||
| */ | ||
| @SpringBootTest | ||
| @EnabledIfEnvironmentVariable(named = "MOONSHOT_API_KEY", matches = ".+") | ||
| public class MoonShotChatModelIT { |
There was a problem hiding this comment.
I recall there's a test class called MoonshotChatModelFunctionCallingIT that already contains unit tests for function calls. Instead of creating a new test class, maybe you could consider adding the new test methods to the existing one. And there is already a class org.springframework.ai.moonshot.chat.MoonshotChatModelIT
Also, the correct model name is Moonshot, not MoonShot.
There was a problem hiding this comment.
Got it. I moved these tests into the existing MoonshotChatModelFunctionCallingIT.
sorry, i forget finish the review... |
- Accumulate the token usage when toolcalling is invoked
- Fix both call() and stream() methods
- Add `usage` field to the Chat completion choice as the usage is returned via Choice
- Add Mootshot chatmodel ITs for functioncalling tests
ilayaperumalg
force-pushed
the
moonshot-toolcalling-usage
branch
from
de9a356 to
4f4c2d0
Compare
|
@mxsl-gr Could you check the latest updates from this PR? Thank you |
usagefield to the Chat completion choice as the usage is returned via Choice