-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add ONNXModifier for optimising the ONNX model before converting for RVC4 execution #55
Conversation
…odel before converting to RVC4
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.
Thanks! LGTM in general, just left a few comments.
import onnx | ||
import onnx_graphsurgeon as gs |
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.
Could we use graphsurgeon
to also simplify the code in onnx_attach_normalization_to_inputs
? Or add it to the ONNXModifier
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.
Even though I would like the onnx_attach_normalization_to_inputs
to be another function of ONNXModifier
, I think its better to leave this one outside of the class. The reason is that its the fallback solution in case the optimisations fail or do not produce the correct results compared to the "base" model which is produced by this exact function.
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.
But the onnx_attach_normalization_to_inputs
alone can be changed to use graphsurgeon
instead, yeah.
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.
Looks good, some comments mostly regarding where and what operations to check.
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
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
The ONNXModifier performs a series of onnx modifications that were found to be beneficial in terms of performance on RVC4, without altering the final outputs of the model. Below is the list of optimisations/modifications introduced: