Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 22 additions & 17 deletions src/Custom/Chat/ChatClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,12 @@ internal async Task<ClientResult<ChatCompletion>> CompleteChatAsync(IEnumerable<
}

options ??= new();
CreateChatCompletionOptions(messages, ref options);
using OpenTelemetryScope scope = _telemetry.StartChatScope(options);
var clonedOptions = CreateChatCompletionOptions(messages, options);
using OpenTelemetryScope scope = _telemetry.StartChatScope(clonedOptions);

try
{
using BinaryContent content = options.ToBinaryContent();
using BinaryContent content = clonedOptions.ToBinaryContent();

ClientResult result = await CompleteChatAsync(content, requestOptions).ConfigureAwait(false);
ChatCompletion chatCompletion = (ChatCompletion)result;
Expand All @@ -181,12 +181,12 @@ public virtual ClientResult<ChatCompletion> CompleteChat(IEnumerable<ChatMessage
Argument.AssertNotNullOrEmpty(messages, nameof(messages));

options ??= new();
CreateChatCompletionOptions(messages, ref options);
using OpenTelemetryScope scope = _telemetry.StartChatScope(options);
var clonedOptions = CreateChatCompletionOptions(messages, options);
using OpenTelemetryScope scope = _telemetry.StartChatScope(clonedOptions);

try
{
using BinaryContent content = options.ToBinaryContent();
using BinaryContent content = clonedOptions.ToBinaryContent();
ClientResult result = CompleteChat(content, cancellationToken.ToRequestOptions());
ChatCompletion chatCompletion = (ChatCompletion)result;

Expand Down Expand Up @@ -242,9 +242,9 @@ internal AsyncCollectionResult<StreamingChatCompletionUpdate> CompleteChatStream
}

options ??= new();
CreateChatCompletionOptions(messages, ref options, stream: true);
var clonedOptions = CreateChatCompletionOptions(messages, options, stream: true);

using BinaryContent content = options.ToBinaryContent();
using BinaryContent content = clonedOptions.ToBinaryContent();
return new AsyncSseUpdateCollection<StreamingChatCompletionUpdate>(
async () => await CompleteChatAsync(content, requestOptions).ConfigureAwait(false),
StreamingChatCompletionUpdate.DeserializeStreamingChatCompletionUpdate,
Expand All @@ -269,9 +269,9 @@ public virtual CollectionResult<StreamingChatCompletionUpdate> CompleteChatStrea
Argument.AssertNotNull(messages, nameof(messages));

options ??= new();
CreateChatCompletionOptions(messages, ref options, stream: true);
var clonedOptions = CreateChatCompletionOptions(messages, options, stream: true);

using BinaryContent content = options.ToBinaryContent();
using BinaryContent content = clonedOptions.ToBinaryContent();
return new SseUpdateCollection<StreamingChatCompletionUpdate>(
() => CompleteChat(content, cancellationToken.ToRequestOptions(streaming: true)),
StreamingChatCompletionUpdate.DeserializeStreamingChatCompletionUpdate,
Expand Down Expand Up @@ -380,19 +380,24 @@ public virtual ClientResult<ChatCompletionDeletionResult> DeleteChatCompletion(s
return ClientResult.FromValue((ChatCompletionDeletionResult)result, result.GetRawResponse());
}

private void CreateChatCompletionOptions(IEnumerable<ChatMessage> messages, ref ChatCompletionOptions options, bool stream = false)
private ChatCompletionOptions CreateChatCompletionOptions(IEnumerable<ChatMessage> messages, ChatCompletionOptions options, bool stream = false)
{
options.Messages = messages.ToList();
options.Model = _model;
var clonedOptions = options.Clone();
foreach (var message in messages)
{
clonedOptions.Messages.Add(message);
}
clonedOptions.Model ??= _model;
if (stream)
{
options.Stream = true;
options.StreamOptions = s_includeUsageStreamOptions;
clonedOptions.Stream = true;
clonedOptions.StreamOptions = s_includeUsageStreamOptions;
}
else
{
options.Stream = null;
options.StreamOptions = null;
clonedOptions.Stream = null;
clonedOptions.StreamOptions = null;
}
return clonedOptions;
}
}
41 changes: 41 additions & 0 deletions src/Custom/Chat/ChatCompletionOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.ClientModel.Primitives;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;

namespace OpenAI.Chat;
Expand Down Expand Up @@ -215,4 +216,44 @@ public ChatResponseModalities ResponseModalities
public ChatOutputPrediction OutputPrediction { get; set; }

internal BinaryContent ToBinaryContent() => BinaryContent.Create(this, ModelSerializationExtensions.WireOptions);

internal ChatCompletionOptions Clone()
{
// Copy the values of the properties to a new instance of ChatCompletionOptions. For collection properties, create a new list and add the items from the original list.
var clone = new ChatCompletionOptions();
clone.Model = Model;
clone.N = N;
clone.Stream = Stream;
clone.StreamOptions = StreamOptions;
clone.IncludeLogProbabilities = IncludeLogProbabilities;
clone.TopLogProbabilityCount = TopLogProbabilityCount;
foreach (var s in StopSequences) clone.StopSequences.Add(s);
foreach (var l in LogitBiases) clone.LogitBiases[l.Key] = l.Value;
clone.ToolChoice = ToolChoice;
clone.FunctionChoice = FunctionChoice;
clone.AllowParallelToolCalls = AllowParallelToolCalls;
clone.EndUserId = EndUserId;
clone._deprecatedMaxTokens = _deprecatedMaxTokens;
clone.MaxOutputTokenCount = MaxOutputTokenCount;
foreach (var f in Functions) clone.Functions.Add(f);
foreach (var m in Metadata) clone.Metadata[m.Key] = m.Value;
clone.StoredOutputEnabled = StoredOutputEnabled;
clone.ReasoningEffortLevel = ReasoningEffortLevel;
clone.InternalModalities = _internalModalities?.ToList();
clone.ResponseModalities = _responseModalities;
clone.ResponseFormat = ResponseFormat;
clone.AudioOptions = AudioOptions;
clone.OutputPrediction = OutputPrediction;
clone.Messages = Messages?.Select(m => m).ToList();
foreach (var t in Tools) clone.Tools.Add(t);
clone.Temperature = Temperature;
clone.TopP = TopP;
clone.SafetyIdentifier = SafetyIdentifier;
clone.ServiceTier = ServiceTier;
clone.FrequencyPenalty = FrequencyPenalty;
clone.PresencePenalty = PresencePenalty;
clone.WebSearchOptions = WebSearchOptions;
clone.Seed = Seed;
return clone;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would using MemberwiseClone work here? It would be nice to not have to update this method every time a new property is added.

We are currently using MemberwiseClone in Responses:

internal CreateResponseOptions GetClone()
{
CreateResponseOptions copiedOptions = (CreateResponseOptions)this.MemberwiseClone();
copiedOptions.Patch = _patch;
return copiedOptions;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if it would do the same thing with the collection properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't. It doesn't do a great job of even a shallow clone inline. We tried this with the messaging libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you are correct that it won't be exactly the same: The current implementation creates a new collection and adds references to the existing items, while MemberwiseClone will simply copy the reference to the existing collection.

I'm thinking that MemberwiseClone might be sufficient because the intention here is not to make ChatCompletionOptions thread-safe, but rather to simply avoid modifying it. In other words, the user's instance of ChatCompletionOptions should be unchanged after they call CompleteChat. The reason why this doesn't work today is because we're trampling over a few internal properties like Messages, Stream, etc. But if we MemberwiseClone it, we are free to modify the internal properties of the clone while keeping the user's instance intact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The customer issue motivating this change specifically deals with concurrency issues. Taking the memberwise clone approach would not resolve the issue, I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think MemberwiseClone would solve the issue because the user is specifically talking about the Messages property, which is one of the internal properties that we're trampling over. By design, our models are not guaranteed to be thread-safe (only our clients are thread-safe), so if the user were asking us to make it thread-safe, I think we would choose not to fix it. However, in this particular case, I think this is still a valid issue because the customer is wondering: "If I'm not modifying the ChatCompletionOptions instance at all, shouldn't I be able to use it across requests even if it's not thread-safe?". And the reason why it doesn't work like that is because it is us who are modifying it as part of CompletChat, which we shouldn't do because the ChatCompletionOptions parameter is not a ref parameter.

Here's another way of thinking about this issue separate from concurrency:

  1. First, the user calls ModelReaderWriter.Write to serialize an instance of ChatCompletionOptions
  2. Then, the user calls CompleteChat and passes this instance of ChatCompletionOptions as an argument
  3. Finally, if the user were to call ModelReaderWriter.Write to serialize the same instance of ChatCompletionOptions again, the result should be the same as in Step 1. However, this is currently not the case because we are changing a handful of internal properties that appear in the serialization.

Copy link
Collaborator

@jsquire jsquire Jan 9, 2026

Choose a reason for hiding this comment

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

We should not be mutating ANY of the options in an operation call. Ever. That's a risk for any concurrent calls to the client, which violates our thread safety guarantees. Even if it works today, its by coincidence and not design. I don't want to carry that debt forward.

Cloning the options is something that we should do for client-level options to protect us from the caller mutating after instantiation. We shouldn't have to clone on an operation because we should be treating these as immutable. We need to find a better way to set the per-request model. The underlying pattern is a problem.

In the meantime, I'd rather we go with the heavier option that gives us full safety and concentrate efforts on fixing the client.

}
}
30 changes: 30 additions & 0 deletions tests/Chat/ChatTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,36 @@ public async Task CompleteChatStreamingClosesNetworkStream()
Assert.That(response.IsDisposed);
}

[RecordedTest]
public async Task ValidateConcurrency()
{
ChatClient client = GetTestClient();
ChatCompletionOptions _options = new()
{
Temperature = 0,
};
IReadOnlyList<string> _messages =
[
"2 + 4 = ?",
"3 + 5 = ?",
"4 + 6 = ?",
];
List<string> responses = [];

await Parallel.ForEachAsync(_messages, async (message, cancellationToken) =>
{
List<ChatMessage> messages =
[
new UserChatMessage(message),
];
ChatCompletion completion = await client.CompleteChatAsync(messages, _options, cancellationToken);
responses.Add(completion.Content[0].Text);
Console.WriteLine($"Message: {message}, Response: {completion.Content[0].Text}");
Assert.That(completion.Content[0].Text, Does.StartWith(message.Substring(0, 7)));
});
Assert.That(responses, Has.Count.EqualTo(_messages.Count));
}

[OneTimeTearDown]
public void TearDown()
{
Expand Down
32 changes: 18 additions & 14 deletions tests/SessionRecords/ChatTests/ChatServiceTierWorks.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"Authorization": "Sanitized",
"Content-Length": "200",
"Content-Type": "application/json",
"User-Agent": "OpenAI/2.4.0 (.NET 9.0.9; Microsoft Windows 10.0.26100)"
"User-Agent": "OpenAI/2.7.0 (.NET 9.0.10; Darwin 25.1.0 Darwin Kernel Version 25.1.0: Mon Oct 20 19:32:56 PDT 2025; root:xnu-12377.41.6~2/RELEASE_ARM64_T8132)"
},
"RequestBody": {
"service_tier": "default",
Expand All @@ -25,39 +25,43 @@
"Access-Control-Expose-Headers": "X-Request-ID",
"Alt-Svc": "h3=\":443\"",
"cf-cache-status": "DYNAMIC",
"CF-RAY": "9801cd4e9d7a98e5-SEA",
"CF-RAY": "9aad8e836ce369e2-DFW",
"Connection": "keep-alive",
"Content-Length": "4163",
"Content-Length": "3717",
"Content-Type": "application/json",
"Date": "Tue, 16 Sep 2025 16:40:07 GMT",
"Date": "Sanitized",
"openai-organization": "Sanitized",
"openai-processing-ms": "18891",
"openai-processing-ms": "Sanitized",
"openai-project": "Sanitized",
"openai-version": "2020-10-01",
"Server": "cloudflare",
"Set-Cookie": [
"Sanitized",
"Sanitized"
],
"Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload",
"X-Content-Type-Options": "nosniff",
"x-envoy-upstream-service-time": "19052",
"x-envoy-upstream-service-time": "16330",
"x-openai-proxy-wasm": "v0.1",
"x-ratelimit-limit-requests": "30000",
"x-ratelimit-limit-tokens": "150000000",
"x-ratelimit-remaining-requests": "29999",
"x-ratelimit-remaining-tokens": "149999967",
"x-ratelimit-remaining-tokens": "149999970",
"x-ratelimit-reset-requests": "2ms",
"x-ratelimit-reset-tokens": "0s",
"X-Request-ID": "Sanitized"
},
"ResponseBody": {
"id": "chatcmpl-CGT0WemoYUYz07tLAhaWMFpV0kuOS",
"id": "chatcmpl-CkYAe42Mqhx2VsHDRXee94cQNpkqt",
"object": "chat.completion",
"created": 1758040788,
"created": 1765210476,
"model": "o3-mini-2025-01-31",
"choices": [
{
"index": 0,
"message": {
"role": "assistant",
"content": "During the 1970s and 1980s, science fiction in popular media overwhelmingly dealt with themes that not only entertained audiences but also reflected contemporary cultural and technological concerns. A comprehensive review of films, television shows, literature, and other media from that era highlights several recurring motifs:\n\n1. Space Exploration and Epic Space Opera \n\u2003• Movies such as Star Wars reinvigorated the idea of interstellar adventures, where vast, fantastical galaxies, heroic quests, and larger-than-life conflicts were central. \n\u2003• This era celebrated the mystery of the cosmos, using space as a canvas for exploring humanity’s hopes, fears, and the eternal quest for discovery.\n\n2. Alien Encounters and Extraterrestrial Life \n\u2003• Films like Close Encounters of the Third Kind and Alien brought to the forefront the question of “Are we alone?” and the possibilities (or threats) inherent in first contact with non-human intelligence. \n\u2003• These stories not only provided thrills and wonder but also served as metaphors for “the other” and fostered discussions about communication, difference, and even imperialism.\n\n3. Dystopian Futures and Post-Apocalyptic Narratives \n\u2003• The lingering anxieties of the Cold War, nuclear arms race, and environmental concerns found expression in stories portraying bleak, controlled, or ruined societies. \n\u2003• Movies such as Blade Runner (1982) explored urban decay, corporate dominance, and moral ambiguity, using dystopia as a way to question the cost of rapid technological and societal change.\n\n4. Artificial Intelligence, Robotics, and the Nature of Humanity \n\u2003• There was a growing fascination with the boundaries between human and machine—exemplified by robots, cyborgs, and artificial intelligences. \n\u2003• Works like The Terminator, along with literary works from the period, questioned the ethics of automation and artificial beings, asking what it truly means to be human in an age of technological marvels and potential threats.\n\n5. The Intersection of Technology and Identity \n\u2003• Many narratives used futuristic technology as a lens to examine personal identity, memory, and consciousness. \n\u2003• Issues of existential doubt, human enhancement, and the integration (or conflict) of man and machine became central themes, reflecting society’s hope for—and trepidation about—rapid technological progress.\n\n6. Political and Social Allegory \n\u2003• The era’s science fiction often doubled as a commentary on contemporary societal issues—ranging from Cold War paranoia and government surveillance to corporate greed and the erosion of individuality. \n\u2003• By projecting present-day dilemmas onto future or alternate realities, these works allowed audiences to confront complex ethical and political questions in a speculative context.\n\nIn summary, the sci-fi media of the 1970s and 1980s frequently navigated a landscape where the wonders of space, the terror of dystopian futures, the enigma of alien contact, and the profound questions about technology’s impact on society intersected. These themes resonated with audiences who were living through a time of rapid technological change and global uncertainty, making science fiction not just a form of escapism but also a mirror reflecting the pressing issues of its time.",
"content": "A review of popular media from the 1970s and 1980s reveals several recurring sci‑fi themes that reflected both a fascination with technology’s promise and a cautious awareness of its potential downsides. Here are some of the most common themes from that era:\n\n1. Space Exploration and Cosmic Adventure\n\u2003• Films like Star Wars (1977) and Close Encounters of the Third Kind (1977) sparked imaginations by envisioning vast, uncharted universes. This theme celebrated the wonder of interstellar travel, epic conflicts among empires, and the mystery of distant worlds.\n\n2. Dystopian Futures and the Dark Side of Technology\n\u2003• With the Cold War and the threat of nuclear annihilation looming large, many works—ranging from films such as A Clockwork Orange (1971) and Blade Runner (1982) to various literary works—explored futures marked by societal decay, oppressive regimes, and environmental collapse. These narratives questioned whether technological progress was leading humanity toward liberation or disaster.\n\n3. Artificial Intelligence and the Human–Machine Divide\n\u2003• The rise of computer technology and robotics inspired stories about what happens when our creations take on lives of their own. Films like The Terminator (1984) and Blade Runner (1982) introduced audiences to worlds where machines not only served humans but could someday challenge or even supplant them, raising questions about identity, consciousness, and ethical responsibility.\n\n4. Time Travel and Alternate Realities\n\u2003• Time travel emerged as a compelling narrative device in the 1980s with films such as Back to the Future (1985). This theme allowed storytellers to explore the consequences of altering history, the interconnectedness of past, present, and future, and the fluid nature of reality itself.\n\n5. Encounters with the Alien Other\n\u2003• Many narratives delved into first contact scenarios, where humanity encountered beings from beyond Earth. Whether portraying aliens as benevolent (as in E.T. the Extra-Terrestrial (1982)) or as existential threats (as in Alien (1979)), these stories used extraterrestrial encounters to reflect on human identity, fear of the unknown, and the limits of our understanding of life.\n\n6. Post-Apocalyptic Visions and Environmental Collapse\n\u2003• Reflecting societal anxieties about nuclear war and ecological disaster, works like Mad Max (1979) and its sequels presented stark visions of a world stripped of order and nature’s revenge. These stories often had an undercurrent of warning about unsustainable progress and the human cost of environmental neglect.\n\nTogether, these themes illustrate how 1970s and 1980s sci‑fi media were not merely about futuristic thrills but were also deeply engaged with contemporary issues—examining the promises and perils of technological advancement, the ethics of artificial intelligence, and the ever-present anxieties about societal collapse. They continue to influence the genre by reminding us that science fiction is as much a reflection of our current hopes and fears as it is a playground for imagining lives beyond the present.",
"refusal": null,
"annotations": []
},
Expand All @@ -66,21 +70,21 @@
],
"usage": {
"prompt_tokens": 34,
"completion_tokens": 1610,
"total_tokens": 1644,
"completion_tokens": 1715,
"total_tokens": 1749,
"prompt_tokens_details": {
"cached_tokens": 0,
"audio_tokens": 0
},
"completion_tokens_details": {
"reasoning_tokens": 960,
"reasoning_tokens": 1088,
"audio_tokens": 0,
"accepted_prediction_tokens": 0,
"rejected_prediction_tokens": 0
}
},
"service_tier": "default",
"system_fingerprint": "fp_6c43dcef8c"
"system_fingerprint": "Sanitized"
}
}
],
Expand Down
Loading
Loading