-
Notifications
You must be signed in to change notification settings - Fork 319
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
Exec session updates #1173
Exec session updates #1173
Conversation
Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>
Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ExecutionSession::run( | ||
std::vector<std::unique_ptr<OMTensor, decltype(&omTensorDestroy)>> ins) { | ||
std::vector<OMTensorUniquePtr> ExecutionSession::run( | ||
std::vector<OMTensorUniquePtr> ins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the name OMTensorUniquePtr
, will use it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it in your recent code as well.
Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>
@etiotto recently introduce an alias for
unique_ptr<OMTensor, decltype(&omTensorDestroy)>
, and I used (a slightly renamed version of it) everywhere in the code.Also, our execution session was only supporting vectors of unique ptr of OMTensor, but not the native OMTensorList. I added a function to also support this.
This PR is short, a larger discussion is whether we should add also a C interface in our runtime for execution session, which essentially does dynamic linking of our entry points.