sonos
Make yolov5 postprocessing work
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.
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
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.
@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