Skip to content

Proposal to use method options cloning #354

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
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

trrwilson
Copy link
Collaborator

@trrwilson trrwilson commented Feb 12, 2025

This applies a proposed pattern to handle a problem with request options mutation performed by client operations.

Problem details

  • The library "partializes" some REST request body payload models, emplacing required properties in method parameters or even the client parameterization -- new ChatClient(modelName).CompleteChat(messages, options) "promotes" the modelName and messages parts out of the options
  • In these situations, we need to reconstitute a unified request body payload for wire serialization, intermingling the user-provided (actually optional) options with the otherwise established required properties
  • Currently, we do this by directly modifying internal/private state on the options instances; this can produce unexpected and undesired behavior with out-of-band serialization/deserialization, e.g. as discovered in Azure for the method-applied max_completion_tokens workaround
  • Philosophically, it's "weird" that client methods quietly modify any state of a user-instantiated and provided options type; subtle mutation is by no means intuitive when providing options into an operation call

Longer term approaches

  • Protocol models such that we can merge these together seamlessly
  • Alternative format serializations to further hide/abstract details

Interim approach

  • Augment impacted options types to have an internal "GetClone()" that uses Object.MemberwiseClone() to form a shallow copy, coupled with a shallow copy of the additional binary data properties dictionary (so the operation can update the dictionary)
  • Each client method using an impacted options type gets an internal virtual TOptions CreatePerCallOptions(TOptions originalOptions, ...) that uses GetClone() and then performs the needed reconstitution (like emplacing model and messages for Chat Completions) on the clone, never modifying the original
  • Derived scenario clients (e.g. Azure's AzureChatClient) can override the CreatePerCallOptions to efficiency supplement treatment of the clone without needing to manage "clones of clones" or similar

@trrwilson trrwilson changed the base branch from user/travisw/streaming-audio-and-usage-fix to main February 13, 2025 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant