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

Support DisposableNamedOnnxValue inputs in c# Run() #3175

Merged
merged 13 commits into from
Mar 24, 2020

Conversation

hariharans29
Copy link
Member

@hariharans29 hariharans29 commented Mar 11, 2020

Description: DisposableNamedOnnxValue inputs in c# Run() inputs are to be supported as ``DisposableNamedOnnxValueisNamedOnnxValue`.

This change has the following parts:

  1. Add logic to handle DisposableNamedOnnxValue inputs to Session Run()
  2. Support a setter for the names of NamedOnnxValue/DisposableNamedOnnxValue. This is required if someone were to pass in the output of Run() to the input of another Run() as the names of the input feed is likely to be different from the name of the output feed.
  3. Tests

Motivation and Context
Resolve #2983

@hariharans29 hariharans29 requested a review from a team as a code owner March 11, 2020 06:52
@hariharans29 hariharans29 requested a review from jignparm March 11, 2020 06:53
@hariharans29 hariharans29 changed the title Initial commit Guard against DisposableNamedOnnxValue inputs in c# Run() Mar 11, 2020
@hariharans29
Copy link
Member Author

Synced offline with @fs-eire. After discussing, we felt DisposableNamedOnnxValue inputs in Run() should be accommodated as DisposableNamedOnnxValue is NamedOnnxValue (it is a derived class). Adjusted the PR contents accordingly.

@hariharans29 hariharans29 changed the title Guard against DisposableNamedOnnxValue inputs in c# Run() Support DisposableNamedOnnxValue inputs in c# Run() Mar 12, 2020
@hariharans29 hariharans29 requested a review from fs-eire March 12, 2020 19:29
Copy link
Contributor

@fs-eire fs-eire left a comment

Choose a reason for hiding this comment

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

as discussed offline with @hariharans29 , NamedOnnxValue should be kept stateless. it does not implement interface IDisposable so it should not hold the IntPtr inside. ToNativeOnnxValue may need to be updated so that it outputs a boolean value to help Run() to determine whether to clean up the value or not.

for adding a setter to Name, this is a valid requirement for users to pass the output as input to another inference session. There are 2 options to support this :

  • enabling users to change the Name property of a NamedOnnxValue object
  • enabling users to create a new NamedOnnxValue object by performing a shallow copy from another.

it looks like both options have pros and cons so I would like to hear from more people - @jignparm @pranavsharma

protected IDisposable _nativeMemoryManager;
protected DisposableNamedOnnxValue(string name, Object value, IDisposable nativeMemoryManager)
private NativeMemoryHandler _nativeMemoryManager;
private DisposableNamedOnnxValue(string name, Object value, NativeMemoryHandler nativeMemoryManager)
Copy link
Member Author

Choose a reason for hiding this comment

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

ctor is private now as per @fs-eire 's suggestion

@hariharans29
Copy link
Member Author

as discussed offline with @hariharans29 , NamedOnnxValue should be kept stateless. it does not implement interface IDisposable so it should not hold the IntPtr inside. ToNativeOnnxValue may need to be updated so that it outputs a boolean value to help Run() to determine whether to clean up the value or not.

for adding a setter to Name, this is a valid requirement for users to pass the output as input to another inference session. There are 2 options to support this :

  • enabling users to change the Name property of a NamedOnnxValue object
  • enabling users to create a new NamedOnnxValue object by performing a shallow copy from another.

it looks like both options have pros and cons so I would like to hear from more people - @jignparm @pranavsharma

Made the change to keep NamedOnnxValue stateless as it was previously.

// Make sure that this instance hasn't been disposed yet
if (disposedValue)
{
throw new Exception("This instance of DisposableNamedOnnxValue has already been disposed");
Copy link
Member

Choose a reason for hiding this comment

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

This instance of DisposableNamedOnnxValue has already been disposed [](start = 37, length = 67)

Should be a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

bug in the user code ? Probably. But they get a user friendly error message this way...

fs-eire
fs-eire previously approved these changes Mar 18, 2020
Copy link
Contributor

@fs-eire fs-eire left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@jignparm jignparm left a comment

Choose a reason for hiding this comment

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

LGTM.

@hariharans29 hariharans29 merged commit ef7b98f into master Mar 24, 2020
@hariharans29 hariharans29 deleted the DisposableNamedOnnxValue branch March 24, 2020 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error using output of one model as input for another
5 participants