Skip to content

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Feb 3, 2025

Microsoft Reviewers: Open in CodeFlow

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI.Ollama Line 80 78.25 🔻
Microsoft.Gen.MetadataExtractor Line 98 57.35 🔻
Microsoft.Gen.MetadataExtractor Branch 98 62.5 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.AI.OpenAI 77 78
Microsoft.Extensions.AI 88 89
Microsoft.Extensions.AI.Abstractions 83 84

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=938045&view=codecoverage-tab

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review February 3, 2025 16:08
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner February 3, 2025 16:08
@SteveSandersonMS
Copy link
Member Author

I'm working through the build failure by mirroring the required packages to dotnet-public.

@SteveSandersonMS
Copy link
Member Author

@stephentoub I know you approved before but that was before I added most of the content. Do you want to look at it again or can I interpret the approval as still in effect?

We're not ready for a detailed code review on the template yet as they are still so much in flux. But several people need to collaborate on them so it's valuable to get the work-in-progress state into this repo. So my proposal is we merge this now and once @MackinnonBuck, @jmatthiesen etc are happy with the state of it, we do a more detailed review.

"type": "bind",
"binding": "HostIdentifier"
},
"AiServiceProvider": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to communicate that the choice made here can easily be changed later, e.g. the Ollama section suggests that's using llama3.2, but changing that later is changing a single string. Or is that expected to be implicitly understood for templates, that choices made at setup are easily alterable later?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to resolve all the conversations before CI will let me merge this. I'll unresolve after it merges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is that expected to be implicitly understood for templates, that choices made at setup are easily alterable later?

In general no. Choices made at setup can be arbitrarily impactful. For example in the ASP.NET Core templates, whether or not you enable auth makes a huge difference and it's very much not easy to alter it later.

I don't think we need to provide specific guidance in the template wizard about what choices are easily changeable later. At least, it's not a thing we normally do.

<PackageReference Include="Azure.AI.OpenAI" Version="2.1.0" />
#else -->
<PackageReference Include="Azure.AI.OpenAI" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.AI.OpenAI" Version="9.1.0-preview.1.25064.3" />
Copy link
Member

Choose a reason for hiding this comment

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

Can we consolidate these versions into one place so that as we rev the template with newer M.E.AI versions, we only need to change one place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to resolve all the conversations before CI will let me merge this. I'll unresolve after it merges.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Feb 4, 2025

Choose a reason for hiding this comment

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

Tracked in https://github.com/dotnet/ai-private-planning/issues/276

It's not straightforwards to make these versions update automatically since arcade/darc type updates have nothing to do with template contents. We would need to create a custom build step that does some kind of parameter replacement, similar to what's done for ASP.NET Core templates: https://github.com/dotnet/aspnetcore/blob/main/src/ProjectTemplates/GenerateContent.targets

<PackageReference Include="Microsoft.Extensions.AI" Version="9.1.0-preview.1.25064.3" />
<PackageReference Include="Microsoft.SemanticKernel.Core" Version="1.34.0" />
<PackageReference Include="PdfPig" Version="0.1.9" />
<PackageReference Include="System.Linq.Async" Version="6.0.1" />
Copy link
Member

Choose a reason for hiding this comment

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

We should have this use the new System.Linq.AsyncEnumerable package as soon as the preview 1 version of it is on nuget.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to resolve all the conversations before CI will let me merge this. I'll unresolve after it merges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's not available yet, tracked in https://github.com/dotnet/ai-private-planning/issues/277

<!--#endif -->
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.0" />
<PackageReference Include="Microsoft.Extensions.AI" Version="9.1.0-preview.1.25064.3" />
<PackageReference Include="Microsoft.SemanticKernel.Core" Version="1.34.0" />
Copy link
Member

Choose a reason for hiding this comment

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

SK has already bumped to 1.35. Is there any way we can tie these versions to what's automatically updated by dependabot?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to resolve all the conversations before CI will let me merge this. I'll unresolve after it merges.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub
Copy link
Member

@stephentoub I know you approved before but that was before I added most of the content. Do you want to look at it again or can I interpret the approval as still in effect?

We're not ready for a detailed code review on the template yet as they are still so much in flux. But several people need to collaborate on them so it's valuable to get the work-in-progress state into this repo. So my proposal is we merge this now and once @MackinnonBuck, @jmatthiesen etc are happy with the state of it, we do a more detailed review.

Feel free to merge to unblock collaboration. I skimmed and left a few comments.

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI.Ollama Line 80 78.25 🔻
Microsoft.Gen.MetadataExtractor Line 98 57.35 🔻
Microsoft.Gen.MetadataExtractor Branch 98 62.5 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.AI.Abstractions 83 84
Microsoft.Extensions.AI 88 89
Microsoft.Extensions.AI.OpenAI 77 78

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=938562&view=codecoverage-tab

@SteveSandersonMS SteveSandersonMS merged commit fa798ad into main Feb 3, 2025
6 checks passed
@SteveSandersonMS SteveSandersonMS deleted the stevesa/chat-template branch February 3, 2025 20:10
@jeffhandley jeffhandley added the area-ai-templates Microsoft.Extensions.AI.Templates label Mar 7, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ai-templates Microsoft.Extensions.AI.Templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants