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 accuracy results #92

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dtrawins
Copy link

What does this PR do?

Fixing accuracy results with openvino adapter when sending multiple requests
Fixes # (issue)
Symptoms were with identical results returned for different sequential requests

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

self.infer_request.infer(dict_data)
return self.get_raw_result(self.infer_request)
result= self.infer_request.infer(dict_data)
return self.get_raw_result(result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this solve

Symptoms were with identical results returned for different sequential requests

? You simply moved from get_tensor() API to infer() return value

Copy link
Author

Choose a reason for hiding this comment

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

the main issue is in the infer_sync method which is returning the same results for sequential calls.
the fix was verified in the demo from https://github.com/openvinotoolkit/model_server/tree/stable-diffusion/demos/stable-diffusion

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the problem statement. I don't understand how this patch solves it. Can you explain?

Copy link
Author

Choose a reason for hiding this comment

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

it makes a copy of the response so it is not reusing the previous response

Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably misuse API. Is infer_sync() used in multithreaded environment? There's copy_raw_result() which copies data and infer_async() for running multiple inferences at the same time.

Copy link
Author

Choose a reason for hiding this comment

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

I'm afraid the problem is even in a single threaded application. Sequential infer_sync calls give incorrect responses.
copy_raw_results is not used as it is not a part of the inference adapter and is not needed in ovms adapter.

Copy link
Collaborator

@Wovchena Wovchena Jun 26, 2023

Choose a reason for hiding this comment

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

I need a way to reproduce it because I still don't understand how your patch fixes the problem. I wrote a reproducer: https://github.com/Wovchena/repr/blob/5a28ec94a8033b2b2a695bb1e2c0b110fb3b0913/repr.py. It gives different predictions for different images. I also tried calling model.inference_adapter.infer_sync() in the reproducer directly and it works fine for me without your patch.

I feel frustrated about this, so I'm starting to generate dumb ideas: You say

Sequential infer_sync calls give incorrect responses

Maybe you forget to call model.preprocess() before the second infer_sync()? I made this mistake while I was preparing the reproducer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, I will be on holidays this week, you should expect bigger delays of my replies.

Copy link
Collaborator

@Wovchena Wovchena Jul 7, 2023

Choose a reason for hiding this comment

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

I just realized. Since you are fine with this implementation, this means that self.infer_request.infer(dict_data) returns a copy as a result. And this copy is performed regardless of whether this implementation stores the returned value or discards.

The only possible problem is that model output tensor may have multiple names, but result dict may contain only one of them as key.

This means the overall idea of your PR is acceptable. Before I merge this, please, update get_raw_result() so the keys contained in returned value were from self.get_output_layers() but not just get_any_name().

Wovchena added a commit to Wovchena/repr that referenced this pull request Jun 26, 2023
Wovchena added a commit to Wovchena/repr that referenced this pull request Jun 26, 2023
Wovchena added a commit to Wovchena/repr that referenced this pull request Jun 26, 2023
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.

2 participants