IKernel is mostly immutable, except for exposing RegisterCustomFunction (which ImportFunctions uses) and the FunctionInvoking/FunctionInvoked events.
Is the intent for IKernel to be mutable or immutable?
If the intent is for it to be immutable, then these members need to be removed. RegisterCustomFunction just delegates to FunctionCollection.AddFunction, so its functionality exists elsewhere already (though it appears some higher-level helpers expect to be able to be passed an IKernel and store into it... should they instead be extensions on KernelBuilder?) And the events could be changed to be delegates passed to RunAsync.
If the intent is indeed for it to be mutable, then I don't understand the combination of FunctionsCollection wrapping a ConcurrentDictionary<> (instead of a Dictionary<>) and the FunctionInvoking/FunctionInvoked events living on this type. The former suggests that it's intended for an IKernel to be used concurrently by many concurrent executions, but if that's the case, those uses may also be setting and removing handlers from the events, in which case the events are going to erroneously cross-polinate between concurrent uses.
I believe we want to make the kernel immutable. In the triage call, it appears there is a consensus on this approach. @markwallace-microsoft to add additional comments.
IKernel
is mostly immutable, except for exposingRegisterCustomFunction
(whichImportFunctions
uses) and theFunctionInvoking
/FunctionInvoked
events.Is the intent for
IKernel
to be mutable or immutable?If the intent is for it to be immutable, then these members need to be removed.
RegisterCustomFunction
just delegates toFunctionCollection.AddFunction
, so its functionality exists elsewhere already (though it appears some higher-level helpers expect to be able to be passed anIKernel
and store into it... should they instead be extensions onKernelBuilder
?) And the events could be changed to be delegates passed toRunAsync
.If the intent is indeed for it to be mutable, then I don't understand the combination of
FunctionsCollection
wrapping aConcurrentDictionary<>
(instead of aDictionary<>
) and theFunctionInvoking
/FunctionInvoked
events living on this type. The former suggests that it's intended for anIKernel
to be used concurrently by many concurrent executions, but if that's the case, those uses may also be setting and removing handlers from the events, in which case the events are going to erroneously cross-polinate between concurrent uses.So... what is the intent here?