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

Automate chat template JS dependency updates #5853

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Feb 7, 2025

Automate chat template JS dependency updates

Following are the main changes in this PR:

  1. Add a package.json to specify project template dependencies and how they should be copied into template content
    • Note that this is only a development-time dependency and doesn't ship with the templates. That is, we're not adding back a dependency on NPM for the shipping templates.
  2. Add a copyDependency.mjs script that carries out the actual dependency copying.
    • It also updates the package version in the README file
  3. Reduce the number of assets required for PDF viewing (more info below).

PDF viewer changes

The pdfjs dependency was trickier than the others, because its NPM package (pdfjs-dist) does not include the built-in PDF viewer that was being utilized by the chat template. There are numerous issues in the pdf.js GitHub repo asking for the viewer to be included, but the maintainers have stated that they don't want people to use the PDF viewer "as-is" in actual code:

The viewer is built on the display layer and is the UI for the PDF viewer in Firefox and the other browser extensions within the project. It can be a good starting point for building your own viewer. However, we do ask if you plan to embed the viewer in your own site, that it not just be an unmodified version. Please re-skin it or build upon it.

Given this statement and the fact that it was intentionally made difficult to use the built-in PDF viewer, it's probably best to find an alternative.

Thankfully, it's pretty straightforward to recreate a super minimal PDF viewer using pdf.js directly. I've added two assets to the wwwroot folder in the template that define a custom viewer:

  • pdf_viewer.html
  • pdf_viewer.js

These assets won't get updated automatically, but they probably won't need to be anyway unless there's a significant breaking change in pdf.js.

It's pretty minimal, but I don't think we need anything more advanced for a template:
pdf-viewer

One advantage to this is that we can significantly reduce the number of assets required for PDF viewing (all the images except for the loading icon can be removed).

In the future, we could even replace pdf_viewer.html with a Blazor component that displays the citation in a modal dialog. But I'm hoping we would address that in a follow-up.

Future work

This PR does not add a CI pipeline to update template dependencies. It's still "manual" in that you have to clone the repo and run these two commands from the src/ProjectTemplates folder:

npm install
npm run copy-dependencies

However, this is how it works in the ASP.NET Core repo as well, and I'd imagine it saves about 90% of the effort required to update dependencies.

This PR also doesn't do anything to automate .NET dependency updates. If we think this would be helpful, I'll address it in a follow-up PR to keep this one focused.

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

Microsoft Reviewers: Open in CodeFlow

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 82.77 🔻
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=943589&view=codecoverage-tab

@SteveSandersonMS
Copy link
Member

Thankfully, it's pretty straightforward to recreate a super minimal PDF viewer using pdf.js directly. I've added two assets to the wwwroot folder in the template that define a custom viewer:

This is wonderful. I love it. Much better than having that massive dump of files we had before. Brilliant.

This PR also doesn't do anything to automate .NET dependency updates. If we think this would be helpful, I'll address it in a follow-up PR to keep this one focused.

I definitely think that would that would be helpful, even more so than the JS files since the .NET deps are going to rev continually. Hopefully we could use an equivalent technique to what we do in dotnet/aspnetcore so we automatically use whatever version this host repo itself depends on, so the automatic dependency flow keeps things up-to-date for us: https://github.com/dotnet/aspnetcore/blob/main/src/ProjectTemplates/GenerateContent.targets

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI.Ollama Line 80 78.11 🔻
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.Caching.Hybrid 86 87
Microsoft.Extensions.AI.AzureAIInference 91 92
Microsoft.Extensions.AI 88 89
Microsoft.Extensions.AI.Abstractions 83 85

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

@SteveSandersonMS SteveSandersonMS force-pushed the mbuck/automate-template-dep-updates branch from 73ad687 to b93d183 Compare February 10, 2025 11:30
…tWithCustomData/ChatWithCustomData.Web/wwwroot/pdf_viewer/viewer.mjs
@SteveSandersonMS SteveSandersonMS enabled auto-merge (squash) February 10, 2025 11:33
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 82.92 🔻
Microsoft.Extensions.AI.Ollama Line 80 78.11 🔻
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 85
Microsoft.Extensions.AI.AzureAIInference 91 92
Microsoft.Extensions.AI 88 89

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

@SteveSandersonMS SteveSandersonMS merged commit 66bc675 into main Feb 10, 2025
6 checks passed
@SteveSandersonMS SteveSandersonMS deleted the mbuck/automate-template-dep-updates branch February 10, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants