Repository navigation
.Net: Revisit namespaces and type placement #3151
Description
Important
Labeled Urgent because it may require a breaking change.
(Metapoint: Try rewriting all the examples without using var and seeing how many namespaces need to be imported to successfully compile.)
- (1)
To do anything meaningful with memory now I need both:
using Microsoft.SemanticKernel.Memory; // for ISemanticTextMemory, unless I rely exclusively on var
using Microsoft.SemanticKernel.Plugins.Memory; // for MemoryBuilder and friendsplus a using for whatever store I'm using it if it's not the VolatileMemoryStore, e.g.
using Microsoft.SemanticKernel.Connectors.Memory.AzureCognitiveSearch;That's three usings just to create an ISemanticTextMemory. Can these be consolidated?
- (2)
To do anything meaningful with planning, I now need both:
using Microsoft.SemanticKernel.Planners; // for the actual planner
using Microsoft.SemanticKernel.Planning; // for PlanThe difference between "Planners" and "Planning" is also very subtle. Can these be consolidated?
-
(3)
It looks like Microsoft.SemanticKernel.Diagnostics exists now just forSKException,HttpOperationException, andTelemetry. Do we need this namespace at all? Can we move these types somewhere else, like movingSKExceptionto the rootMicrosoft.SemanticKernelnamespace? -
(4)
Similarly, can we consolidate Microsoft.SemanticKernel.Events with something else? "Events" is pretty broad; the types here are specific to pre/post hooks with plan execution, right? -
(5)
Why do we need Microsoft.SemanticKernel.Plugins.Core? Can't those just move down into Plugins? -
(6)
We should also move "extensions" types to the namespace with the types they extend. For example, it looks like ImageGenerationServiceExtensions, TextEmbeddingServiceExtensions, TextCompletionServiceExtensions, only have extension methods and those only apply toIAIServicein Microsoft.SemanticKernel.Services... can they move there? And can they be consolidated into an AIServicesExtensions class? AndOpenAIModelResultExtensionshas methods that operate only onMicrosoft.SemanticKernel.Orchestration.ModelResult... can it move there? -
(7)
What is MemoryConfiguration and why does it live in the root namespace? -
(8)
The main thing I do with anIKernel(which lives in the root namespace) isInvokeAsyncon it, and to get a useful result from it, I need to access itsKernelResult, which now lives inMicrosoft.SemanticKernel.Orchestration. I suggest at leastKernelResultandFunctionResultmove to be next toIKernelandISKFunctionin the root namespace. And at that point, theOrchestrationnamespace contains very little and should likely be consolidated with something else. -
(9)
Microsoft.SemanticKernel.TemplateEngine contains just a single type (IPromptTemplateEngine) and then a subBasicnamespace. Can everything inBasicjust move up? -
(10)
Microsoft.SemanticKernel.Reliabilityappears to solely be about HTTP and retries. Why is this separate fromMicrosoft.SemanticKernel.Http? Can we consolidate them? -
(11)
.Net: PromptTemplateConfig and IPromptTemplate namesapce refactor #3173
Metadata
Metadata
Labels
Type
Projects
- Status
Activity
[-]Revisit namespaces and type placement[/-][+].Net: Revisit namespaces and type placement[/+]dmytrostruk commentedon Oct 11, 2023
Stephen Toub (@stephentoub) Thanks for very useful feedback!
(1)
Microsoft.SemanticKernel.MemoryandMicrosoft.SemanticKernel.Plugins.Memory- this is just transient phase, while we marked Memory related methods and properties in Kernel asObsolete. In next versions, all memory-related functionality will be placed inPlugins.Memorypackage and use one root namespaceMicrosoft.SemanticKernel.Plugins.Memory.Regarding Memory Stores (
Microsoft.SemanticKernel.Connectors.Memory.AzureCognitiveSearch) - they are placed in their own packages, so namespace is based onMicrosoft.SemanticKernel+ folder path of concrete Memory store. We either have to place all of them under one package or use the same namespace for all of them (does it respect namespace naming conventions?). Which way is better here?(2-5) I think these can be fixed.
(6)
In case of
OpenAIModelResultExtensions- it contains extension methods forMicrosoft.SemanticKernel.Orchestration.ModelResult, but the logic of these methods is applicable only to OpenAI-related functionality. In this case shouldn't it be placed underMicrosoft.SemanticKernel.Connectors.AI.OpenAInamespace? (to align namespace with actual class location).(7)
MemoryConfigurationwill be removed in future versions when we remove Memory-related functionality from Kernel. I marked its methods asObsolete, but I should probably also mark asObsoletethe class itself.(8-11) I think these can be fixed.
madsbolaris commentedon Oct 12, 2023
Will need some additional feedback from Stephen so we can split this into multiple issues and provide a better estimate.
stephentoub commentedon Oct 13, 2023
I certainly wouldn't say they all need to be in the same package; that's both limiting and infeasible for something that's meant to have external plug-ins all playing in the same design. If we believe conflicts in naming of types between plug-ins is going to happen, then they should be in their own namespaces, though my expectation was that it would be rare to have such conflicts and we'd actively encourage folks to use a prefix on their type names, e.g. it'd be
Microsoft.SemanticKernel.Connectors.Memory.AzureCognitiveSearchMemoryStorerather thanMicrosoft.SemanticKernel.Connectors.Memory.AzureCognitiveSearch.MemoryStore, at which point there shouldn't be conflicts.That said, my comment was more concerned about consolidating from 3 down to 2 rather than from 3 down to 1, and from your comments it sounds like that's already in the works, which is good.
My main objection was having them in the root namespace as they currently are. If we want to place them in Microsoft.SemanticKernel.Connectors.AI.OpenAI, that's ok, but that will also mean that if you haven't otherwise imported that namespace, you may not see them in IntelliSense in VS while exploring and seeing what you can add. I'd need to double-check, but I think ASP.NET typically includes its methods for adding into IServiceCollection in the same namespace that defines IServiceCollection, for example: that way, when you add the relevant package, and you already have the namespace that brings in IServiceCollection, you also have the namespace that brings in the extensions on it.
On that note, it's worth noting that ASP.NET generally also calls such methods "Add" not "With". SK should consider aligning with that as well, simply for familiarity for devs used to using ASP.NET.
dmytrostruk commentedon Oct 13, 2023
Yes, this will definitely be fixed. My main concern was about putting it to the namespace where original type is located (
Microsoft.SemanticKernel.Orchestration.ModelResult) instead ofMicrosoft.SemanticKernel.Connectors.AI.OpenAI, where it's more applicable based on extension method logic and the fact that it lives inConnectors.AI.OpenAIpackage.I think it's a little bit different case, and here it makes sense, because usually you work with
IServiceCollectionto register services from different libraries, and if eachIServiceCollectionextension method will live in its library namespace, yourStartup.csfile will be massively overloaded with different namespaces. So, in this pattern it makes sense to keep namespace of original type we extend, and we can actually apply the same strategy to ourKernelBuilderand all its extensions.But if we extend some base type (like
ModelResult) with additional functionality, which is specific to concrete integration (e.g.OpenAI), I'm not sure if having the namespace of original type will provide some benefits. It could be even confusing, that this method is available by default, although I didn't include any OpenAI specific namespaces in my file.I agree. I think we had discussions about that previously. Although, this change should definitely include
Obsolete, and for some period of time we will have both.Withand.Addmethods in place for smooth migration.dmytrostruk commentedon Oct 13, 2023
Regarding this, looks like IntelliSense will show you which extension methods from different namespaces are available:
stephentoub commentedon Oct 13, 2023
If you have this enabled:
Note, though, that the option also has some issues: note that some of those extensions aren't actually applicable to the thing you're dot'ing off of, which it will realize the moment you select the extension and it imports the namespace, at which point you'll get an error because the extension isn't actually valid. So some folks turn this off.
I'm also not sure of the experience here in VS Code.
14 remaining items
.Net: Blocks Namespace Flattening + Content -> Contents folder (#4195)
.Net: Simplify memory namespaces (#4230)
SergeyMenshykh commentedon Dec 14, 2023
Changing the priority of the issue and move it to
vnextto complete task#5by transferring plugins from thePlugins.Coreproject to thePluginsfolder in the samples project..Net: Blocks Namespace Flattening + Content -> Contents folder (micro…
.Net: Simplify memory namespaces (microsoft#4230)