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
Added onnx config whisper #19525
Merged
Merged
Added onnx config whisper #19525
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
3c7c712
Added onnx config whisper
mht-sharma da52a43
added whisper support onnx
mht-sharma f61ae96
add audio input data
mht-sharma 79ad032
added whisper support onnx
mht-sharma 8bd740b
fixed the seqlength value
mht-sharma ba50fa3
Updated the whisper onnx ocnfig
mht-sharma f9557dd
restore files to old version
mht-sharma 85fdb68
removed attention mask from inputs
mht-sharma b1a2e4a
Updated get_dummy_input_onnxruntime docstring
mht-sharma 59a5965
Updated relative imports and token generation
mht-sharma 0d1904c
update docstring
mht-sharma File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
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.
If I'm not mistaken, this function doesn't do anything - why do we need it?
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.
Hi @lewtun , this is true that this function does not do anything for existing models. However, this function can be overridden in some cases where the model inputs and ONNX Runtime inputs are different. This was needed when exporting the decoder in encoder-decoder models using encoder_outputs. For example: Check the following Decoder config in optimum where I am using the function DecoderOnnxConfig.
Since we are waiting for the optimum PR to merge and migrating the changes this is no longer needed in the current merge.
But we still need to update the
VisionEncoderDecoderConfig
using encoder_outputs in that case we would need such function. Should we merge with this and utilise this in newVisionEncoderDecoder
PR (In case the user wants to create the PR for this change, this option would become easier for him)? Or we could remove it from here and add the function along with the new PR?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.
I see, thanks for the clarification! Looking at the
optimum
code, it seems like we use this function to add new fields to the ORT inputs - is there a reason we can't capture that logic in a singlegenerate_dummy_inputs()
function associated with the ONNX config?(I'm happy to include this function as is, just trying to understand if we really need it or not)
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.
The function (at least in this case) update the existing keys in the input dict, but the values remains the same.
For exporting these models we would require 2 different input sets. This is because the
model.forward()
has a different input signature (since this is the full model before export) that the generated ONNX model (only decoder is exported). Therefore we need a way to alter the existing model inputs to run inference with both models invalidate_model_outputs
.I think creating a separate function is a much cleaner way. But I am open to any suggestions. We can add the logic in the
generate_dummy_inputs
by adding a new argument to the function, maybe calledort_inputs=True/False
, and for these models we return different sets of inputs. But this will require updation of all the OnnxConfigs having this 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.
OK great, now I understand well why we need this - thanks. IMO it's fine to include this function in this PR if we add a small note in the docstring like "Override to run inference with seq2seq models which have the encoder and decoder exported as separate ONNX files."
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.
Done