semantic-kernel
semantic-kernel copied to clipboard
Clean up exposure and handling of CancellationToken
Motivation and Context
Clean up exposure and handling of CancellationToken in public APIs.
Description
- In public API, they should always be the last parameter (unless they can't be, e.g. due to a params array), named "cancellationToken", and be non-nullable.
- Ensure they're forwarded appropriately from one API to the next. There may be some more missing cases, but I fixed all the ones I found.
- Make the XML docs consistent.
- Remove unnecessary cancellation-related analyzer suppressions
One issue I noticed I haven't addressed here:
- Some APIs take both an SKContext and a CancellationToken separately. If both are supplied, one of the tokens (either the explicitly passed one or the one from the SKContext) might be ignored. If they're not the same token and they're both cancelable, a combined token should really be passed on to the target so that the target ends up monitoring both.
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
- [ ] I didn't break anyone :smile:
Todo: for ISKFunction and Plan it would be nice having the cancellation token as an optional parameter, without ever forcing the user to pass one. There are a lot of changes so I'll have to check the code locally. As a guideline, if the context is available, I would assume it has a valid cancellation token. If the context is created locally though, that's false so we need to check for that scenario. If the context is null and the user didn't pass a cancellation token I think we should log a warning (or an error).