microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.36k stars 3.14k forks source link

.Net remove sequential planner #3830

Closed SergeyMenshykh closed 8 months ago

SergeyMenshykh commented 9 months ago

Consider removing sequential planner from git post v1 if it's not requested by community.

dcostea commented 9 months ago

@SergeyMenshykh @matthewbolanos SequentialPlanner is removed from the final release??

What planner will inherit its functionality?

I was using successfully SequentialPlanner to command a robot API using AI (LLM). SequentialPlanner was used to break down complex actions into basic actions which API was able to perform.

matthewbolanos commented 9 months ago

I'm currently authoring a blog that I'll be posting later today to show how to use the Handlebars planner as a replacement. We are making this change since it performs much better on all of our metrics (number of plugins, token usage, loops, etc.) all without losing functionality.

Are you able to see if the Handlebars planner is a sufficient replacement for your scenarios?

matthewbolanos commented 9 months ago

@dcostea, here's the promised blog post! https://devblogs.microsoft.com/semantic-kernel/migrating-from-the-sequential-and-stepwise-planners-to-the-new-handlebars-and-stepwise-planner/

dcostea commented 9 months ago

@dcostea, here's the promised blog post! https://devblogs.microsoft.com/semantic-kernel/migrating-from-the-sequential-and-stepwise-planners-to-the-new-handlebars-and-stepwise-planner/

@matthewbolanos This is wonderful! Thank you for the quick reaction.

I have a plugin with mixed native functions and semantic functions. When I invoke the planner I get this kind of exceptions: Template references a helper that cannot be resolved. Helper 'mod' Template references a helper that cannot be resolved. Helper 'capitalize'

Before it worked nicely with SequentialPlanner using ExcludedPlugins to filter out the unwanted functions.

~~So I have moved the native functions needed for handlerbarPlanner in another plugin and I tried to exclude the semantic functions from handlerbarPlannerConfig but the ExcludedPlugins is a get-only property! Therefore I ended up having two kernels for now.~~

But at least it works!

matthewbolanos commented 9 months ago

Oh dear! Thanks for letting us know. I'll create an issue so we can get this resolved.

runceel commented 9 months ago

In my understanding, Handlebars doesn't support async method. So, current implementation of Handlebars planner locks a thread. If it was correct, there is serious performance issue on web app scenarios. And it might make deadlock on Windows app scenarios.

So, I think that SK should not remove SequentialPlanner.

stephentoub commented 9 months ago

In my understanding, Handlebars doesn't support async method. So, current implementation of Handlebars planner locks a thread. If it was correct, there is serious performance issue on web app scenarios. And it might make deadlock on Windows app scenarios.

Is there actually asynchrony being introduced? Using .Wait / .GetAwaiter().GetResult() doesn't actually cause any thread starvation issues if the Task being waited on has already completed, which means if the Task runs synchronously to completion, it's not a problem. Is there something in the code path that's actually causing work to be queued?

runceel commented 9 months ago

The following line is using .GetAwaiter().GetResult(), a method to wait for an asynchronous method synchronously, https://github.com/microsoft/semantic-kernel/blob/524b06fc1d634a59479cc2182064a870359b9616/dotnet/src/Planners/Planners.Handlebars/Handlebars/HandlebarsTemplateEngineExtensions.cs#L434

This is a best practice for Handlebars that does not support using async methods as helpers. However, it is recommended to avoid this method for Web/Windows app scenarios, as it can cause performance and deadlock issues.

Am I understanding you correctly?

stephentoub commented 9 months ago

The following line is using .GetAwaiter().GetResult(), a method to wait for an asynchronous method synchronously,

That will block the current thread waiting for the task returned from InvokeAsync to complete if the task it returns hasn't yet completed. If the function is actually synchronous, e.g.

KernelFunction function = KernelFunctionFactory.CreateFromMethod(() => DateTime.UtcNow.ToString("r"), "GetCurrentUtcTime");

the returned task will already be completed, because it completed synchronously, and so the .GetAwaiter().GetResult() won't block anything and will instead be a nop.

