Skip to content

Clone CreateResponseOptions #904

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.

Sign up for GitHub

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
christothes merged 4 commits into from
Jan 22, 2026
+636 −6
Merged

Clone CreateResponseOptions #904

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93e336d
Clone CreateResponseOptions
christothes Jan 21, 2026
f83d84e
fix
christothes Jan 21, 2026
8e754ed
fix
christothes Jan 22, 2026
7df356b
fb
christothes Jan 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Custom/Responses/CreateResponseOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ namespace OpenAI.Responses;
[CodeGenType("CreateResponse")]
public partial class CreateResponseOptions
{
internal static readonly CreateResponseOptions Default = new CreateResponseOptions();
Copy link
Collaborator

@jsquire jsquire Jan 22, 2026

Choose a reason for hiding this comment

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

Will this work without being [ThreadStatic]? Won't we end up with concurrency issues because of the mutation during operations?


// CUSTOM: Added as a convenience.
public CreateResponseOptions(IEnumerable<ResponseItem> inputItems, string model = default) : this()
{
Expand Down
2 changes: 1 addition & 1 deletion src/Custom/Responses/ResponsesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ public virtual AsyncCollectionResult<ResponseItem> GetResponseInputItemsAsync(st
internal virtual CreateResponseOptions CreatePerCallOptions(CreateResponseOptions userProvidedOptions)
{
CreateResponseOptions clonedOptions = userProvidedOptions is null
Copy link
Collaborator

@jsquire jsquire Jan 22, 2026

Choose a reason for hiding this comment

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

Should we consider doing a thread-static shared instance for default options rather than creating an instance per-call?

Copy link
Collaborator Author

@christothes christothes Jan 22, 2026

Choose a reason for hiding this comment

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

What scenario did you have in mind where we'd use the default options? My assumption is that we'll typically only clone when someone is passing in a non-default options instance.

Copy link
Collaborator

@jsquire jsquire Jan 22, 2026

Choose a reason for hiding this comment

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

We're doing a new() if there's no custom options passed. My suggestion is to replace that with a thread-static version. We need to be able to safely mutate for this call but there's no reason why we need a fresh instance each time, is there?

Copy link
Collaborator Author

@christothes christothes Jan 22, 2026

Choose a reason for hiding this comment

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

Ah, makes sense. For some reason I thought this comment was on the clone method.

? new()
? CreateResponseOptions.Default
: userProvidedOptions.Clone();

// If the model is null, use the default specified in the client.
Expand Down