fix: bug file uploading error - #684
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the solution’s OpenAI/Azure OpenAI invocation paths to better support GPT‑5 deployments by avoiding incompatible request parameters (notably non‑default temperatures) and using non‑streaming APIs where GPT‑5 streaming isn’t supported.
Changes:
- Adjust Azure OpenAI text/chat generation to detect GPT‑5 deployments and conditionally omit legacy sampling parameters and streaming calls.
- Stop explicitly setting
Temperaturein the backend chat execution settings to avoid GPT‑5 incompatibilities. - Update Kernel Memory handler defaults from
Temperature=0toTemperature=1in a couple of prompt execution settings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| App/kernel-memory/service/Core/Handlers/KeywordExtractingHandler.cs | Changes the handler’s prompt execution temperature default from 0 to 1. |
| App/kernel-memory/service/Core/DataFormats/Image/ImageContextDecoder.cs | Changes the image decoding prompt execution temperature default from 0 to 1. |
| App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs | Adds GPT‑5 detection and switches between streaming/non‑streaming plus parameter omission for GPT‑5. |
| App/backend-api/Microsoft.GS.DPS/API/ChatHost/ChatHost.cs | Removes explicit temperature from prompt execution settings and adds GPT‑5 compatibility note. |
Comments suppressed due to low confidence (1)
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:194
- Same as above: the comment claims GPT-5 uses
max_completion_tokens, but this implementation does not set an alternative token-limit parameter for GPT-5 (it only omits MaxTokens/sampling params). The comment should reflect the actual behavior to avoid confusion when debugging token truncation/cost issues.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming, so we use non-streaming API for GPT-5.
if (!isGpt5Deployment)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:136
- These comments say GPT-5 uses
max_completion_tokens, but this implementation doesn't actually set any token limit for GPT-5 (it just omitsMaxTokens). Consider rewording to match the current behavior and avoid implyingmax_completion_tokensis being sent.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming.
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:193
- These comments say GPT-5 uses
max_completion_tokens, but the current code does not send any GPT-5-specific token limit (it only omitsMaxTokens). Please reword so the comment reflects actual behavior, or implement an explicit GPT-5 token-limit parameter once supported by the SDK.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming, so we use non-streaming API for GPT-5.
- Updated Chat_SystemPrompt.txt to clarify that content already has citations - Added deduplication logic in ChatHost.cs to remove consecutive duplicate citations - Prevents LLM from adding duplicate citations when content already includes them
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:219
- In the chat-completions path, TokenSelectionBiases are still applied even when isGpt5Deployment is true (the block above this GPT-5 branching runs unconditionally). This is inconsistent with the text-completions path (where biases are skipped for GPT-5) and can reintroduce GPT-5 request validation failures if logit bias is not supported.
if (options.StopSequences is { Count: > 0 })
{
foreach (var s in options.StopSequences) { openaiOptions.StopSequences.Add(s); }
}
if (options.TokenSelectionBiases is { Count: > 0 })
{
foreach (var (token, bias) in options.TokenSelectionBiases) { openaiOptions.TokenSelectionBiases.Add(token, (int)bias); }
}
openaiOptions.Messages.Add(new ChatRequestSystemMessage(prompt));
// GPT-5 does not support streaming - use non-streaming API
if (isGpt5Deployment)
{
Response<ChatCompletions>? response = await this._client.GetChatCompletionsAsync(openaiOptions, cancellationToken).ConfigureAwait(false);
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:136
- The GPT-5 comment says the request should use max_completion_tokens instead of max_tokens, but this code never sets any max-completion-tokens equivalent for GPT-5 (and also leaves MaxTokens unset). This is misleading for future maintainers reading the code.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming.
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:193
- Same as above: this GPT-5 comment claims max_completion_tokens behavior, but the implementation does not set a GPT-5-specific max-token property. Consider updating the comment to match the implementation (or implement max_completion_tokens if the SDK supports it).
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming, so we use non-streaming API for GPT-5.
App/backend-api/Microsoft.GS.DPS/Prompts/Chat_SystemPrompt.txt:16
- These instructions have grammatical issues that can reduce prompt clarity (e.g., "in your every sentences", "Make a detail answer"). Consider tightening the wording while keeping the intent the same.
You should include citations in your every sentences. The [Content] section already contains citations - do NOT add duplicate citations.
Make a detail answer as much as possible up to over 4000 characters.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:198
- In the chat-completions path,
TokenSelectionBiasesare still applied for GPT-5 deployments even though other sampling parameters are gated behind!isGpt5Deployment. This is inconsistent with the completion path (where biases are gated) and can reintroduce GPT-5 request rejections if logit bias is treated as a legacy sampling parameter.
if (!isGpt5Deployment)
{
openaiOptions.MaxTokens = options.MaxTokens;
openaiOptions.Temperature = (float)options.Temperature;
openaiOptions.NucleusSamplingFactor = (float)options.NucleusSampling;
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:136
- The comment says GPT-5 uses
max_completion_tokensinstead ofmax_tokens, but the code does not set any GPT-5-specific max token field (it just omitsMaxTokens). This makes the comment misleading for future maintainers—either implement the alternative field (if supported) or update the comment to reflect the actual behavior.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming.
App/kernel-memory/extensions/AzureOpenAI/AzureOpenAITextGenerator.cs:193
- The comment mentions GPT-5 using
max_completion_tokensinstead ofmax_tokens, but the code path doesn’t actually set a GPT-5-specific max token parameter (it only omitsMaxTokens). This mismatch can confuse future updates; either set the correct parameter (if available in the SDK) or update the comment to match the implementation.
// GPT-5 deployments use max_completion_tokens instead of max_tokens
// and require Temperature = 1.0 (default), rejecting other legacy sampling parameters.
// GPT-5 also does NOT support streaming, so we use non-streaming API for GPT-5.
App/backend-api/Microsoft.GS.DPS/Prompts/Chat_SystemPrompt.txt:16
- This line still has grammatical issues: “Make a detail answer … up to over 4000 characters” is awkward/contradictory. Consider rephrasing to a single clear limit.
Make a detail answer as much as possible up to over 4000 characters.
Purpose
This pull request introduces improved compatibility with GPT-5 deployments and updates default model parameters across several components. The most significant changes are focused on handling GPT-5's unique requirements, such as default temperature settings and the lack of support for streaming or legacy sampling parameters.
GPT-5 Compatibility and Model Parameter Handling:
Updated
AzureOpenAITextGenerator.csto detect GPT-5 deployments and bypass legacy sampling parameters (likeTemperature,NucleusSamplingFactor, etc.), using API defaults instead. For GPT-5, the code now uses the non-streaming API, as streaming is not supported. Non-GPT-5 deployments continue to use the streaming API and legacy parameters. [1] [2] [3] [4]In
ChatHost.cs, changed the prompt execution settings to avoid explicitly setting theTemperatureparameter, preventing compatibility issues with GPT-5 which requires the default value (Temperature=1.0).Default Parameter Updates:
Temperatureparameter to1(from0) in bothImageContextDecoder.csandKeywordExtractingHandler.cs, aligning with GPT-5's requirements and improving consistency across the codebase. [1] [2]Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify that the following are valid
Other Information