-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Change TopK operator to allow dynamic 'k' #1829
Conversation
CC: @linkerzhang |
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.
LGTM. Thank you!
@houseroad any comments please? thanks a lot! |
@houseroad @linkerzhang - Is this change good to go ? Thanks! |
@hariharans29 - there was a failing backend test which is why the CI has failed. That test ( |
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.
Sorry to block:
Yes, we do use topk in pytorch-onnx-caffe2
https://github.com/pytorch/pytorch/blob/master/torch/onnx/symbolic.py#L1305
@houseroad - just FYI - the failing test was |
@houseroad - Sorry I don't quite understand. Could you please elaborate on the changes you are requesting ? |
@hariharans29 I don't mean you need to change anything, but we need to prepare for the change. Otherwise, it will break our internal pipeline. I am thinking of setting the pytorch onnx exporter to some stable opset by default. So such changes won't cause problem anymore. |
@houseroad - Got it. I will wait for your go-ahead. Thanks. |
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.
Good to ship now.
@houseroad - Thanks a lot! |
* More changes * More changes * More changes * More changes * Check-in modified model for topK * More changes * More changes * More cahnges * Changes to md files * More changes * More changes * Fix broken build * Fix build break * Fix build break * Fix build break * More changes * Fix build break * revert * revert file to old state
* More changes * More changes * More changes * More changes * Check-in modified model for topK * More changes * More changes * More cahnges * Changes to md files * More changes * More changes * Fix broken build * Fix build break * Fix build break * Fix build break * More changes * Fix build break * revert * revert file to old state
This PR is modifying TopK operator from having 'k' as attribute to having it as an input