Skip to content

[System.ClientModel] Support all properties needed to create a client in ClientCache and consolidate ClientConnection.Credential usage #49315

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 19 commits into from
Apr 30, 2025

Conversation

ShivangiReja
Copy link
Member

@ShivangiReja ShivangiReja commented Apr 9, 2025

Task 1:
Enhanced the ClientCache to support client reuse based on all necessary properties required to create a client instance.

  • Updated the internal cache key to account for differences in client properties including client options by adding an IEquatable<object> identifier.
  • This ensures that multiple clients of the same type and same ID but with different option instances (e.g., AzureOpenAIClientOptions) are cached and retrieved independently.

Example:

project.GetAzureOpenAIChatClient("a", options1);
project.GetAzureOpenAIChatClient("a", options2);

Both clients are now stored as distinct entries if options1 != options2.

  • Included a targeted test case validating the correct behavior for distinct option instances.

Task 2:
Simplified ClientConnection by removing the ApiKeyCredential property.

  • All authentication details are now routed through the unified Credential property.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Apr 9, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

Azure.AI.OpenAI
System.ClientModel
Azure.Search.Documents
Azure.AI.Inference
Azure.AI.Projects

@KrzysztofCwalina
Copy link
Member

It would be good if ClientConnection.Credential was typed as AuthenticationTokenProvider and not Object.
But I am not sure if we have time to coordinate. Maybe we just rename ClientConnection.Credential to ClientConnection.TokenProvider?

@christothes

@KrzysztofCwalina
Copy link
Member

@ShivangiReja , @christothes, should we remove ClientConnection.ApiKeyCredential and use the ClientConnection.Credential property for API key case?

@ShivangiReja ShivangiReja force-pushed the shreja/SCM_ClientCache_Options branch from 84d7f0b to cb0ba90 Compare April 29, 2025 07:06
@ShivangiReja ShivangiReja changed the title [System.ClientModel] Add support for client options in ClientCache [System.ClientModel] Support all properties needed to create a client in ClientCache and consolidate ClientConnection.Credential usage Apr 29, 2025
@ShivangiReja ShivangiReja marked this pull request as ready for review April 29, 2025 19:24
@ShivangiReja ShivangiReja requested review from dargilco, glharper and a team as code owners April 29, 2025 19:24
@azure-sdk
Copy link
Collaborator

azure-sdk commented Apr 29, 2025

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.AI.OpenAI
System.ClientModel
Azure.Search.Documents
Azure.AI.Inference
Azure.AI.Projects

@ShivangiReja ShivangiReja merged commit eac4d87 into Azure:main Apr 30, 2025
18 checks passed
nick863 pushed a commit that referenced this pull request May 8, 2025
… in ClientCache and consolidate ClientConnection.Credential usage (#49315)

[System.ClientModel] Add support for client options in ClientCache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants