microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.96k stars 3.27k forks source link

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

Closed JRA-zachpennington closed 1 year ago

JRA-zachpennington commented 1 year ago

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

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

On May 1, AddOpenAITextEmbeddingGenerationService was refactored: serviceId parameter was removed https://github.com/microsoft/semantic-kernel/commit/57cbc93d05924ac8c2ba323d198148c0a1484094#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.

JRA-zachpennington commented 1 year ago

PR https://github.com/microsoft/semantic-kernel/pull/2946

JRA-zachpennington commented 1 year ago

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

JRA-zachpennington commented 1 year ago

Does anyone have opinions on these code comments please?

https://github.com/microsoft/semantic-kernel/blob/4a2cf70c9fb254b73b9fac1f99493f62650ce7fa/dotnet/samples/KernelSyntaxExamples/Example14_SemanticMemory.cs#L33

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).


https://github.com/microsoft/semantic-kernel/blob/4a2cf70c9fb254b73b9fac1f99493f62650ce7fa/dotnet/samples/KernelSyntaxExamples/Example14_SemanticMemory.cs#L129

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 commented 1 year ago

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

JRA-zachpennington commented 1 year ago

Thank you @matthewbolanos and @dmytrostruk 💪

JRA-zachpennington commented 1 year ago

I am available to help as required

dmytrostruk commented 1 year ago

Closing as resolved in this PR: https://github.com/microsoft/semantic-kernel/pull/3100. @JRA-zachpennington Thanks for reporting!