Skip to content

OpenAI-DotNet 8.6.3 #440

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

Merged
merged 3 commits into from
Mar 21, 2025
Merged

OpenAI-DotNet 8.6.3 #440

merged 3 commits into from
Mar 21, 2025

Conversation

StephenHodgson
Copy link
Member

  • Fixed Assistant Reasoning Model request serialization when using thread.CreateRunAsync

- Fixed Assistant Reasoning Model request serialization when using thread.CreateRunAsync
@StephenHodgson StephenHodgson requested a review from Copilot March 21, 2025 13:16
Copy link

@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 the serialization of the Assistant Reasoning Model request when using thread.CreateRunAsync. Key changes include updating the temperature and topP parameter handling based on assistant.ReasoningEffort in the ThreadExtensions.cs file and adding a new test to validate reasoning model streaming in TestFixture_03_Threads.cs.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
OpenAI-DotNet/Threads/ThreadExtensions.cs Updated parameter selection for temperature and topP based on ReasoningEffort and added reasoningEffort to the request.
OpenAI-DotNet-Tests/TestFixture_03_Threads.cs Added a new test (Test_06_02_CreateRun_Reasoning_Streaming) to ensure correct handling of reasoning model streaming.
Files not reviewed (1)
  • OpenAI-DotNet/OpenAI-DotNet.csproj: Language not supported
Comments suppressed due to low confidence (1)

OpenAI-DotNet/Threads/ThreadExtensions.cs:185

  • The fallback value for topP is set to assistant.Temperature; confirm if this is intentional or if it should use assistant.TopP instead.
topP: assistant.ReasoningEffort > 0 ? assistant.TopP : assistant.Temperature,

@StephenHodgson StephenHodgson requested a review from Copilot March 21, 2025 13:36
Copy link

@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 an issue with the serialization of the Assistant Reasoning Model by updating CreateRunAsync to conditionally pass temperature and topP parameters based on ReasoningEffort and adding a new streaming test.

  • Updated CreateRunAsync to set temperature and topP to null when ReasoningEffort is active, and pass the reasoningEffort parameter.
  • Added a new test (Test_06_02_CreateRun_Reasoning_Streaming) to cover streaming functionality for reasoning.
  • Removed an unused using directive in the test file.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
OpenAI-DotNet/Threads/ThreadExtensions.cs Updated parameter handling for reasoning model requests.
OpenAI-DotNet-Tests/TestFixture_03_Threads.cs Added a test for reasoning streaming and removed an unused using directive.
Files not reviewed (1)
  • OpenAI-DotNet/OpenAI-DotNet.csproj: Language not supported
Comments suppressed due to low confidence (2)

OpenAI-DotNet/Threads/ThreadExtensions.cs:184

  • Ensure that the CreateRunRequest constructor accepts null values for 'temperature' and 'topP' when ReasoningEffort is greater than zero to avoid type or serialization issues.
temperature: assistant.ReasoningEffort > 0 ? null : assistant.Temperature,

OpenAI-DotNet-Tests/TestFixture_03_Threads.cs:14

  • The removal of 'using System.Threading;' may lead to missing references (e.g., CancellationToken) in this file; please verify that all necessary types are still available.
using System.Threading;

@StephenHodgson StephenHodgson marked this pull request as ready for review March 21, 2025 13:40
@StephenHodgson StephenHodgson merged commit 5836844 into main Mar 21, 2025
4 checks passed
@StephenHodgson StephenHodgson deleted the development branch March 21, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant