Skip to content
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.

Sign up for GitHub

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

Jump to bottom

.Net: add amazon nova support (text) only #10021

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

.Net: add amazon nova support (text) only #10021

wants to merge 3 commits into from

Conversation

EriksonBahr
Copy link

@EriksonBahr EriksonBahr commented Dec 19, 2024 •
edited
Loading

Motivation and Context

Add support for amazon nova.

Description

This PR adds amazon nova support. Also re-structures existing Titan files.

Contribution Checklist

@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Dec 19, 2024
@github-actions github-actions bot changed the title add amazon nova support (text) only .Net: add amazon nova support (text) only Dec 19, 2024
@EriksonBahr EriksonBahr mentioned this pull request Dec 19, 2024
Open
@EriksonBahr
Copy link
Author

@microsoft-github-policy-service agree

@EriksonBahr please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

RogerBarreto
RogerBarreto reviewed Jan 7, 2025
View reviewed changes
dotnet/src/Connectors/Connectors.Amazon/Bedrock/Core/Clients/BedrockTextGenerationClient.cs Outdated
@@ -76,7 +77,7 @@ internal async Task<IReadOnlyList<TextContent>> InvokeBedrockModelAsync(
try
{
var requestBody = this._ioTextService.GetInvokeModelRequestBody(this._modelId, prompt, executionSettings);
using var requestBodyStream = new MemoryStream(JsonSerializer.SerializeToUtf8Bytes(requestBody));
using var requestBodyStream = new MemoryStream(JsonSerializer.SerializeToUtf8Bytes(requestBody, new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull }));
Copy link
Member

Choose a reason for hiding this comment

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

Recommend using the options serializer as a static instance, avoiding instantiation each request.

Copy link
Author

Choose a reason for hiding this comment

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

done

dotnet/src/Connectors/Connectors.Amazon/Bedrock/Core/IBedrockTextGenerationService.cs Outdated

/// <summary>
/// Converts the streaming JSON into <see cref="IEnumerable{String}"/> for output.
/// </summary>
/// <param name="chunk">The payloadPart bytes provided from the streaming response.</param>
/// <returns><see cref="IEnumerable{String}"/> output strings.</returns>
internal IEnumerable<string> GetTextStreamOutput(JsonNode chunk);
internal IEnumerable<string> GetTextStreamOutput(string modelId, JsonNode chunk);
Copy link
Member

Choose a reason for hiding this comment

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

With the latest merge from my changes this signature needs to be updated to return StreamingChatMessageContent instead of string.

Copy link
Author

Choose a reason for hiding this comment

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

done

dotnet/src/Connectors/Connectors.Amazon/Bedrock/Core/Models/Amazon/AmazonService.cs Outdated

var requestBody = new TitanRequest.TitanTextGenerationRequest()
if (modelId.StartsWith("amazon.nova-", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

Strongly suggest to move this service to AmazonNovaService and not mix this with titan as the models (API contracts) are also different.

When moving to AmazonNovaService implementation, revert the extra modalId from the interface and feel free to rename the existing AmazonService to titan specific AmazonTitanService.

It's clear that the responsibility to know with model should be only in the factory for this perspective, avoiding bringing extra complexity to the service level.

Copy link
Author

Choose a reason for hiding this comment

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

done

dotnet/src/Connectors/Connectors.Amazon/Bedrock/Core/Models/Meta/MetaService.cs Outdated
@@ -75,7 +75,7 @@ public ConverseRequest GetConverseRequest(string modelId, ChatHistory chatHistor
}

/// <inheritdoc/>
public IEnumerable<string> GetTextStreamOutput(JsonNode chunk)
public IEnumerable<string> GetTextStreamOutput(string modelId, JsonNode chunk)
Copy link
Member

Choose a reason for hiding this comment

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

Note

Apply to all services

Revert this change as all the services were designed to not use the modelId information, worth having a specific service to handle the different Nova requests.

Suggested change
public IEnumerable<string> GetTextStreamOutput(string modelId, JsonNode chunk)
public IEnumerable<string> GetTextStreamOutput(JsonNode chunk)

Copy link
Author

Choose a reason for hiding this comment

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

done

dotnet/src/Connectors/Connectors.Amazon/Bedrock/Settings/AmazonNovaExecutionSettings.cs
/// Prompt execution settings for Nova Text Generation
/// </summary>
[JsonNumberHandling(JsonNumberHandling.AllowReadingFromString)]
public class AmazonNovaExecutionSettings : PromptExecutionSettings
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this specific Nova execution settings feels even stronger the need to have a dedicated concrete service for Nova models within Amazon's folder.

Copy link
Author

Choose a reason for hiding this comment

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

indeed, I re-organized stuff.

@RogerBarreto
Copy link
Member

@EriksonBahr Thanks for the amazing contribution, some extra conflicts needs to be addressed. Happy new year! 🎉

@RogerBarreto RogerBarreto added the PR: feedback to address Waiting for PR owner to address comments/questions label Jan 7, 2025
@RogerBarreto
Copy link
Member

@EriksonBahr, let me know if you plan on applying the suggestions above.

@EriksonBahr
Copy link
Author

@EriksonBahr, let me know if you plan on applying the suggestions above.

Should get this moving next week

@EriksonBahr EriksonBahr force-pushed the add-amazon-nova-support branch from 7a2e36d to 0d1bee7 Compare February 1, 2025 20:05
@EriksonBahr
Copy link
Author

I synced with main and re-done my commit following the updated interfaces. Also, updated the structure, indeed looks much better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RogerBarreto RogerBarreto RogerBarreto left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@RogerBarreto RogerBarreto

Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: feedback to address Waiting for PR owner to address comments/questions
Projects
Semantic Kernel
Status: Community PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EriksonBahr @RogerBarreto @markwallace-microsoft