Excel hangs when starting many ExcelFunctions

[Kasper Marstal]

Hi all,

My Add-In causes Excel to hang when ~20 formulas or more are started at the same time. In once instance, a user reported he started 25.000 formulas at once which completely froze his machine. I must be doing something wrong or have an incorrect mental model of how to do work in a background thread. I hope you can help.

I tried to design my function to get out of the main thread's way as quickly as possible:

• I do the lightest of argument processing in the main body, taking care to call GetValue() only once per ExcelReference, and return an IExcelObservable to do actual work (mostly waiting on network IO) (full source):

// We must parse arguments on the main thread
var arguments = argumentParser
    .AddProvider(providerAndModel)
    .AddModel(providerAndModel)
    .AddInstructionsOrCells(instructionsOrCells)
    .AddInstructionsOrTemperature(instructionsOrTemperature)
    .AddTemperature(temperature)
    .Parse();

// ObserveResponse will send request on another thread and update the cell value on the main thread
return ExcelAsyncUtil.Observe(
    nameof(PromptWith),
    new object[] { providerAndModel, instructionsOrCells, instructionsOrTemperature, temperature },
    () => new ObserveResponse(arguments));

• In the IExcelObservable's Subscribe() I do all work in an async task (full source):

_task = Task.Run(async () =>
{
    try
    {
        ... [Omitted for brevity] ...

        var prompt = new PromptBuilder()
            .SetModel(arguments.Model)
            .SetTemperature(arguments.Temperature)
            .SetMaxOutputTokens(cellmAddInConfiguration.CurrentValue.MaxOutputTokens)
            .AddSystemMessage(SystemMessages.SystemMessage)
            .AddUserMessage(userMessage)
            .Build();

        // Check for cancellation before sending request
        _cancellationTokenSource.Token.ThrowIfCancellationRequested();

        var client = CellmAddIn.Services.GetRequiredService<Client>();
        var response = await client.GetResponseAsync(prompt, arguments.Provider, _cancellationTokenSource.Token);
        var assistantMessage = response.Messages.LastOrDefault()?.Text ?? throw new InvalidOperationException("No text response");

        // Check for cancellation before notifying observer
        _cancellationTokenSource.Token.ThrowIfCancellationRequested();

        // Notify observer on the main thread
        ExcelAsyncUtil.QueueAsMacro(() =>
        {
            _observer?.OnNext(assistantMessage);
            _observer?.OnCompleted();
        });
    }
    catch (OperationCanceledException ex) { ... [Omitted for brevity] .... }
    catch (Exception ex) { ... [Omitted for brevity] ... }
}, _cancellationTokenSource.Token);

return new ActionDisposable(() =>
{
    _logger.LogDebug("Getting response {id} ... Disposing ({status})", _task.Id, _task.IsCompleted ? "done" : "cancelled");
    _cancellationTokenSource.Cancel();
});

Here is a video where you can literally see for how long it hangs before UI is updated with #GETTING_STARTED for ~80 cells: https://drive.google.com/file/d/1JcP48tsGYxzXl3cNd0q_hsmgOmWq3Hxv/view?usp=sharing

Any input is greatly appreciated.

Thanks,

Kasper

[Govert van Drimmelen]

Hi Kasper,

I think the problem is at your use of this "ResiliencePipeline" library, which is not behaving as you expect.

The async "Execute" function you use is not returning soon, and looks like it's wrapping some stuff around a slow sync call.

To check this, you might change the Client.GetResponseAsync code to something like this (with the corresponding change to the call):

internal class Client(ISender sender, ResiliencePipelineProvider<string> resiliencePipelineProvider)
{

    public async Task<Prompt> GetResponseAsync(Prompt prompt, Provider provider, CancellationToken cancellationToken)
    {

        // No Resilience . . . .
        var response = await sender.Send(new ProviderRequest(prompt, provider), cancellationToken);
        return response.Prompt;
    }

}

Although not the problem, you can simplify away the whole Observable story too.

