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

Make yolov5 postprocessing work #786

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cchudant
Copy link
Contributor

Hello :)

This PR is made to make the yolov5 postprocessing (which uses the If operator with a non-constant condition!).

This PR is not supposed to be merged as-is, as it removes (maybe?) important checks...
Instead, i'd like to know what you think.

I'll make some comments directly on the code using the github review interface.

@kali
Copy link
Collaborator

kali commented Aug 11, 2022

I'm curious, what does yolo do that needs a non-constant if conditional ?

@cchudant
Copy link
Contributor Author

I'm curious, what does yolo do that needs a non-constant if conditional ?

I think it's conditionally calling NonMaxSuppression depending on the number of boxes found in the image. This is unmodified yolov5-rt-stack, so there is probably a way to rewrite the graph so that it does not use If without constant condition..

but i kinda want that onnx models that are often used Just Work without having to resort to weird rewrites

@kali
Copy link
Collaborator

kali commented Aug 11, 2022

No worries here. I fully agree that "common" networks should run out of the box in tract. Within reason of course: Tensor List and/or Sequence support would be another story as they would be a massively invasive change, but I don't think there hype anymore.

My question was more about... only one internal branch of the operator will be evaluated. This is good. But if there is heavy computation in some inputs which are not used by the "active" internal branch, then that can lead to waste. If it's too bad, we may need to make changes to the Plan, making the eval order conditional, and getting the plan one step closer to a bytecode runner.
One other way would be to detect the pattern (input used only by one of the branches) and pull these operation inside the iff.

But let's not get ahead of ourselves :) I'm happy to merge this once it is just working, we'll figure out what need to be optimised when it arises, if it arises.

@cchudant cchudant force-pushed the make_yolov5_postprocessing_work branch from 7fd406e to cc55da7 Compare September 14, 2022 12:12
@0xBYTESHIFT
Copy link

what's the status of it now? does yolov5 work in tract?

@cchudant
Copy link
Contributor Author

@0xBYTESHIFT

what's the status of it now? does yolov5 work in tract?

YoloV5 works without any problem!
It's just that the postprocessing that they use in yolov5rt with torchvision.nonmaxsuppression gets translated to something that has an ONNX If operator that does not make a lot of sense
I managed to change the code of the postprocessing on our end.
I don't know whether it works on tract upstream master now, or whether we had to make some other ugly patches on our fork that we forgot to upstream

I wanted to make sure we tidy up and upstream everything we do in this PR, but i am very short-handed and i realized I don't have the bandwidth for tract contributions in general. which is kind of sad :(

I think we upstreamed the most important patches though, and we are not currently working on adding support on any new model

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.

3 participants