If the KernelFunction actually does do asynchronous work and the operation doesn't complete by the time the task is awaited, then yes, the current thread will be blocked until the work completes. There isn't any alternative if the caller requires the operation to be done prior to this operation returning to the caller.

runceel commented 9 months ago

I think that this implementation may cause performance issues in some cases, so it would be better not to delete SequentialPlanner until Handlebars supports asynchronous operations.

dcostea commented 9 months ago

@SergeyMenshykh @matthewbolanos Now it's async. Thanks!

var planner = new HandlebarsPlanner(handlebarsPlannerOptions);
var plan = await planner.CreatePlanAsync(kernel, refinedAskList!);
var result = await plan.InvokeAsync(kernel, variables);

I think the issue can be closed.

runceel commented 8 months ago

In my understanding, the implementation is sync over async.

// it's OK. Because the prompt of HandlebarsPlanner doesn't invoke any async KernelFunction.
var planner = new HandlebarsPlanner(handlebarsPlannerOptions);
var plan = await planner.CreatePlanAsync(kernel, refinedAskList!);

// The following line might lock thread if HandlebarsPlanner choosen async KernelFunction such as invoking Web APIs and invoking OpenAI with some prompt.
var result = await plan.InvokeAsync(kernel, variables);

I understand that the root cause is NOT at Semantic Kernel, it depends Handlebars.Net doesn't support async helpers. But if SequentialPlanner was removed, we will not have any workaround.

If there's anything I've misunderstood, please let me know.

stephentoub commented 8 months ago

What is your KernelFunction doing? Are you speculating it's a showstopper for you or have you actually seen it be a problem?

runceel commented 8 months ago

@stephentoub Thanks for the reply. I am not talking about individual codes but general best practices. For example, even if KernelFunction is purely a function that sleeps using Task.Delay, the problem still occurs. In a real scenario you would call the Web API or call OpenAI, but in this case for simplicity we will use Task.Delay.

Since a web app requires load testing to cause performance issues, we have created a WPF Windows application. This application is a program that uses HandlerbarsPlanner to perform a 10-second sleep using the following simple plugin.

using Microsoft.SemanticKernel;
using System.ComponentModel;
using System.Diagnostics;

namespace WpfApp2;
public class MyPlugin
{
    [KernelFunction]
    [Description("Sleep")]
    public async Task SleepAsync([Description("The number of seconds to sleep.")]int seconds)
    {
        Debug.WriteLine($"SleepAsync(seconds: {seconds}) started.");
        await Task.Delay(seconds * 1000).ConfigureAwait(false);
        Debug.WriteLine($"SleepAsync(seconds: {seconds}) finished.");
    }
}

The HandlebarsPlanner call is in MainWindow.xaml.cs and is described as follows The code appears to be completely asynchronous.

private async void ExecuteHandlebarsPlannerButtonClick(object sender, RoutedEventArgs e)
{
    try
    {
        var kernel = CreateKernel();
        var planner = new HandlebarsPlanner();
        textBlockStatusMessage.Text = "Planning...";
        var plan = await planner.CreatePlanAsync(kernel, "Speep 10 seconds.");
        textBlockStatusMessage.Text = $"""
        Executing...

        {plan}
        """;
        await Task.Delay(100); // to see the status message

        textBlockStatusMessage.Text = await plan.InvokeAsync(kernel);
    }
    catch (Exception ex)
    {
        textBlockStatusMessage.Text = ex.ToString();
    }
}

However, when the button that actually calls the above method is clicked, the application completely freezes for 10 seconds at the timing of the plan execution.

I could not find a SequentialPlanner that works with Semantic Kernel version 1, so instead I tested the following code that performs the same task with a FunctionCallingStepwisePlanner that does not block threads I tested it with the following code.

private async void ExecuteFunctionCallingStepwisePlannerButtonClick(object sender, RoutedEventArgs e)
{
    try
    {
        var kernel = CreateKernel();
        var planner = new FunctionCallingStepwisePlanner();
        textBlockStatusMessage.Text = "Executing..." ;
        var result = await planner.ExecuteAsync(kernel, "Speep 10 seconds.");
        textBlockStatusMessage.Text = result.FinalAnswer; }
    }
    catch (Exception ex)
    {
        textBlockStatusMessage.Text = ex.ToString(); }
    }
}

