-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[IE PYTHON] wait for infer callback to complete #7374
[IE PYTHON] wait for infer callback to complete #7374
Conversation
@@ -350,7 +351,7 @@ def execute(self, input_data): | |||
self.cv.release() | |||
status = self.request.wait(ie.WaitMode.RESULT_READY) | |||
assert status == ie.StatusCode.OK | |||
assert self.status_code == ie.StatusCode.OK | |||
assert self.status_code == ie.StatusCode.RESULT_NOT_READY |
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.
It looks strange from API level perspective. User calls wait with wait mode == RESULT_READY
, but receives RESULT_NOT_READY
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 am curious about it, why we get RESULT_NOT_READY
? I understand the idea behind it is to capture status inside callback which will be equal to RESULT_NOT_READY
, and only after returning to python we can obtain OK
. However this could render previous callbacks' "policy" absolutely obsolete. The behavior was even present around March of the current year:
Lines 24 to 29 in 6478f17
def callback(self, statusCode, userdata): | |
if (userdata != self.id): | |
log.error(f"Request ID {self.id} does not correspond to user data {userdata}") | |
elif statusCode != 0: | |
log.error(f"Request {self.id} failed with status code {statusCode}") | |
self.cur_iter += 1 |
I totally understand an idea to be able to call wait inside callback itself. However why we are still passing "the same" status code as a parameter? To my knowledge this statusCode
would be equal to OK
as cpp implementation marks it this way as we are able to enter a callback and not to confuse users with different names of callback's exit codes. The parameter is here:
def callback(self, statusCode, userdata): |
This needs a follow-up (in my opinion) as it requires:
- either reconsidering a behavior of the callbacks in python and picking of the one "true" way of obtaining the status inside the callback
- or changing c++ core code to apply aligned behavior in both APIs.
@nkogteva @akuporos what are your views on that?
@AlexeyLebedev1 could you elaborate more on this change?
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.
@jiwaszki From my point of view, we can't disallow the user to call wait inside callback and vice versa we should not force to do it just to check the status code. I agree that it is strange if status codes received in these two ways would be different. At least it should be documented.
But anyway the behavior should be aligned with C++. cc @ilya-lavrenov
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.
The main point of these changes that InferRequest.wait(RESULT_READY) must block the thread until infer_callback function completes, as in c++ api. When we call wait inside callback with no-wait status, we get RESULT_NOT_READY, also like in c++ api and it can confuse user, i agree, but if we want to wait for infer_callback maybe it makes sense.
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.
One more note: as far as I know user can access output blobs inside callback like in demo pipeline. Thus it makes sense to provide the correct status code which informs of present state - which is OK
. @ilya-lavrenov is it possible to obtain results in C++ API's callbacks as well? If yes, we should consider changes to C++ API to return OK
in callback or some new status that is clearer about output blobs status instead of focusing on InferRequest's "life".
If we are going to align code to C++ without any changes then it should be properly documented and reflected on the python side.
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.
How i knew from @ilya-lavrenov , c++ api supposses that callback is the last stage of inference, that's why InferRequest.wait(0)
inside callback returns RESULT_NOT_READY. I don't see why we should worry about this status code
self.status_code = self.request.wait(ie.WaitMode.STATUS_ONLY) |
def callback(self, statusCode, userdata): |
openvino/inference-engine/ie_bridges/python/src/openvino/inference_engine/ie_api_impl.cpp
Lines 538 to 545 in f79d649
if (code != InferenceEngine::StatusCode::OK) { | |
IE_EXCEPTION_SWITCH(code, | |
ExceptionType, | |
InferenceEngine::details::ThrowNow<ExceptionType>{} <<= | |
std::stringstream{} | |
<< IE_LOCATION | |
<< InferenceEngine::details::ExceptionTraits<ExceptionType>::string()); | |
} |
inference-engine/ie_bridges/python/src/openvino/inference_engine/ie_api.pyx
Show resolved
Hide resolved
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 love the concept of the change however it needs some refining. Please also look at this:
Lines 123 to 133 in 9e68a67
for i in output_queue: | |
# Immediately returns a inference status without blocking or interrupting | |
infer_status = exec_net.requests[i].wait(0) | |
if infer_status == StatusCode.RESULT_NOT_READY: | |
continue | |
log.info(f'Infer request {i} returned {infer_status}') | |
if infer_status != StatusCode.OK: | |
return -2 |
There can be a lot more places where this behavior occurs - please look through the project where the similar "no-wait" call can occur, also with a usage of ExecutableNetwork since it is using the same mechanisms underneath.
@@ -350,7 +351,7 @@ def execute(self, input_data): | |||
self.cv.release() | |||
status = self.request.wait(ie.WaitMode.RESULT_READY) | |||
assert status == ie.StatusCode.OK | |||
assert self.status_code == ie.StatusCode.OK | |||
assert self.status_code == ie.StatusCode.RESULT_NOT_READY |
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 am curious about it, why we get RESULT_NOT_READY
? I understand the idea behind it is to capture status inside callback which will be equal to RESULT_NOT_READY
, and only after returning to python we can obtain OK
. However this could render previous callbacks' "policy" absolutely obsolete. The behavior was even present around March of the current year:
Lines 24 to 29 in 6478f17
def callback(self, statusCode, userdata): | |
if (userdata != self.id): | |
log.error(f"Request ID {self.id} does not correspond to user data {userdata}") | |
elif statusCode != 0: | |
log.error(f"Request {self.id} failed with status code {statusCode}") | |
self.cur_iter += 1 |
I totally understand an idea to be able to call wait inside callback itself. However why we are still passing "the same" status code as a parameter? To my knowledge this statusCode
would be equal to OK
as cpp implementation marks it this way as we are able to enter a callback and not to confuse users with different names of callback's exit codes. The parameter is here:
def callback(self, statusCode, userdata): |
This needs a follow-up (in my opinion) as it requires:
- either reconsidering a behavior of the callbacks in python and picking of the one "true" way of obtaining the status inside the callback
- or changing c++ core code to apply aligned behavior in both APIs.
@nkogteva @akuporos what are your views on that?
@AlexeyLebedev1 could you elaborate more on this change?
Thanks, i also will update ExecutableNetwork.wait() |
* test commit * Add test * Remove threading.event from InferRequest * Fix wait for ExecutableNetwork
Tickets: