.Net: Missing CancellationToken in InvokePromptAsync<T>(...)

[TaffarelJr]

Describe the bug
In Semantic Kernel 1.0.1, KernelExtensions.InvokePromptAsync<T>(...) doesn't include a CancellationToken parameter.

To Reproduce
N/A

Expected behavior
Async methods should allow a CancellationToken to be passed in. The other extensions in this class do; it looks like this one was forgotten.

[mehrandvd]

I’m eager to take on this issue and make my first contribution to SK.

[stephentoub]

Ugh, that's an oversight, it should have had one.

@matthewbolanos, @markwallace-microsoft, here's a small test of our non-breaking-change will: it's a binary breaking change to add a CancellationToken argument to the existing method, even as an optional parameter (it's not source breaking because existing callers would successfully recompile and just get the optional default value, but existing binaries calling the old method would start getting MethodNotFoundExceptions when targeting a newer version that only includes the new signature). So the right answer if we do care about that, which I think we do, is to add a new overload, even though it looks inconsistent.

And adding new overloads of methods with optional parameters is a little tricky, because if you just add another identical method but with an extra optional parameter, it's source breaking because existing callers will start to get ambiguity errors, with the compiler not knowing which method to pick if there's not an exact match (i.e. if you were passing all arguments). The answer is basically change the existing method to not have any optional arguments (removing the default value from all parameters), and then add a new overload with everything that was optional still optional plus the new optional method.

@bartonjs, do we have the canonical steps for this written down somewhere?

[markwallace-microsoft]

@mehrandvd We would be very happy for you to take this. Please link your PR to this issue and I'll look out for it. Thanks.

[mehrandvd]

@stephentoub As I understood, this task is not simply adding a cancellationToken to InvokePromptAsync (avoiding a binary breaking change)
Instead, I should change the existing method to not have any optional arguments like:

public static Task<T?> InvokePromptAsync<T>(
    this Kernel kernel,
    string promptTemplate,
    KernelArguments? arguments,
    string? templateFormat,
    IPromptTemplateFactory? promptTemplateFactory)

and then add a new overload with everything optional still optional plus the new optional method like:

public static Task<T?> InvokePromptAsync<T>(
    this Kernel kernel,
    string promptTemplate,
    KernelArguments? arguments = null,
    string? templateFormat = null,
    IPromptTemplateFactory? promptTemplateFactory = null,
    CancellationToken cancellationToken = default)

Am I right?

[stephentoub]

That's correct, with the existing method then delegating to the new one that has the implementation.

[bartonjs]

@bartonjs, do we have the canonical steps for this written down somewhere?

Framework Design Guidelines 3rd edition, section 5.1.1, p147 in the print edition, under: "DO move all default parameters to the new, longer overload when adding optional parameters to an existing method."

But I think y'all have the gist here; the only thing missing is to consider adding [EditorBrowsable(EditorBrowsableState.Never)] to the older overload if you want to reduce intellisense complexity.

With all of our guidelines stacked up, I'd expect

public static Task<T?> InvokePromptAsync<T>(
    this Kernel kernel,
    string promptTemplate,
    CancellationToken cancellationToken = default)
{
    // feel free to specify them all, but this's enough to latch to the final one.
    return InvokePromptAsync(kernel, promptTemplate, arguments: default, cancellationToken: cancellationToken);
}

[EditorBrowsable(EditorBrowsableState.Never)]
public static Task<T?> InvokePromptAsync<T>(
    this Kernel kernel,
    string promptTemplate,
    KernelArguments? arguments,
    string? templateFormat,
    IPromptTemplateFactory? promptTemplateFactory)
{
    return InvokePromptAsync(
        kernel,
        promptTemplate,
        arguments,
        templateFormat,
        promptTemplateFactory,
        default(CancellationToken));
}

public static Task<T?> InvokePromptAsync<T>(
    this Kernel kernel,
    string promptTemplate,
    KernelArguments? arguments = null,
    string? templateFormat = null,
    IPromptTemplateFactory? promptTemplateFactory = null,
    CancellationToken cancellationToken = default) { ... }

and if there was already a (kernel, promptTemplate) overload for it to also get EB-Never'd in favor of the (kernel, promptTemplate, cancellationToken) one.

[mehrandvd]

@bartonjs Just to confirm, since there isn't a (kernel, promptTemplate) overload at the moment, the last two implementations should do the trick, right?

[bartonjs]

The first one comes from

DO provide a simple overload, with no default parameters, for any
method with two or more defaulted parameters.

combined with the guidance to always default a CancellationToken.

It's not important for the "how do I add a new defaulted parameter without making a breaking change?" question, but it's something that "should" be there in the end.