-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
.Net: Adds an AI connector for Anthropic #3476
Conversation
…mantic-kernel into anthropic-ai-connector
Anthropic ai connector
Also ran complete build and all linters and formatters
Fixed solution file
@microsoft-github-policy-service agree company="Advocat AI" |
Resolve conflicts
That's cool @glorious-beard! Could this be extended to support functions as well? That would be an adaptation of this code from @mshumer Thanks in advance! |
|
||
private readonly ILogger? _log; | ||
private readonly HttpClient _httpClient; | ||
private readonly bool _disposeHttpClient; |
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.
Is there any situation where this needs to be disposed? If it came from externally, we musn't. And if we created it from the internal factory, we don't need to.
/// <summary> | ||
/// A chat completion connector for the Anthropic API. | ||
/// </summary> | ||
public class AnthropicChatCompletion : IChatCompletion, ITextCompletion, IDisposable |
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.
sealed
|
||
private HttpRequestMessage CreateHttpRequest(AnthropicRequest request) | ||
{ | ||
var json = JsonSerializer.Serialize(request); |
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'd be more efficient (skipping an intermediate string) to use SerializeToUTF8 and create a ByteArrayContent.
catch (HttpOperationException e) when (!string.IsNullOrWhiteSpace(e.ResponseContent)) | ||
{ | ||
this._log?.LogError(e, "Error sending request to Anthropic API: {Error}", e.ResponseContent); | ||
var error = JsonSerializer.Deserialize<AnthropicError>(e.ResponseContent!); |
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.
What if this throws an exception?
private static string ToPrompt(ChatHistory chat) | ||
{ | ||
var promptBuilder = new StringBuilder(); | ||
foreach (var message in chat.Where(message => message.Role == AuthorRole.User || message.Role == AuthorRole.Assistant)) |
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'd be more efficient to just do this as an if check at the beginning of the while loop rather than using a Where
/// </remarks> | ||
[JsonPropertyName("temperature")] | ||
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
public double? Temperature |
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.
Why is it nullable?
/// </remarks> | ||
[JsonPropertyName("top_p")] | ||
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
public double? TopP |
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.
Why is it nullable?
|
||
this._temperature = value; | ||
|
||
if (this._temperature == null) |
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.
Having one property set another like this is anti-pattern, as it means order of setting matters.
/// <summary> | ||
/// A successful response from the Anthropic API. | ||
/// </summary> | ||
public class AnthropicResponse |
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.
sealed
await Task.Yield(); | ||
yield return _message; | ||
} | ||
} |
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 don't understand the complexity of this method. Shouldn't the whole thing just be:
yield return new ChatMessage(AuthorRole.Assistant, this._response.Completion);
If the concern is the warning about lack of awaits, that can be suppressed.
@glorious-beard Thanks for your contribution. @RogerBarreto is working on our strategy to expand our AI Connectors. We will have an ADR available soon describing the approach we want to use and we will work with you to progress your PR. |
@glorious-beard we published our ADR regarding the process to add community connectors here Let me know if you or any contributor from the community have intention on continuing the work in this connector and I can create a dedicated feature branch moving forward. |
Motivation and Context
This allows solutions utilizing Claude or Claude 2 to use the Microsoft Semantic Toolkit.
It provides an additional AI connector that can be used for solutions where the generative AI is Anthropic, and also where multiple generative AIs may be working together.
Agnostic access to generative AI
.Net: Add Anthropic Support (I can contribute a PR for this) #3462
Description
This PR introduces a new chat completion object that implements
IChatCompletion
andITextCompletion
follows the same design patterns as the other AI connectors already in the kernel.Contribution Checklist