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

Fix a memleak in RunAsync python #17326

Merged
merged 7 commits into from
Aug 30, 2023
Merged

Conversation

RandySheriffH
Copy link
Contributor

@RandySheriffH RandySheriffH commented Aug 29, 2023

Release ort value outputs that are created and released from ort::run(...).

@RandySheriffH RandySheriffH marked this pull request as ready for review August 29, 2023 03:06
Copy link
Contributor

@pranavsharma pranavsharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of Run where the output values need to be released is not documented in the header. I looked at the header file and it wasn't evident to me if fetches need to be released. Let's take this opportunity to document the 2 newly introduced Run functions.

onnxruntime/python/onnxruntime_pybind_state.cc Outdated Show resolved Hide resolved
@RandySheriffH
Copy link
Contributor Author

This version of Run where the output values need to be released is not documented in the header. I looked at the header file and it wasn't evident to me if fetches need to be released. Let's take this opportunity to document the 2 newly introduced Run functions.

Documented both c/cxx API accordingly.

Copy link
Contributor

@pranavsharma pranavsharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I said documentation in my previous comment, I was referring to inference_session.h file. See this. None of these newly introduced functions are documented. It's not clear if the user needs to release any parameters or not.

Also, to keep life simple, is it possible to use vector<unique_ptr<OrtValue>> in AsyncResource?

  • It won't work
    Because the "run" API we are calling requires an array of OrtValues* (could be null) as outputs, when "run" API returns, the array will be filled with allocated OrtValue*, a vector of unique_ptr does not fit in.

@RandySheriffH RandySheriffH merged commit 6c39641 into main Aug 30, 2023
93 of 95 checks passed
@RandySheriffH RandySheriffH deleted the rashuai/AsyncResourceRelease branch August 30, 2023 19:54
@natke natke added the triage:approved Approved for cherrypicks for release label Sep 1, 2023
er3x3 pushed a commit that referenced this pull request Sep 1, 2023
Release ort value outputs that are created and released from
ort::run(...).

---------

Co-authored-by: Randy Shuai <rashuai@microsoft.com>
snnn pushed a commit that referenced this pull request Sep 7, 2023
Cherry-pick 2nd round for 1.16.0 release.
PR List:

#17201
#17270
#17311
#17315
#17320
#17326
#17355
#17227
#17380
#17386
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
Release ort value outputs that are created and released from
ort::run(...).

---------

Co-authored-by: Randy Shuai <rashuai@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage:approved Approved for cherrypicks for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants