-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.Net: Bug: Application crashes when using the ONNX connector and OnnxRuntimeGenAI v0.5.0 package #9628
Comments
### Motivation and Context - Add missing integration tests - Resolves #9628
Hi @RogerBarreto. I don't know if I'm missing something but looking at the PR that closed this issue, I don't see anything that addresses the problem I reported above. Moreover, I pulled the latest changes from the repository and tested the code again and it still crashes when it shuts down. |
@f2bo , you are right, To avoid having the problem shown in the 0.5.0 the example the caller now needs to handle the I'm giving here the final code how it should be to avoid the error. using var ogaHandle = new OgaHandle();
IKernelBuilder builder = Kernel.CreateBuilder();
builder.Services
.AddOnnxRuntimeGenAIChatCompletion(
modelId: "phi3.0",
modelPath: "... path ...");
IKernelBuilder kernelBuilder = builder.Services.AddKernel();
Kernel kernel = builder.Build();
var chat = (kernel.Services.GetRequiredService<IChatCompletionService>() as OnnxRuntimeGenAIChatCompletionService);
ChatHistory history = [];
history.AddUserMessage("Hi");
await foreach (var content in chat.GetStreamingChatMessageContentsAsync(history))
{
Console.Write(content);
}
Console.WriteLine("\nDone!");
chat.Dispose(); |
@f2bo , the For scenarios like this I would rather than creating a kernel to get the service, create the service directly with a i.e: using var chat = new OnnxRuntimeGenAIChatCompletionService(
modelId: "phi3.0",
modelPath: "... path ...");
await foreach (var content in chat.GetStreamingChatMessageContentsAsync(history))
{
Console.Write(content);
}
Console.WriteLine("\nDone!"); |
I'm not sure I follow. Currently, whether I instantiate a new service directly or retrieve it from the DI container, I still need to create an OgaHandle in my code. Isn't that the case? My concern is that this requires special handling for the ONNX connector and is somewhat brittle. I was hoping that it could be somehow encapsulated behind the code to While I was troubleshooting, I experimented by adding the following to the public sealed class OnnxRuntimeGenAIChatCompletionService : IChatCompletionService, IDisposable
{
...
private readonly static OgaHandle s_ogaHandle = new();
static OnnxRuntimeGenAIChatCompletionService()
=> AppDomain.CurrentDomain.ProcessExit += (sender, e) => s_ogaHandle.Dispose();
...
} The other remaining issue is to ensure that the ONNX connector is disposed before the process ends. Where to do this will depend on how it was created. When using However, when using |
After some investigation I changed the internal implementation of the service, so no more |
I've left a comment in the PR with some concerns. |
Describe the bug
It seems that the latest release of the OnnxRuntimeGenAI (v0.5.0) introduces additional checks to prevent resource leaks (see microsoft/onnxruntime-genai#799). As a result, an application using the ONNX connector crashes when it shuts down if it references this version of the ONNXGenAI runtime.
To Reproduce
Add a reference to the Microsoft.ML.OnnxRuntimeGenAI.DirectML Version="0.5.0" package and run the following code:
Running the code above produces the following output and then crashes:
I found a workaround based on the changes found in this PR. It seems that declaring (and disposing) an
OgaHandle
will ensure a proper shutdown. I couldn't find much information about it except for the comment "Responsible for cleaning up the library during shutdown" here.However, simply declaring an OgaHandle is still insufficient to ensure a clean shutdown. The ONNX connector must also be disposed before the application exits. Otherwise, it will still crash with the following output:
Platform
The text was updated successfully, but these errors were encountered: