-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add Set terminate Option for user #985
base: main
Are you sure you want to change the base?
Conversation
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.
Is it possible to add a unit test for this feature?
Do we intend to expose this in other language bindings? |
…into abjindal/set_terminate
…into abjindal/set_terminate
@@ -65,7 +65,18 @@ void State::Run(OrtSession& session, int new_batch_size) { | |||
} |
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.
In session.Run(...) above on line 58, we don't do anything on termination?
The issue is that there's a potential error race. If the user calls 'SetTerminate()' and then a non terminate error was thrown from session.Run() the user will not know it was a non termination related error (unless they look at the string).
Can we detect termination from session.Run? If so, we should rename session_terminated to session_terminate_set and have a second variable for "session_terminated" for if we actually hit the termination case (or if we call ThrowErrorIfSessionTerminated, as that will catch the non session.Run terminate cases).
This way we separate requesting termination from hitting termination, and IsTerminated() should only return true if we hit termination. This does mean that IsTerminated() will return false after SetTerminate() is called, and won't return true until a function is called that checks for termination. This could be a problem.
@baijumeswani can review my thinking also.
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.
that's an interesting question. I believe the condition you are mentioning is rare, if the session is already terminated then it might be possible that the error might be different because of it being in terminated state. Also, if the error occurs because of some other reason, it should be produced in another run when the processing happens in non-terminated state and can be caught at that time.
…into abjindal/set_terminate
Since we wanted to add runoptions instead of individual APIs, created a new PR for this work #1011. Will close this once the PR is merged. |
Add an option to Terminate the current session and the session remains in the terminated state until the user calls the unset terminate option.