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.

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

Use just-built packages in the AI chat template by default #6096

Merged
merged 19 commits into from
Mar 14, 2025

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Mar 12, 2025

This PR makes the following changes to the chat template:

  1. By default, reference just-built MEAI package versions
  2. Use the Microsoft.EntityFrameworkCore.Sqlite version defined in Versions.props instead of being hard-coded
  3. Add new MSBuild properties that can be specified to utilize public versions of template dependencies instead of just-built versions
  4. Generate development-only files that allow the templates to utilize locally produced NuGet packages
  5. Adapt to the recent breaking changes in MEAI.
Microsoft Reviewers: Open in CodeFlow

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner March 12, 2025 22:18
@github-actions github-actions bot added the area-ai-templates Microsoft.Extensions.AI.Templates label Mar 12, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jeffhandley jeffhandley requested a review from a team as a code owner March 14, 2025 05:17
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

@MackinnonBuck I merged from main, including the snapshot tests, and tried to work through getting everything green again. I've hit a couple of snags that we'll have to figure out though.

  1. The snapshot is specific to whether we're using the pinned version or the just-built version, with the NuGet.config and Directory.Build.Targets files either included or excluded.
  2. The snapshot bakes in the just-built version number, which is different between local development and CI.
  3. Question: Is there something in place already where for the official build pipeline we would be able to toggle which mode to build with, or would we just update the targets file to toggle which is the default based on the next release's approach?

To get around the snapshot issues in CI and since we expect our next template release to use the MEAI and Sqlite packages released earlier this week, I swapped it to use the pinned versions by default, and I updated to the versions released this week. This got tests passing locally and in CI.

I hope you don't mind I pushed those changes on into your branch. Feel free to rewind the history and force-push if you want to discard my commits.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Mar 14, 2025

Thanks for adding those updates, @jeffhandley!

  1. The snapshot is specific to whether we're using the pinned version or the just-built version, with the NuGet.config and Directory.Build.Targets files either included or excluded.

Ideally the snapshot would ignore files that don't get included in the actual .nupkg, but the tests just treat the source files in the repo as the template content and ignore what gets specified as <Content> in Microsoft.Extensions.AI.Templates. We could try to figure out an alternative that somehow takes the build output of M.E.AI.Templates into account.

  1. The snapshot bakes in the just-built version number, which is different between local development and CI.

Yeah, also unfortunate. If we did my suggestion above, maybe the tests could build M.E.AI.Templates and specify MSBuild props that normalize that version number somehow (maybe even using a fake one). Or, I think there's a way for the tests to do some post-processing on the output (here); maybe we could do the normalization there. In fact, maybe the template testing infrastructure allows us to completely remove files from the snapshot (like NuGet.config and Directory.Build.targets), which would also solve 1.).

  1. Question: Is there something in place already where for the official build pipeline we would be able to toggle which mode to build with, or would we just update the targets file to toggle which is the default based on the next release's approach?

If the official build pipeline allows us to customize the build command on the fly, we could specify /p:TemplateUsePinnedMicrosoftExtensionsAIVersion or any of the other properties in GeneratedContent.targets. Otherwise we'd have to edit the .targets file manually. Maybe someone more familiar with that area of the build infrastructure would know better than I do. @RussKie?

MackinnonBuck and others added 3 commits March 14, 2025 10:13

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ationTests/Snapshots/aichatweb.Basic.verified/aichatweb/Components/Pages/Chat/Chat.razor

Co-authored-by: Jeff Handley <[email protected]>
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner March 14, 2025 18:19
@MackinnonBuck
Copy link
Member Author

Updates:

  • Snapshots now remove the -ci and -dev version suffixes
  • Snapshots now ignore content that doesn't get included in the M.E.AI.Templates NuGet package
  • The templates now utilize a new AddMessages() API

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@MackinnonBuck MackinnonBuck merged commit 2810fc5 into main Mar 14, 2025
6 checks passed
@MackinnonBuck MackinnonBuck deleted the mbuck/chat-template-build-improvements branch March 14, 2025 22:49
@RussKie
Copy link
Member

RussKie commented Mar 17, 2025

@MackinnonBuck I merged from main, including the snapshot tests, and tried to work through getting everything green again. I've hit a couple of snags that we'll have to figure out though.

  1. The snapshot is specific to whether we're using the pinned version or the just-built version, with the NuGet.config and Directory.Build.Targets files either included or excluded.
  2. The snapshot bakes in the just-built version number, which is different between local development and CI.
  3. Question: Is there something in place already where for the official build pipeline we would be able to toggle which mode to build with, or would we just update the targets file to toggle which is the default based on the next release's approach?

To get around the snapshot issues in CI and since we expect our next template release to use the MEAI and Sqlite packages released earlier this week, I swapped it to use the pinned versions by default, and I updated to the versions released this week. This got tests passing locally and in CI.

I hope you don't mind I pushed those changes on into your branch. Feel free to rewind the history and force-push if you want to discard my commits.

Thanks for adding those updates, @jeffhandley!

  1. The snapshot is specific to whether we're using the pinned version or the just-built version, with the NuGet.config and Directory.Build.Targets files either included or excluded.

Ideally the snapshot would ignore files that don't get included in the actual .nupkg, but the tests just treat the source files in the repo as the template content and ignore what gets specified as <Content> in Microsoft.Extensions.AI.Templates. We could try to figure out an alternative that somehow takes the build output of M.E.AI.Templates into account.

  1. The snapshot bakes in the just-built version number, which is different between local development and CI.

Yeah, also unfortunate. If we did my suggestion above, maybe the tests could build M.E.AI.Templates and specify MSBuild props that normalize that version number somehow (maybe even using a fake one). Or, I think there's a way for the tests to do some post-processing on the output (here); maybe we could do the normalization there. In fact, maybe the template testing infrastructure allows us to completely remove files from the snapshot (like NuGet.config and Directory.Build.targets), which would also solve 1.).

  1. Question: Is there something in place already where for the official build pipeline we would be able to toggle which mode to build with, or would we just update the targets file to toggle which is the default based on the next release's approach?

If the official build pipeline allows us to customize the build command on the fly, we could specify /p:TemplateUsePinnedMicrosoftExtensionsAIVersion or any of the other properties in GeneratedContent.targets. Otherwise we'd have to edit the .targets file manually. Maybe someone more familiar with that area of the build infrastructure would know better than I do. @RussKie?

I think this discussion directly relates and is the cause of #6128.

Can you help me understand why the versions are different between local development and CI - or more specifically between the public and the internal CI?

@MackinnonBuck
Copy link
Member Author

Can you help me understand why the versions are different between local development and CI - or more specifically between the public and the internal CI?

@RussKie, the reason is because the version suffix when building locally is -dev, but when building in CI it's -ci. I had some changes in this PR that removed the suffix from the snapshots as an attempt to "normalize" the version number. But it looks the internal branches use yet another suffix (-preview.x.x.x).

We could probably update the test to also scrub versions matching that suffix as well. Or we could do something possibly more robust and find the exact <PackageReference />s for packages defined within the repo and completely scrub the Version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ai-templates Microsoft.Extensions.AI.Templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants