Skip to content
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: Bump ONNX to 0.5.2 #9644

Merged

Conversation

RogerBarreto
Copy link
Member

Motivation and Context

@RogerBarreto RogerBarreto requested a review from a team as a code owner November 11, 2024 17:40
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Nov 11, 2024
@f2bo
Copy link

f2bo commented Nov 11, 2024

Doesn't this change mean that the model is reloaded into memory every time that you run an inference? This might take several seconds each time.

On the other hand, isn't having multiple OgaHandles, one for each service instance, a problem? It's not guaranteed that you will only have a singleton instance and disposing of one of the ONNX services will shut down the ONNX runtime for the process, so that all other services become invalid.
On the other hand, isn't creating multiple OgaHandles, one for each invocation of the service, a problem? The handle is disposed after the first invocation, which will shut down the ONNX runtime for the process and as a result, any subsequent invocations should fail.

@RogerBarreto
Copy link
Member Author

@f2bo This won't be merged as is, I will add some extra Unit Testing to ensure it doesn't affect the status-quo, for the moment recommend using the ONNX Connector with previous version of GenAI.

@RogerBarreto RogerBarreto marked this pull request as draft November 11, 2024 23:07
@RogerBarreto RogerBarreto changed the title .Net: ONNX 0.5.0 Add Ogahandle resource managment to Service. .Net: ONNX 0.5.0 Add Ogahandle resource managment to Service [WIP] Nov 11, 2024
…nto issues/9628-adding-ogahandle-resource
@RogerBarreto RogerBarreto changed the title .Net: ONNX 0.5.0 Add Ogahandle resource managment to Service [WIP] .Net: Bump ONNX to 0.5.2 Nov 28, 2024
@RogerBarreto RogerBarreto marked this pull request as ready for review November 28, 2024 16:05
@f2bo
Copy link

f2bo commented Nov 28, 2024

Rather than relying on users to ensure that an OgaHandle is created and disposed, I wonder if you have any objections to the following change:

public sealed class OnnxRuntimeGenAIChatCompletionService : IChatCompletionService, IDisposable
{
    ...
    private readonly static OgaHandle s_ogaHandle = new();

    static OnnxRuntimeGenAIChatCompletionService()
        => AppDomain.CurrentDomain.ProcessExit += (_, _) => s_ogaHandle.Dispose();
  ...
}

@RogerBarret0
Copy link
Contributor

RogerBarret0 commented Nov 28, 2024

@f2bo Yes, I personally object this approach as this injects static resources and behavior on customers environment/application domain without their knowledge to handle a non-conventional resource monitoring.

And some other considerations regarding the potential pitfalls of using ProcessExit on different platforms.

  1. Execution Time Limit: ProcessExit handlers have limited time and may not finish long-running tasks.
  2. Unreliable State: The app may already be in an inconsistent state when the event fires.
  3. Unhandled Exceptions: Errors in handlers are hard to debug and can terminate the app abruptly.
  4. Cross-Platform Inconsistencies: Shutdown behavior varies across platforms and may bypass ProcessExit.

Co-authored-by: westey <164392973+westey-m@users.noreply.github.com>
@RogerBarreto RogerBarreto added this pull request to the merge queue Dec 2, 2024
Merged via the queue into microsoft:main with commit 4674281 Dec 2, 2024
15 checks passed
@RogerBarreto RogerBarreto deleted the issues/9628-adding-ogahandle-resource branch December 2, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Bug: Application crashes when using the ONNX connector and OnnxRuntimeGenAI v0.5.0 package
6 participants