Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Apr 21, 2025

PR Type

Enhancement, Bug fix


Description

  • Refactored LLM token and cost tracking for multimodal support

    • Introduced granular token fields (text/audio, cached/non-cached)
    • Updated cost model to support detailed input/output cost breakdowns
  • Updated all LLM provider plugins to use new token/cost fields

    • Replaced legacy prompt/completion fields with new structure
    • Ensured accurate token accounting for cached and audio tokens
  • Adjusted configuration to new cost schema in appsettings.json

    • Migrated from flat PromptCost/CompletionCost to nested Cost object
  • Improved statistics and reporting logic for LLM usage and costs


Changes walkthrough 📝

Relevant files
Enhancement
17 files
TokenStatsModel.cs
Refactor token stats model for multimodal support               
+9/-3     
LlmModelSetting.cs
Redesign LLM cost model for detailed input/output               
+13/-15 
TokenStatistics.cs
Update statistics logic for new token/cost fields               
+17/-12 
ChatCompletionProvider.cs
Update token stats usage to new fields                                     
+2/-2     
ChatCompletionProvider.cs
Use granular token fields for Azure OpenAI chat                   
+15/-5   
TextCompletionProvider.cs
Use new token fields for text completions                               
+3/-2     
ChatCompletionProvider.cs
Update DeepSeekAI chat to new token/cost model                     
+12/-4   
TextCompletionProvider.cs
Use new token fields for DeepSeekAI text completions         
+6/-2     
GeminiChatCompletionProvider.cs
Update Gemini chat to new token/cost fields                           
+4/-6     
GeminiTextCompletionProvider.cs
Use new token fields for Gemini text completions                 
+2/-3     
TextCompletionProvider.cs
Update LangChain provider to new token stats                         
+8/-8     
ChatCompletionProvider.cs
Use new token fields for MetaGLM chat completions               
+2/-2     
ResponseDone.cs
Add detailed input/output token structures for OpenAI       
+39/-0   
ChatCompletionProvider.cs
Update OpenAI chat to use granular token fields                   
+12/-5   
RealTimeCompletionProvider.cs
Use detailed token stats in real-time OpenAI completions 
+19/-11 
TextCompletionProvider.cs
Use new token fields for OpenAI text completions                 
+2/-2     
ChatCompletionProvider.cs
Update SparkDesk provider to new token fields                       
+4/-4     
Miscellaneous
1 files
InstructModeController.cs
Rename variable for audio data clarity                                     
+2/-2     
Configuration changes
1 files
appsettings.json
Migrate LLM model cost config to nested Cost object           
+73/-20 

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @GGHansome
    Copy link

    Auto Review Result:

    Code Review Summary

    Change Overview: The code changes are aimed at enhancing the TokenStatsModel by incorporating more detailed input and output token tracking, including separate accounting for text and audio tokens. This improves accuracy in determining token usage costs for various input/output types in a machine learning context.

    Issues Found

    Issue 1: Consistency Issue

    • Description: There are instances of variable naming inconsistency when handling token counts. In some parts of the code, cached tokens are subtracted from total tokens, while in others, cached tokens are explicitly tracked.
    • Suggestion: Ensure consistent handling and naming of cached and total tokens across the entire codebase to avoid confusion and potential errors.
    • Example:
      // Current
      TextInputTokens = (tokenUsage?.InputTokenCount ?? 0) - (inputTokenDetails?.CachedTokenCount ?? 0);
      // Ensure consistency
      TextInputTokens = tokenUsage?.TextTokens ?? 0;
      CachedTextInputTokens = inputTokenDetails?.CachedTokenCount ?? 0;

    Issue 2: Potential Null Reference

    • Description: There are multiple points where nullable types such as ?. and ?? operators are used, which could lead to potential null reference exceptions if not carefully handled.
    • Suggestion: Include additional null checks or use default values throughout the code where nullable chaining is used to avoid runtime exceptions.
    • Example:
      // Current
      TextInputTokens = response?.UsageMetadata?.PromptTokenCount ?? 0;
      // Suggested
      if (response?.UsageMetadata != null) {
          TextInputTokens = response.UsageMetadata.PromptTokenCount;
      } else {
          TextInputTokens = 0;
      }

    Overall Evaluation

    The changes enhance the token management system by differentiating between text and audio tokens accurately. While the logic seems sound, particular attention should be paid to code consistency and avoiding potential null reference issues. Consider refactoring or encapsulating repetitive null handling code to improve readability and maintainability.

    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Null Reference

    The code uses null-conditional operators but still accesses properties that might be null, potentially causing NullReferenceException. For example, inputTokenDetails?.TextTokens ?? 0 - inputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0 has incorrect parentheses grouping.

    TextInputTokens = inputTokenDetails?.TextTokens ?? 0 - inputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0,
    CachedTextInputTokens = data.Usage?.InputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0,
    AudioInputTokens = inputTokenDetails?.AudioTokens ?? 0 - inputTokenDetails?.CachedTokenDetails?.AudioTokens ?? 0,
    CachedAudioInputTokens = inputTokenDetails?.CachedTokenDetails?.AudioTokens ?? 0,
    TextOutputTokens = outputTokenDetails?.TextTokens ?? 0,
    AudioOutputTokens = outputTokenDetails?.AudioTokens ?? 0
    Undefined Variable

    The variable deltaTotal is used but its calculation has been refactored and moved to a new line. The code still references this variable but it might not contain the expected value.

    var deltaTotal = deltaPromptCost + deltaCompletionCost;
    _promptCost += deltaPromptCost;
    _completionCost += deltaCompletionCost;

    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix missing parentheses

    The subtraction operation is missing parentheses, which will cause incorrect
    token calculation. The current expression will subtract the result of
    inputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0 from
    inputTokenDetails?.TextTokens ?? 0 rather than subtracting cached tokens from
    total tokens.

    src/Plugins/BotSharp.Plugin.OpenAI/Providers/Realtime/RealTimeCompletionProvider.cs [598]

    -TextInputTokens = inputTokenDetails?.TextTokens ?? 0 - inputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0,
    +TextInputTokens = (inputTokenDetails?.TextTokens ?? 0) - (inputTokenDetails?.CachedTokenDetails?.TextTokens ?? 0),
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The current expression has incorrect operator precedence which would cause a calculation error. Without parentheses, the subtraction would happen after the null coalescing operator, leading to incorrect token counting and potentially negative values.

    High
    Handle null audio delta

    The code is missing handling for the case when audio?.Delta is null. The removed
    else block suggests there was previously a fallback handling, but now there's no
    action taken when Delta is null, which could lead to missed audio responses.

    src/Plugins/BotSharp.Plugin.OpenAI/Providers/Realtime/RealTimeCompletionProvider.cs [173-181]

     else if (response.Type == "response.audio.delta")
     {
         var audio = JsonSerializer.Deserialize<ResponseAudioDelta>(receivedText);
         if (audio?.Delta != null)
         {
             _logger.LogDebug($"{response.Type}: {receivedText}");
             onModelAudioDeltaReceived(audio.Delta, audio.ItemId);
         }
    +    else
    +    {
    +        _logger.LogDebug($"{response.Type}: {receivedText}");
    +        onModelAudioDeltaReceived(null, audio?.ItemId);
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The PR removed an else block that handled null audio deltas, which could lead to missed audio responses. Restoring this handling ensures all audio responses are properly processed, improving robustness of the audio processing pipeline.

    Medium
    • More

    @iceljc iceljc requested a review from Oceania2018 April 21, 2025 16:40
    @iceljc iceljc merged commit 4de62cb into SciSharp:master Apr 21, 2025
    4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants