-
Notifications
You must be signed in to change notification settings - Fork 354
Clone options fully for ChatCompletion calls #861
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
Open
christothes
wants to merge
1
commit into
openai:main
Choose a base branch
from
christothes:chriss/chatclient_concurrency
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using
MemberwiseClonework here? It would be nice to not have to update this method every time a new property is added.We are currently using
MemberwiseClonein Responses:openai-dotnet/src/Custom/Responses/CreateResponseOptions.cs
Lines 125 to 130 in aa8b034
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
MemberwiseClonewill simply copy the reference to the existing collection.I'm thinking that
MemberwiseClonemight be sufficient because the intention here is not to makeChatCompletionOptionsthread-safe, but rather to simply avoid modifying it. In other words, the user's instance ofChatCompletionOptionsshould be unchanged after they callCompleteChat. The reason why this doesn't work today is because we're trampling over a few internal properties likeMessages,Stream, etc. But if weMemberwiseCloneit, we are free to modify the internal properties of the clone while keeping the user's instance intact.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
MemberwiseClonewould solve the issue because the user is specifically talking about theMessagesproperty, 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 theChatCompletionOptionsinstance 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 ofCompletChat, which we shouldn't do because theChatCompletionOptionsparameter is not arefparameter.Here's another way of thinking about this issue separate from concurrency:
ModelReaderWriter.Writeto serialize an instance ofChatCompletionOptionsCompleteChatand passes this instance ofChatCompletionOptionsas an argumentModelReaderWriter.Writeto serialize the same instance ofChatCompletionOptionsagain, 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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.