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

[Feature Request] Implement provider destruction notification mechanism (e.g: IExecutionProvider::OnSessionEnd() ) #22970

Open
Hopobcn opened this issue Nov 28, 2024 · 2 comments
Labels
feature request request for unsupported feature or enhancement

Comments

@Hopobcn
Copy link

Hopobcn commented Nov 28, 2024

Describe the feature request

I am implementing profiling for a new Execution Provider, this new provider traces contain information about the memory allocations/deallocaitons performed in the accelerator (among other events). Problem is, due current destruction ordering in InferenceSession::~InferenceSession() the tracing session may be ended before 1) session_state_ is wiped and 2) execution providers are destroyed. This proposal would address (2).

Currently, Execution Providers have no opportunity to get notified that the session will finish to perform cleanup before profiling session finishes.

Would it be acceptable to add

  /**
     Called when session is destroyed
     This provides an opportunity for execution providers to optionally synchronize and
     clean up its temporary resources to reduce memory before the session is destroyed.
   */
  virtual common::Status OnSessionEnd() { return Status::OK(); }

to IExecutionProvider

and refactor ~InferenceSession() destructor accordingly?

InferenceSession::~InferenceSession() {
  ORT_TRY {
    for (auto& xp : execution_providers_) {
      auto end_status = xp->OnSessionEnd();
      if (!end_status.IsOK()) {
       LOGS(*session_logger_, ERROR) << end_status.ErrorMessage();
      }
    }
  }
  ORT_CATCH(const std::exception& e) {
    // TODO: Currently we have no way to transport this error to the API user
    ORT_HANDLE_EXCEPTION([&]() {
      LOGS(*session_logger_, ERROR) << "Error during ~InferenceSession(): " << e.what();
    });
  }
  ORT_CATCH(...) {
    LOGS(*session_logger_, ERROR) << "Unknown error during ~InferenceSession()";
  }

I can contribute with the change if necessary.

Describe scenario use case

Improve information contained in traces by informing ExecutionProviders that the session will end, letting them perform optional cleanups before the tracing session ends.

@Hopobcn Hopobcn added the feature request request for unsupported feature or enhancement label Nov 28, 2024
@Hopobcn Hopobcn changed the title [Feature Request] Implement provider destruction notification callback (e.g: IExecutionProvider::OnSessionEnd() ) [Feature Request] Implement provider destruction notification mechanism (e.g: IExecutionProvider::OnSessionEnd() ) Nov 28, 2024
@skottmckay
Copy link
Contributor

Is this something that needs to be a long-term part of the Execution Provider API, or something that you can temporarily add locally for this sort of testing?

@Hopobcn
Copy link
Author

Hopobcn commented Nov 29, 2024

For the use case i'm presenting I agree that this could be implemented as an optional behavior tied to whether profiling/tracing is enabled. However, I believe that providing such a notification (e.g., OnSessionEnd) would serve a broader purpose beyond just my immediate use case. It would give Execution Providers a chance to perform optional cleanup or synchronization tasks when the session is being destroyed, which can be valuable in other scenarios as well.

In my case, this mechanism would allow the Execution Provider to finalize its profiling traces properly, ensuring they include all relevant events (such as memory deallocation) that occur during the destruction of InferenceSession. Without this notification, some important cleanup or finalization steps might be performed outside of the tracing scope giving a sensation of leaking resources.

By making OnSessionEnd part of the API but keeping its implementation optional, Execution Providers that do not need this feature could simply rely on the default no-op implementation. This would ensure backward compatibility while allowing providers that need it to implement this functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request request for unsupported feature or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants