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

Using Semantic Kernel Memory #297

Closed
wants to merge 3 commits into from

Conversation

aaronpowell
Copy link

This moves from the custom solution for vector searching on Postgres, instead using the SK memory feature to do it. Added some new SK dependencies for the memory (and PG memory store), then removed the Embedding column from the current data model, as there is a new table with all that in it.

Refactored the seed logic to load the memory store using the previously generated embeddings.

Changed the CatalogAPI route to use the ISemanticTextMemory search feature to search memory, rather than the custom SQL query. This does mean we don't get distance surfaced, also, pagination is currently lost and SK memory doesn't support that (we could roll that ourselves if we want).

Included a fix so that AOAI can be deployed (issue #280), and pgadmin for easier debugging of the data in the database.

Fixes #282

This moves from the custom solution for vector searching on Postgres, instead using the SK memory feature to do it. Added some new SK dependencies for the memory (and PG memory store), then removed the Embedding column from the current data model, as there is a new table with all that in it.

Refactored the seed logic to load the memory store using the previously generated embeddings.

Changed the CatalogAPI route to use the ISemanticTextMemory search feature to search memory, rather than the custom SQL query. This does mean we don't get distance surfaced, also, pagination is currently lost and SK memory doesn't support that (we could roll that ourselves if we want).

Included a fix so that AOAI can be deployed (issue dotnet#280), and pgadmin for easier debugging of the data in the database.
This means we do less code changes during testing, use dotnet user-secrets set EnableAI true instead
itemsOnPage = itemsWithDistance.Select(i => i.Item).ToList();
}
else
await foreach (var item in itemsWithDistance)
Copy link
Member

Choose a reason for hiding this comment

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

Noting that in addition to losing pagination and the distance, this performs an additional database roundtrip for each result (so for a page size of 5, this does one query via SearchMemoryAsync, and then 5 FindAsync invocations totaling 6)... That's not great in terms of performance.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

@aaronpowell can I ask for a bit more context on the goal of this PR?

I think that generally, eShop is supposed to provide a sample of how an actual real-world app would be written (at least I think of it that way). If I were to write such an app, and have chosen pgvector as my vector store solution, I think I'd code directly against it - as the code currently is - rather than introduce an abstraction, which in this particular case seems to cause more friction than helping... Of course, the abstraction would 100% make sense in other scenarios, e.g. when using the vector DB within the SK stack (where it would be necessary), but I'm not sure it makes sense here...

What do you think?

@adityamandaleeka adityamandaleeka deleted the branch dotnet:aspire-preview5 April 12, 2024 01:11
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.

None yet

3 participants