semantic-kernel icon indicating copy to clipboard operation
semantic-kernel copied to clipboard

Clean up exposure and handling of CancellationToken

Open stephentoub opened this issue 2 years ago • 1 comments

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:

stephentoub avatar Apr 24 '23 13:04 stephentoub

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).

dluc avatar Apr 25 '23 00:04 dluc