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: Remove Memory property from SKContext #2079

Merged
merged 22 commits into from
Jul 20, 2023

Conversation

shawncal
Copy link
Member

Motivation and Context

Different skills and functions require different types of memory. Some skills might just need local, high-access stores while others need remote, authenticated access. Some are powered by embedding (vectors) indexed in the cloud, while others may be generating lightweight embeddings locally, on the fly. We cannot assume that memory is "one size fits all", or that even all the skills/functions in a single Plan use the same memory provider.

Description

Instead of surfacing one single 'Memory' instance to all skills, we're peeling this back and simplifying somewhat. In this first pass, the Memory property of SKContext is removed. The updated guidance will be to instantiate Skill classes with the memory provider you'd like them to have access to. This also opens the door for the TextMemorySkill to be registered several times in the kernel (with different aliases), granting access to different memory stores.

BREAKING CHANGE: though this is not expected to have a large impact, updates will be required for anyone:

  • using SKContext.Memory: will need to pass it into the function via an alternate route; likely Skill constructor injection
  • or those using the new SKContext() constructor and explicity passing the optional ISemanticTextMemory: creating a new SKContext with the new constructor is not advised, as this is typically done by a call through the Kernel. The new SKContext constructor is planned to be hidden in a future release.

Contribution Checklist

@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel and removed memory connector labels Jul 19, 2023
stephentoub
stephentoub previously approved these changes Jul 20, 2023
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Delete all the things :)

@shawncal shawncal closed this Jul 20, 2023
@shawncal shawncal reopened this Jul 20, 2023
@dmytrostruk dmytrostruk added PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge. labels Jul 20, 2023
@RogerBarreto
Copy link
Member

Food for thought.

I saw that the suggested implementation in this change for the MemorySkill was injecting the Memory when the skill was constructed not on usage/context basis.

Currently in our implementations there's no way to access our available AIServices based in the context of execution of a skill and IMHO we should think in a scenario where we should be able to get a service from within a Semantic Function depending on the context it is being executed.

i.e: Memory Skill might use different memories/text completion services when executed from different IKernels using the same ISkillCollection...

@shawncal
Copy link
Member Author

Food for thought.

I saw that the suggested implementation in this change for the MemorySkill was injecting the Memory when the skill was constructed not on usage/context basis.

Currently in our implementations there's no way to access our available AIServices based in the context of execution of a skill and IMHO we should think in a scenario where we should be able to get a service from within a Semantic Function depending on the context it is being executed.

i.e: Memory Skill might use different memories/text completion services when executed from different IKernels using the same ISkillCollection...

Yeah good call. To do this currently, you would create several TextMemorySkill instances and name them with different skill names (aliases).

The single memory provider is the core problem we're (starting to) fix with this PR. This change is one of likely several (to keep it "focused").

Additional changes ahead:

  • Moving memory skill into its own package, so it can evolve and grow faster than SK.Core
  • Consider having a collection of memory services registerable in Kernel, instead of just one (may or may not be needed)

@shawncal shawncal added this pull request to the merge queue Jul 20, 2023
Merged via the queue into microsoft:main with commit 9dd8604 Jul 20, 2023
@shawncal shawncal deleted the remove-context-memory branch July 20, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants