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

AI Templates - Fixes for Ollama and OpenAI scenarios #5855

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jmatthiesen
Copy link

@jmatthiesen jmatthiesen commented Feb 7, 2025

Fixes several issues for the different combinations of environment choices:

  • Fixed Azure AI Search integration by adding a check in the data ingestor to see if there are documents to delete before trying to delete them.
  • Adjusted the main chat prompt so that citations work with Ollama/Llama3.2 after recent changes - verified it works for all other supported AI model providers
  • Fixed using statements when pairing OpenAI with Azure AI Search (was missing a using Azure statement)
  • Removed the Attach Media button after a recent decision to not include that for now.
  • Fixed the conditionals in Chat.razor to include the proper code for Ollama.
Microsoft Reviewers: Open in CodeFlow

@@ -52,7 +56,7 @@
chatSuggestions?.Clear();
await chatInput!.FocusAsync();

#if (IsOllama)
@*#if (IsOllama)
Copy link
Author

Choose a reason for hiding this comment

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

This looks wrong, but it is the conditional syntax used by the templating engine for .razor files.

Copy link
Member

@SteveSandersonMS SteveSandersonMS Feb 7, 2025

Choose a reason for hiding this comment

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

Actually the syntax here isn't quite right because it will break the application if you run it directly from sources here. Note that in VS it's displayed like this:

image

That is, both branches of the if/else are greyed out, because as far as Razor syntax is concerned, this is all just one big comment block. So if you run the application like this, neither branch will execute and the app won't work. This will block us from being able to work on the application code directly. It's just by luck that it even still compiles.

I think what we need is the following syntax instead:

image

Now Razor itself sees the Ollama part of the code as being a comment, and the rest as being real code. So the app will still work if we run it directly from sources.

I think at least that the dotnet new templating system will also understand this as an if/else and emit the desired code in both cases. (Update: yes, I verified it does)

@@ -40,8 +40,11 @@ public async Task IngestDataAsync(IIngestionSource source)
{
logger.LogInformation("Processing {file}", modifiedDoc.Id);

await vectorCollection.DeleteBatchAsync(modifiedDoc.Records.Select(r => r.Id));

if (modifiedDoc.Records.Count > 0)
Copy link
Author

Choose a reason for hiding this comment

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

Without this, Azure AI Search integration breaks.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks.

The previous version of this check seemed to have the braces in a different place and was suppressing the CreateRecordsForDocumentAsync call as well, but maybe something weird happened in the Git merge.

In any case this version of the check looks totally safe!

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI.Ollama Line 80 78.25 🔻
Microsoft.Extensions.Caching.Hybrid Line 86 82.85 🔻
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 88 89
Microsoft.Extensions.AI.OpenAI 77 78
Microsoft.Extensions.AI.Abstractions 83 84

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

Comment on lines +26 to +31
Use the search tool to find relevant information. When you do this, end your
reply with citations in the special XML format:

<citation filename='string' page_number='number'>exact quote here</citation>

Always include the citation in your response if there are results.
Copy link
Member

Choose a reason for hiding this comment

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

Nbd but something quirky about the indentation. Not sure if it's a tabs/spaces thing:

Suggested change
Use the search tool to find relevant information. When you do this, end your
reply with citations in the special XML format:
<citation filename='string' page_number='number'>exact quote here</citation>
Always include the citation in your response if there are results.
Use the search tool to find relevant information. When you do this, end your
reply with citations in the special XML format:
<citation filename='string' page_number='number'>exact quote here</citation>
Always include the citation in your response if there are results.

Copy link
Member

Choose a reason for hiding this comment

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

In the GitHub UI, the word "Always" looks misaligned (indented one space too much) but in the code diff it's unclear why.

image

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