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

.Net: Samples, Example 14: Fix Parameters WithOpenAITextEmbeddingGenerationService #2947

Closed
JRA-zachpennington opened this issue Sep 22, 2023 · 7 comments
Assignees
Labels
.NET Issue or Pull requests regarding .NET code

Comments

@JRA-zachpennington
Copy link

Describe the bug
Update Example 14 (dotnet, KernelSyntaxExamples) to conform to the proper use of WithOpenAITextEmbeddingGenerationService. The old serviceId parameter is no longer required here.

To Reproduce
Steps to reproduce the behavior:

  1. Run Example14 as instructed here: https://github.com/microsoft/semantic-kernel/tree/main/dotnet/samples/KernelSyntaxExamples#readme
  2. Let the 2 sections of examples run:
    Notice, the AzureCognitiveSearchMemoryStore example works.
    But then the VolatileMemoryStore example produces the error:
    HTTP 401 (Unauthorized)
    Azure.RequestFailedException: Incorrect API key provided: text-emb**********-002
    This error results from the modelId "text-embedding-ada-002" being passed instead of the API key.

Expected behavior
The VolatileMemoryStore example should work the same as the first example.
We simply remove the serviceId parameter, as it is no longer required.

Screenshots
image

Platform

  • OS: Windows 10 Home
  • IDE: Microsoft Visual Studio Community 2022 (64-bit) - Version 17.7.4
  • Language: C#
  • Source: SK-dotnet repo (544b6c1); KernelSyntaxExamples project.

Additional context
Note, the serviceId refactor goes way back to before PR #993 (serviceregistry)

On May 1, AddOpenAITextEmbeddingGenerationService was refactored:
serviceId parameter was removed
57cbc93#diff-a95fd1663266edb1b8f7fa87865bfd8e2750bb1c7b92643255b1b39d609ac862L153

At the time of the refactor (May 1), AzureCognitiveSearchMemory did not require a TextEmbedding service
But the VolatileMemoryStorage did require a TextEmbeddingGenerationService. Back then, serviceId was required as the first parameter.
https://github.com/microsoft/semantic-kernel/blame/5e4e0edfaa94b96f008cbd3f8669056e7a145ffa/dotnet/samples/KernelSyntaxExamples/Example14_SemanticMemory.cs

Later, TextEmbeddingGenerationSerivce was required for AzureCognitiveSearch also.
So TextEmbeddingGenerationService was added to the first example:
https://github.com/microsoft/semantic-kernel/blame/52fc85af7805d6923a4544594cf493345b9dbb7c/dotnet/samples/KernelSyntaxExamples/Example14_SemanticMemory.cs

In this same commit, the serviceId paramter was removed from the VolatileMemoryStorage example:
https://github.com/microsoft/semantic-kernel/blame/52fc85af7805d6923a4544594cf493345b9dbb7c/dotnet/samples/KernelSyntaxExamples/Example14_SemanticMemory.cs

The serviceId parameter was added back to Example 14 on Jul 17. (Maybe this line was accidentally added in the merge?)

THANK YOU to all the contributors at Microsoft & the community.

@shawncal shawncal added .NET Issue or Pull requests regarding .NET code triage labels Sep 22, 2023
@github-actions github-actions bot changed the title dotnet Samples, Example 14: Catch up to .Net: dotnet Samples, Example 14: Catch up to Sep 22, 2023
@JRA-zachpennington
Copy link
Author

PR #2946

@JRA-zachpennington
Copy link
Author

This was a very small change, so I believe this PR handles it

@JRA-zachpennington JRA-zachpennington changed the title .Net: dotnet Samples, Example 14: Catch up to .Net: dotnet Samples, Example 14: Fix Parameters WithOpenAITextEmbeddingGenerationService Sep 22, 2023
@JRA-zachpennington JRA-zachpennington changed the title .Net: dotnet Samples, Example 14: Fix Parameters WithOpenAITextEmbeddingGenerationService .Net: Samples, Example 14: Fix Parameters WithOpenAITextEmbeddingGenerationService Sep 22, 2023
@JRA-zachpennington
Copy link
Author

JRA-zachpennington commented Sep 22, 2023

Does anyone have opinions on these code comments please?

* Azure Cognitive Search automatically indexes your data semantically, so you don't

instead,

This example leverages Azure Cognitive Search to provide SK with semantic memory.
You can build your own semantic memory combining an Embedding Generator
with a Memory storage that supports search by similarity (ie semantic search).


* When using Azure Cognitive Search the data is automatically indexed on write.

instead, I recommend just removing the comment lines about indexing and embedding here.


My source is the MS docs: https://learn.microsoft.com/en-us/azure/search/vector-search-how-to-generate-embeddings

Cognitive Search doesn't host vectorization models, so one of your challenges is creating embeddings for query inputs and outputs.

@matthewbolanos
Copy link
Member

Looping in @dmytrostruk to help with the PR review :)

@JRA-zachpennington
Copy link
Author

Thank you @matthewbolanos and @dmytrostruk 💪

@JRA-zachpennington
Copy link
Author

I am available to help as required

@dmytrostruk dmytrostruk moved this from Bugs to Sprint: In Progress in Semantic Kernel Oct 18, 2023
@dmytrostruk
Copy link
Member

Closing as resolved in this PR: #3100.
@JRA-zachpennington Thanks for reporting!

@github-project-automation github-project-automation bot moved this from Sprint: In Progress to Sprint: Done in Semantic Kernel Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants