Skip to content

Integration for Azure.AI.Inference #9103

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 20 commits into from
May 12, 2025

Conversation

aaronpowell
Copy link
Contributor

@aaronpowell aaronpowell commented May 5, 2025

Description

Implementing an integration for the Azure AI Inference NuGet package.

For the method, I've added Azure to the name, even though the type being registered is a ChatCompletionsClient and it can be used without Azure (it's GitHub Models compatible). This aims to be consistent with the other integrations like Table.

Fixes #9011

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 5, 2025
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 5, 2025
Comment on lines 170 to 176
if (builder.DisableTracing)
{
return result;
}

var loggerFactory = services.GetService<ILoggerFactory>();
return new OpenTelemetryChatClient(result, loggerFactory?.CreateLogger(typeof(OpenTelemetryChatClient)));
Copy link
Member

Choose a reason for hiding this comment

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

Why does it do this instead of using the UseOpenTelemetry helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know it existed. I copied that from the OpenAI integration

Copy link
Member

Choose a reason for hiding this comment

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

Me as the author of the OpenAI integration: 😅

Copy link
Member

Choose a reason for hiding this comment

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

@RussKie RussKie mentioned this pull request May 5, 2025
18 tasks
/// <summary>
/// Gets or sets the name of the AI model to use for chat completions.
/// </summary>
public string? ModelName { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly isn't, I think it was from an older implementation

/// <summary>
/// Gets or sets the ID of the AI model to use for chat completions.
/// </summary>
public string? ModelId { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

The Azure.AI.Inference library calls this "deploymentId", right? We also use the term DeploymentId in AspireChatCompletionsClientBuilder. We should consolidate on a single term here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that in my refactor, will fix.

/// Gets or sets a boolean value that indicates whether the OpenTelemetry tracing is disabled or not.
/// </summary>
/// <remarks>
/// ServiceBus ActivitySource support in Azure SDK is experimental, the shape of Activities may change in the future without notice.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// ServiceBus ActivitySource support in Azure SDK is experimental, the shape of Activities may change in the future without notice.
/// Azure AI Inference client library ActivitySource support in Azure SDK is experimental, the shape of Activities may change in the future without notice.

/// Gets or sets the connection string used to connect to the AI Foundry account.
/// </summary>
/// <remarks>
/// If <see cref="ConnectionString"/> is set, it overrides <see cref="Endpoint"/> and <see cref="Credential"/>.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a bit misleading, since setting ConnectionString will populate Endpoint in ParseConnectionString below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the comment was lifted from here:

/// If <see cref="ConnectionString"/> is set, it overrides <see cref="ServiceUri"/> and <see cref="Credential"/>.

@aaronpowell aaronpowell requested a review from eerhardt May 10, 2025 00:10
@danmoseley
Copy link
Member

@eerhardt could you please review too?

@danmoseley danmoseley added this to the 9.3 milestone May 12, 2025
public static AspireChatCompletionsClientBuilder AddAzureChatCompletionsClient(
this IHostApplicationBuilder builder,
string connectionName,
Action<ChatCompletionsClientSettings>? configureClient = null,
Copy link
Member

Choose a reason for hiding this comment

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

configureClient => configureSettings

/// <summary>
/// The ID of the deployment in Azure AI Foundry.
/// </summary>
public string? DeploymentId { get; } = deploymentId;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this get used?

/// <param name="builder">An <see cref="AspireChatCompletionsClientBuilder" />.</param>
/// <param name="serviceKey">The service key with which the <see cref="IChatClient"/> will be registered.</param>
/// <returns></returns>
public static ChatClientBuilder AddKeyedChatClient(this AspireChatCompletionsClientBuilder builder, string? serviceKey)
Copy link
Member

Choose a reason for hiding this comment

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

serviceKey shouldn't be nullable if you are calling AddKeyedChatClient.

Copy link
Member

Choose a reason for hiding this comment

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

The builder has a service key:

serviceKey = serviceKey ?? builder.ServiceKey;

Use DeploymentId when calling AsIChatClient.

Make serviceKey required.
@sebastienros
Copy link
Member

I am doing the required changes, will merge after

@sebastienros sebastienros merged commit 524992e into dotnet:main May 12, 2025
174 checks passed
@danmoseley
Copy link
Member

danmoseley commented May 12, 2025

@joperezr we want this one even though it was just after 4pm.

@aaronpowell aaronpowell deleted the aaronpowell/issue-9011 branch May 13, 2025 00:03
@joperezr
Copy link
Member

@joperezr we want this one even though it was just after 4pm.

Sure, let's put up a backport PR then. The target is to have the final build by EoD today.

@joperezr
Copy link
Member

Oh nvm, just realized this made the cutoff

@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client Integration for Azure.AI.Inference
7 participants