Skip to content

Conversation

daydaychen
Copy link
Contributor

Motivation and Context

  1. Why is this change required?

    • The previous condition copied usage metadata whenever chunk.usage existed, which could incorrectly propagate usage into chunks that already contained choices.
  2. What problem does it solve?

    • Prevents incorrect propagation of usage metadata that can cause empty messages or duplicated metadata when a streaming chunk contains choices plus usage.
  3. What scenario does it contribute to?

    • Streaming OpenAI chat responses where the API may send a final usage-only chunk (no choices) to report usage stats.
  4. If it fixes an open issue, please link to the issue here.

    • No linked issue.

Description

  • File modified:

    • python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py
  • Change summary:

    • In OpenAIChatCompletionBase._inner_get_streaming_chat_message_contents,
      replace:
      if chunk.usage is not None
      with:
      if len(chunk.choices) == 0 and chunk.usage is not None
  • Design notes:

    • This is a minimal, targeted fix to ensure usage metadata is only copied when the streaming chunk contains no choices (typical for the final usage-only chunk).
    • Behavior of other streaming logic is unchanged.
    • Recommend adding unit tests covering:
      • A chunk that contains choices and usage (should not propagate usage).
      • A usage-only final chunk (should propagate usage to the synthesized choice).

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the SK Contribution Guidelines and formatting script raises no violations
  • All unit tests pass, and I have added new tests where possible
  • I didn't break anyone 😄

Files changed:

  • python/semantic_kernel/connectors/ai/open_ai/services/open_ai_chat_completion_base.py

@daydaychen daydaychen requested a review from a team as a code owner August 20, 2025 11:09
@moonbox3 moonbox3 added the python Pull requests for the Python Semantic Kernel label Aug 20, 2025
@moonbox3 moonbox3 requested a review from Copilot August 21, 2025 01:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the OpenAI chat completion streaming logic to prevent incorrect propagation of usage metadata when it appears alongside actual content choices. The change ensures usage metadata is only copied to synthesized choices when the streaming chunk contains no choices, which is the expected pattern for final usage-only chunks from the OpenAI API.

Key Changes

  • Modified the condition in _inner_get_streaming_chat_message_contents to check for empty choices before propagating usage metadata
  • Prevents incorrect behavior when streaming chunks contain both choices and usage metadata

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@moonbox3
Copy link
Collaborator

moonbox3 commented Aug 21, 2025

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
connectors/ai/open_ai/services
   open_ai_chat_completion_base.py128794%71, 81, 102, 122, 143, 179, 320
TOTAL27111472782% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
3715 22 💤 0 ❌ 0 🔥 1m 36s ⏱️

@moonbox3 moonbox3 enabled auto-merge September 1, 2025 04:24
@moonbox3 moonbox3 added this pull request to the merge queue Sep 1, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 1, 2025
@eavanvalkenburg eavanvalkenburg added this pull request to the merge queue Sep 1, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 1, 2025
@TaoChenOSU TaoChenOSU added this pull request to the merge queue Sep 3, 2025
Merged via the queue into microsoft:main with commit da8ca43 Sep 3, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants