microsoft
SKFunction Invoke Parameter Duplication
Motivation and Context
Each of the SKFunction.Invoke calls have multiple ways of passing the same data. It's not clear that, if one way is specified, the other will be ignored.
https://github.com/microsoft/semantic-kernel/issues/727
Description
This cleans things up such that you either pass a full SKContext, which has everything included (CancellationToken, Logger, input), or you pass each of these as separate args, and an SKContext is created for you.
Contribution Checklist
- [X] The code builds clean without any errors or warnings
- [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with
dotnet format - [X] All unit tests pass, and I have added new tests where possible
- [X] I didn't break anyone :smile:
@stephentoub PR addressing Issue https://github.com/microsoft/semantic-kernel/issues/727
The changes to SKFunction will break scenarios where we pass the context only for the internal helpers, and want to pass the input as a string param.
Yeah I had the same consideration. This is another one in the theme of methods have multiple paths to provide the same arg, and it's unclear which will be used. If I set a context[Input] and then call a Invoke with a empty input (and a valid context), should the input be empty or what was there in context?
I think this makes the API nice and clean: If you already have a context, go ahead and update it however you want, and pass that in. If you don't, give us the pieces (input, vars, logger, cancel) -- most are optional -- and we'll build the context for you. Make sense? I've got an updated PR to send out, and then I'll check in with you on the latest.
Flipping to draft until I have time to update.
If you don't, give us the pieces (input, vars, logger, cancel) -- most are optional -- and we'll build the context for you.
It'd also be good for the SKContext.Skills collection to be non-nullable. If no collection is supplied to the ctor, it can just initialize the Skills to an empty collection. Then downstream consumption doesn't need to check whether the collection is null or not.
If you don't, give us the pieces (input, vars, logger, cancel) -- most are optional -- and we'll build the context for you.
It'd also be good for the SKContext.Skills collection to be non-nullable. If no collection is supplied to the ctor, it can just initialize the Skills to an empty collection. Then downstream consumption doesn't need to check whether the collection is null or not.
Yup, good call! In fact, I've got something like this in my update... Because the IROSkillCollection interface is in Abstractions and the implementation is in SK.Core, I had to create a NullReadOnlySkillCollection in Abstractions. Just need to merge with that big skill collection change, and I'll get the update sent out.