The Observe helper can be replaced with a simpler Task-based async function like this (I've not put the logging in):

internal class AsyncCalls
{
    public static async Task<string> GetResponseAsync(Arguments arguments, CancellationToken cancellationToken)
    {
        var cells = arguments.Cells switch
        {
            Cells argumentCells => ArgumentParser.ParseCells(argumentCells),
            null => "Not available",
            _ => throw new ArgumentException(nameof(arguments.Cells))
        };

        var instructions = arguments.Instructions switch
        {
            string instruction => instruction,
            Cells values => ArgumentParser.ParseCells(values),
            _ => throw new ArgumentException(nameof(arguments.Instructions))
        };

        var userMessage = new StringBuilder()
            .AppendLine(ArgumentParser.AddInstructionTags(instructions))
            .AppendLine(ArgumentParser.AddCellTags(cells))
            .ToString();

        var cellmAddInConfiguration = CellmAddIn.Services.GetRequiredService<IOptionsMonitor<CellmAddInConfiguration>>();

        var prompt = new PromptBuilder()
            .SetModel(arguments.Model)
            .SetTemperature(arguments.Temperature)
            .SetMaxOutputTokens(cellmAddInConfiguration.CurrentValue.MaxOutputTokens)
            .AddSystemMessage(SystemMessages.SystemMessage)
            .AddUserMessage(userMessage)
            .Build();

        var client = CellmAddIn.Services.GetRequiredService<Client>();

        //  ***** This call is problematic - it does not behave like an async method - lots of sync work before it returns that Task *****
        var response = await client.GetResponseAsync(prompt, arguments.Provider, cancellationToken);

        var assistantMessage = response.Messages.LastOrDefault()?.Text ?? throw new InvalidOperationException("No text response");

        return assistantMessage;
    }
}

Then you call from the function like this:

var asyncResult = ExcelAsyncUtil.RunTaskWithCancellation(

    nameof(PromptWith),
    new object[] { providerAndModel, instructionsOrCells, instructionsOrTemperature, temperature },

    ct => AsyncCalls.GetResponseAsync(arguments, ct));

if (asyncResult.Equals(ExcelError.ExcelErrorNA))
    asyncResult = ExcelError.ExcelErrorGettingData;

return asyncResult;

This plan means you have no Task.Run involved, which is not a good pattern is you want to call many functions since it gets clogged in the .NET ThreadPool.

Even if you keep the Observable story, you don't have to call the OnNext / OnCompleted from the main thread.

You should probably move the 'argument parsing' into the async function itself - that should run on the main thread until the first 'await' (you can check this).

The outer wrapper function where you have the 'argument parsing' now runs many times for each async function - you might want to trace this to check.

(I also notice in your installer that you are writing the "OPEN" key directly. This is not correct, and will overwrite an existing add-in if present.

There is a sequence of OPEN, OPEN1, OPEN2 etc. keys that you must find the right slot in.

See the WixInstaller project for one way to add a custom action that finds the right key to add.)

-Govert

[Kasper Marstal]

Hi Govert,

Many thanks for your pointers, you obviously spent some time digging in the code. Thank you for your effort, did not expect that. It is running super smoothly now! I feels completely different to use. I am very happy and I am sure future users will be too!

Using ExecuteAsync instead of Execute did indeed speed things up. After that was fixed, it was easier to spot another issue: When GetValue() was called on an ExcelReference for a cell that hadn't been calculated yet, it would throw an XlCallException (XlReturnUncalced, which is harmless right? I just return #GETTING_DATA now). My logging system was catching this exception and attempting to process on the main thread where the argument parsing happens. With many cells throwing this simultaneously (e.g., during a large recalculation wave with dependencies), the logging bottleneck became apparent.

Regarding your other points:

• ExcelAsyncUtil.RunTaskWithCancellation: Nice if this can further speed things up. But in the end this will also run on the .NET thread-pool right? Can you help me understand why this is this better than Task.Run?
• "The outer wrapper function where you have the 'argument parsing' now runs many times for each async function". How does this work? Is the ExcelFunction being called many times each time the user calls the function from a formula?
• OPEN registry key: Yes indeed, this needs to be done before releasing the installer. Saved this wonderful little task for a rainy Sunday afternoon. The installer template has been very helpful so far.

Thanks again Govert.

Kasper

--
You received this message because you are subscribed to the Google Groups "Excel-DNA" group.
To unsubscribe from this group and stop receiving emails from it, send an email to exceldna+u...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/exceldna/9ff31e67-7a69-4b56-9fc2-bdbba70f1f11n%40googlegroups.com.

[Govert van Drimmelen]

Hi Kasper,

 

• ExcelAsyncUtil.RunTaskWithCancellation: Nice if this can further speed things up. But in the end this will also run on the .NET thread-pool right? Can you help me understand why this is this better than Task.Run?

It’s not quite the same – it implements the async call directly as an IObservable on top of the Task you pass in (actually the Task created from the delegate you pass in).

If the Task you pass in does not come from Task.Run, but is rather a C# async / await method with some ‘native’ async work like a network call where it awaits, then the work does not first have to get scheduled on the threadpool. The completion after the await  (or between awaits) may or may not get scheduled on the ThreadPool, but seems more efficiently managed to me. Anyway, you basically get rid of your own IObservable implementation and use the Excel-DNA one, and you get to write the ‘implementation’ of the async function in a cleaner way as a Task-returning async/await function. So, even if there is no performance difference, it cleans things up a bit.

 

• "The outer wrapper function where you have the 'argument parsing' now runs many times for each async function". How does this work? Is the ExcelFunction being called many times each time the user calls the function from a formula?

You should put a Debug.Print or something in the outermost function to see what I mean.

It gets called initially, and then the async completion causes it to be called again (with an internal structure having prepared the result).

The function name and argument array that you pass to ExcelAsyncUtil.Observe or RunTask… is the ‘key’ in the dictionary that keeps the result, so it’s looked up and returned from there.

If you make a ‘streaming’ function that returns many values to Excel, then the outer wrapper is called for every update.

So you want to make the outer bit (everything that runs before you call ExcelAsyncUtil.xxx) as simple and efficient as possible.

Rather capture all the arguments passed to the function as part of the delegate that you pass to ExcelAsyncUtil.xxx, and dop further processing inside the delegate.

The delegate only runs once per formula setup.

In your case this will probably not make a noticeable difference, but it’s a good pattern to get right early.

 

For a much more dramatic performance improvement, you can try to figure out how to make the async function work as batched calls to the back end (either your own back-end or even as batched calls to the api with some prompt tuning). That lets you have single cell async functions, but make far fewer calls to the back.

One example of how to approach this is here: Samples/AsyncBatch/AsyncBatch/AsyncBatchUtil.cs at master · Excel-DNA/Samples

 

Also keep in mind dynamic arrays, and whether you can make functions that can take and return arrays.

That works very well to improve the async granularity too.

 

Let me know if you have any other questions – it looks like a promising add-in and service.

 

Regards,

Govert

To view this discussion visit https://groups.google.com/d/msgid/exceldna/CAGGgHneRDBTnxTv3fGq_pE3SX-gC4yUu%3DeYtofa_h62ppaH%3D%3Dg%40mail.gmail.com.

[Kasper Marstal]

Hi Govert,

Thank you, that means a lot, especially when coming from you! 

I'm preparing a PR with ExcelAsyncUtil.RunTaskWithCancellation. So far it works really well. Hard to tell if there are actual speedups when starting 10^2 calls (order of magnitude) because everything is so fast now. I have yet to test with 10^3 and 10^4, but I agree the code is cleaner and easier to reason about. 

I do have a couple of questions:

• "Rather capture all the arguments passed to the function as part of the delegate that you pass to ExcelAsyncUtil.xxx, and do further processing inside the delegate." I've been hesitant to call GetValue() on ExcelReferences anywhere other than in the main [ExcelFunction] because I've sometimes seen the "The calling thread cannot access this object because a different thread owns it." when trying to move argument parsing out of the main thread. Is it safe to call GetValue inside the delegate passed to ExcelAsyncUtil.RunTaskWithCancellation? Or could this sometimes fail because of some race condition? If it is safe, argument parsing could be moved as well so it only runs once as you say, and that would be great. Perhaps something else was wrong when I got this exception in the past. The implementation is very different now. 
• I've been dying to support streaming responses from models so that the user sees something happening right away instead of having to wait for the complete response before the cells update. For example, local models that run on the user's computer can take minutes to produce a response if the model is big or the user doesn't have a GPU. Local models are nice because none of the user's sheet data leaves his/her machine. I tried with IExcelObservable, but of course every call to OnNext() triggers recalculation of downstream cells so I had to abandon the effort. Is there any way to update the cell value/contents and only trigger recalculation once a stream of updates is finished? As an example, imagine I could use IExcelObservable and accumulate model responses with StringBuilder, call OnNext(stringBuilder.ToString()) everytime we got a chunk of the response, and trigger recalculation only when calling OnComplete() in the end. I know this is not how IExcelObservable works today, but just trying to convey to you the desired behavior.

Nice to know there is potential for further improvement with batched calls.

To view this discussion visit https://groups.google.com/d/msgid/exceldna/03bb01dbd3ff%24ab095b80%24011c1280%24%40gmail.com.

[Govert van Drimmelen]

Hi Kaspar,

If you're going to call ExcelReference.GetValue on a function argument, you're much better off just taking the value as the argument directly.

If you change the declaration to AllowReference=false (the default). Then 

* if you declare the parameter type as object, and you'll get a string or ExcelMissing / ExcelError etc. or you'll get an object[,] same as you now get from GetValue..

* if you declare the parameter types as object[,] you'll always get a 2D array with one or more items.

The only reason to make it AllowRefrence=true is if you want to do some manipulation of the the cell that is pointed to, and that's not recommended.

It also has other effects in the calculation, e.g. a function that is marked as IsMacroType=true and has an AllowReference=true parameter is always considered volatile, even if you flag it not to be.

So, I recommend no using AllowRefrence=true as long as you can, and dealing with the argument as it comes in.

If you do take the ExcelReference, you are right to have some concern about when you use it. The reference, or indeed the whole sheet or workbook might be gone if you don't use it immediately (in the calculation call), and that can be hard to detect and guard against.

With the upcoming version 1.9.0, for both streaming and Task<T> based functions, you can just declare the functions directly as 

[ExcelFunction(...)]

public static async Task<object> MyAsyncFunction(...) {  }

and 

[ExcelFunction(...)]

public static IObservable<object> MyStreamingFunction(...) {  }

You are right that streaming functions cause the sheet to recalculate - that is the intent. You can't display new values on the sheet without recalculating.

There is some throttling that means Excel will not read the new values too often (by default 2 seconds).

So it might not be ideal for what you have in mind.

-Govert

[Kasper Marstal]

Hi Govert,

I see, using references would make everything simpler.  The reason for using ExcelReferences is to get RowFirst and ColumnFirst coordinates to render the cell as a markdown table with column headers because users sometimes ask e.g. "Extract all person names from text in column A" and the model needs to know what "column A" refers to. Can I get the coordinates for the object and object[,] arguments? Asking because I haven't found a way to do so without the "Microsoft.Office.Interop.Excel.Application)ExcelDnaUtil.Application" where I have to be on the main thread anyway (as far as I understand).

"The reference, or indeed the whole sheet or workbook might be gone if you don't use it immediately (in the calculation call),": Interesting. I need to think about the possible implications here.

This is super useful: "public static async Task<object> MyAsyncFunction(...) {  }" Nice work. Can't wait to try it out and clean up code even further.

Too bad streaming responses are not possible but that's what I thought. Thanks for confirming.

To view this discussion visit https://groups.google.com/d/msgid/exceldna/1d3155bd-8d2a-4848-ba03-a3cde22608cen%40googlegroups.com.

[Govert van Drimmelen]

Hi Kasper,

 

If you need the address of the passed in reference, then you do indeed need AllowReference=true.

 

-Govert

To view this discussion visit https://groups.google.com/d/msgid/exceldna/CAGGgHncrfHijKo0Ziha22wDvE1jgqqb0w9MEZ%3DaZLiU6vVv%3DxA%40mail.gmail.com.

[Kasper Marstal]

Hi Govert,

Okay, thank you for everything so far. Really appreciate it.

Kasper

To view this discussion visit https://groups.google.com/d/msgid/exceldna/00d701dbd53f%24bd8a6060%24389f2120%24%40gmail.com.

[Kasper Marstal]

Hi Govert,

" public static async Task<object> MyAsyncFunction(...) {  }" works well except the initial value is #N/A instead of #GETTING_DATA. I tried:

• Adding the following to AutoOpen() (from another thread) in which case I get "Registration [Error] Repeated function name: 'PROMPT' - previous registration will be overwritten.":
• ExcelRegistration.GetExcelFunctions()            
      .ProcessParameterConversions(ParameterConversions())
      .ProcessAsyncRegistrations()
      .RegisterFunctions();
• static ParameterConversionConfiguration ParameterConversions()
  {
      // This conversion replaces the default #N/A return value of async functions with the #GETTING_DATA value.
      return new ParameterConversionConfiguration()
          .AddReturnConversion((type, customAttributes) => type != typeof(object) ? null : ((Expression<Func<object, object>>)
  }
• Returning IObservable but feels wrong because then we are back where we started with an observer wrapped in ExcelDNA's observer.

How would you recommend setting #GETTING_DATA for native async functions?

Kasper