OWL-ViT box_predictor inefficiency issue#29712
Merged
amyeroberts merged 9 commits intohuggingface:mainfrom Mar 21, 2024
Merged
Conversation
amyeroberts
reviewed
Mar 18, 2024
Contributor
amyeroberts
left a comment
There was a problem hiding this comment.
Thanks for working on this!
Could you add the equivalent changes to owlv2, as in #29713 and add a contribution for @nvbinh15 in the commit?
Once that's done, all I need to see are the outputs for the slow model tests to confirm everything still runs as before:
RUN_SLOW=1 pytest tests/models/owlvit tests/models/owlv2 -vv
Contributor
There was a problem hiding this comment.
Unfortunately, because this is a public method and part of a model which is importable from the top level of transformers, we need to make this change so that it's backwards compatible to some degree. Since I doubt this is being called much, we can do something like the following:
def compute_box_bias(self, num_patches: int, feature_map: Optional[torch.FloatTensor] = None) -> torch.Tensor:
if feature_map is not None:
raise ValueError("feature_map has been deprecated as an input. Please pass in num_patches instead")
Contributor
Author
There was a problem hiding this comment.
Fixed in 83200135c7c78138a32c4aab84bda20c7fc24b4f commit
Contributor
There was a problem hiding this comment.
The good thing to do here is add a lru_cache decorator. Otherwise this is going to be recalculated every time it's called
Contributor
Author
There was a problem hiding this comment.
Fixed in ae0d8bc1ceaf2a01deb1774f25d48bd3dab90a8f commit.
I didn't found any other usage of compute_box_bias, so just added the maxsize as 2
Comment on lines
1349
to
1353
Contributor
There was a problem hiding this comment.
I wouldn't reset self.box_bias here - this effectively creates a side-effect in the function
Suggested change
| self.box_bias = self.box_bias.to(feature_map.device) | |
| pred_boxes += self.box_bias | |
| box_bias = self.box_bias.to(feature_map.device) | |
| pred_boxes += box_bias |
Contributor
Author
There was a problem hiding this comment.
Fixed in edd18742fe6cac818a49cb7d87ca281e87f7d934 commit
ddb96b7 to
5626dbd
Compare
Contributor
Author
|
Here are the result for the slow tests: |
Contributor
Author
|
Also, I am not sure whether is need to create a new PR or how to again trigger this PR with updated code. |
Contributor
|
@rvv-karma Thanks for sharing the test outputs! Here's a guide: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors If you don't have anything else to commit, you can do an empty commit to add the contribution e.g.: You'll need to run |
91eb253 to
99e1886
Compare
99e1886 to
5fe0e13
Compare
Contributor
Author
|
Thanks a lot @amyeroberts! Also, here is the output from |
amyeroberts
approved these changes
Mar 21, 2024
Contributor
amyeroberts
left a comment
There was a problem hiding this comment.
Amazing work - thanks again for adding this and improving our models! 🔥
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am calculating box_bias at the start while initialization, and storing it in a variable, then reusing it at inference. Also, shifting it to the correct memory location at the interence time.
I also converted the numpy implementation to torch implementation, and verified that both are giving the same results.
Fixes #26099
@5hadytru @amyeroberts
Can you please review?
I updated the code to the best of my knwoledge. Please let me know if you need any changes. Thanks.