This one keeps the application in a state where it continues to respond to calls to the same KernelFunction.

This kind of thread-blocking behavior can be problem-prone code in both Windows and Web applications. For example, forgetting to .ConfigureAwait(false) in a Sleep KernelFunction can cause a thread deadlock, which in a Windows application will cause the application to stop completely. In a web application, multiple simultaneous accesses will exhaust the thread that handles the request, resulting in performance problems.

You can read more about this in the best practices for ASP.NET Core and Azure Functions. I have seen this cause performance issues with many of my customers.

Example 1: avoid blocking calls section of the following document https://learn.microsoft.com/en-us/aspnet/core/fundamentals/best-practices?view=aspnetcore-8.0#avoid-blocking-calls

Example 2: Use async code but avoid blocking calls section of the following document https://learn.microsoft.com/en-us/azure/azure-functions/performance-reliability#use-async-code-but-avoid-blocking-calls

Sample code

https://github.com/runceel/HandlebarsPlannerTest

How to use

  1. Clone the repo
  2. Launch WpfApp2 project
  3. Input Endpoint, DeployName, Api Key for Azure OpenAI Service
  4. Click "Execute HandlebarsPlanner (goal: Sleep 10 seconds.)" button.

Expected behavior

The application does not freeze while the plan is running.

Problem

The application freezes during the execution of the plan.

stephentoub commented 8 months ago

I am not talking about individual codes but general best practices. You can read more about this in the best practices for ASP.NET Core and Azure Functions.

Thanks. I promise you I understand how async/await works and best practices around it, having helped invent the feature, owning its current implementation in the .NET runtime, and written extensively about it (e.g. https://devblogs.microsoft.com/dotnet/how-async-await-really-works/), including the post that coined the "sync-over-async" term which describes the concerning situation at hand (https://devblogs.microsoft.com/pfxteam/should-i-expose-synchronous-wrappers-for-asynchronous-methods/). :smile: I am talking about specifics because, from what I've seen, often functions invoked from a prompt template are synchronous in nature, doing work that never or only ever rarely actually completes asynchronously. Hence why I'm asking for the specifics of your use case; I can obviously imagine plenty of situations that would be problematic, but I'm presently interested in the specific functions that you'll be invoking from prompts/plans in real code that would be problematic and without a workaround, in order to understand better why you view this as such a roadblock that would demand exposing an entirely different planner. For UI, for example, it can be worked around by queueing the initial invocation, and given the blocking that can happen internally, that might be something that should be done in the implementation itself, but from my perspective isn't worth throwing the whole system away. For scale, it's quite common to have some amount of blocking in services, even though it's best to have none, and folks can typically work around it and be quite successful by increasing the minimum thread count in the pool. Ideal? Certainly not. But often feasible if the blocking is limited.

runceel commented 8 months ago

@stephentoub Thanks for your reply. I am not a native English speaker, so I may not be getting 100% of what you are trying to say, and I apologize if my writing is overbearing. First of all, I want to thank you very much for this wonderful library you have created.

For example, a plugin that uses the Microsoft Graph API could be embedded in Kernel and ask HandlebarsPlanner to achieve a goal such as "Please add the same content to tomorrow's schedule as you did today." I would expect HandlebarsPlanner to make several calls to the Microsoft Graph API in the generated Plan. In such a case, the execution of the HandlebarsPlanner-generated Plan will lock the thread while calling the Microsoft Graph API.

Of course, it is possible to alleviate the problem by increasing the thread pool, by executing the plan in a separate thread, or by queuing the plan execution itself and executing it sequentially if it is a time-consuming process.

My concern is that the user may be confused when executing a plan created by HandlebarsPlanner, which appears to be a completely asynchronous API, but is actually waiting synchronously behind the scenes for the asynchronous task to finish. SequentialPlanner is implemented without blocking threads, so there may be value in keeping it separate from HandlebarsPlanner.

stephentoub commented 8 months ago

Thank you for the concrete example.