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.

Sign up for GitHub

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

Jump to bottom

bert_module: fix inputs of export model #3815

Merged
merged 2 commits into from
Mar 11, 2022
Merged

bert_module: fix inputs of export model #3815

merged 2 commits into from
Mar 11, 2022

Conversation

virajkarandikar
Copy link
Contributor

@virajkarandikar virajkarandikar commented Mar 9, 2022 •
edited
Loading

Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

What does this PR do ?

Fix swapped model inputs for exported ONNX/TRT model.

Collection: NLP

Changelog

  • Fix order of inputs returned via input_types()

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests? - Updated related tests.
  • Did you add or update any necessary documentation? - Not required
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc) - No
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • Bugfix

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

@MaximumEntropy, @ericharper, @ekmb, @yzhang123

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue)

@virajkarandikar virajkarandikar force-pushed the vkarandikar/fix_bert_model_inputs branch from eaca787 to 2cd9f4f Compare March 9, 2022 11:34
@virajkarandikar virajkarandikar changed the title Draft: bert_module: fix model inputs when exported bert_module: fix model inputs when exported Mar 9, 2022
@virajkarandikar virajkarandikar force-pushed the vkarandikar/fix_bert_model_inputs branch 2 times, most recently from 2bf0c12 to dcac1ab Compare March 9, 2022 18:44
@virajkarandikar virajkarandikar marked this pull request as ready for review March 9, 2022 18:45
@virajkarandikar virajkarandikar force-pushed the vkarandikar/fix_bert_model_inputs branch from dcac1ab to 8d275c7 Compare March 9, 2022 18:47
@virajkarandikar virajkarandikar changed the title bert_module: fix model inputs when exported bert_module: fix inputs of export model Mar 9, 2022
@virajkarandikar
bert_module: fix inputs of exported model
079e148 
Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>
@virajkarandikar virajkarandikar force-pushed the vkarandikar/fix_bert_model_inputs branch from 8d275c7 to 079e148 Compare March 9, 2022 18:48
@ryanleary
Copy link
Contributor

Changes look OK to me.

@yzhang123
Copy link
Contributor

@borisfom please double check before we merge

@borisfom borisfom merged commit 3dd8a5c into NVIDIA:main Mar 11, 2022
crcrpar pushed a commit to crcrpar/NeMo that referenced this pull request Mar 11, 2022
@virajkarandikar @yzhang123 @crcrpar
bert_module: fix inputs of exported model (NVIDIA#3815)
ea22773 
Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
fayejf pushed a commit that referenced this pull request Mar 12, 2022
@virajkarandikar @yzhang123 @fayejf
bert_module: fix inputs of exported model (#3815)
d73e886 
Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
ericharper added a commit that referenced this pull request Mar 14, 2022
@crcrpar @yzhang123
@virajkarandikar @ericharper
Fix MegatronPretrainingRandomSampler by taking data_parallel_size i…
1e1afc5 
…nto account (#3826)

* initial fix: taking data parallel size into consideration

Signed-off-by: Masaki Kozuki <[email protected]>

* update the signature

Signed-off-by: Masaki Kozuki <[email protected]>

* data_parallel_rank -> data_parallel_size

Signed-off-by: Masaki Kozuki <[email protected]>

* fix typo

Signed-off-by: Masaki Kozuki <[email protected]>

* cosmetic changes

Signed-off-by: Masaki Kozuki <[email protected]>

* Revert "update the signature"

This reverts commit 1c134e5.

Signed-off-by: Masaki Kozuki <[email protected]>

* update batch_sampler arguments

Signed-off-by: Masaki Kozuki <[email protected]>

* change how to slice `batch`

Signed-off-by: Masaki Kozuki <[email protected]>

* update not drop_last path

Signed-off-by: Masaki Kozuki <[email protected]>

* add fr asr ckpt to doc (#3809)

Signed-off-by: Yang Zhang <[email protected]>

* bert_module: fix inputs of exported model (#3815)

Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>

* update random batch sampler

Signed-off-by: Masaki Kozuki <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
Co-authored-by: Viraj Karandikar <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
fayejf pushed a commit that referenced this pull request Mar 22, 2022
@virajkarandikar @yzhang123 @fayejf
bert_module: fix inputs of exported model (#3815)
a8f2866 
Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
fayejf pushed a commit that referenced this pull request Mar 22, 2022
@crcrpar @yzhang123
@virajkarandikar @ericharper @fayejf
Fix MegatronPretrainingRandomSampler by taking data_parallel_size i…
f31463a 
…nto account (#3826)

* initial fix: taking data parallel size into consideration

Signed-off-by: Masaki Kozuki <[email protected]>

* update the signature

Signed-off-by: Masaki Kozuki <[email protected]>

* data_parallel_rank -> data_parallel_size

Signed-off-by: Masaki Kozuki <[email protected]>

* fix typo

Signed-off-by: Masaki Kozuki <[email protected]>

* cosmetic changes

Signed-off-by: Masaki Kozuki <[email protected]>

* Revert "update the signature"

This reverts commit 1c134e5.

Signed-off-by: Masaki Kozuki <[email protected]>

* update batch_sampler arguments

Signed-off-by: Masaki Kozuki <[email protected]>

* change how to slice `batch`

Signed-off-by: Masaki Kozuki <[email protected]>

* update not drop_last path

Signed-off-by: Masaki Kozuki <[email protected]>

* add fr asr ckpt to doc (#3809)

Signed-off-by: Yang Zhang <[email protected]>

* bert_module: fix inputs of exported model (#3815)

Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>

* update random batch sampler

Signed-off-by: Masaki Kozuki <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
Co-authored-by: Viraj Karandikar <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
fayejf pushed a commit that referenced this pull request Mar 22, 2022
@virajkarandikar @yzhang123 @fayejf
bert_module: fix inputs of exported model (#3815)
3fcc364 
Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
fayejf pushed a commit that referenced this pull request Mar 22, 2022
@crcrpar @yzhang123
@virajkarandikar @ericharper @fayejf
Fix MegatronPretrainingRandomSampler by taking data_parallel_size i…
c6de98a 
…nto account (#3826)

* initial fix: taking data parallel size into consideration

Signed-off-by: Masaki Kozuki <[email protected]>

* update the signature

Signed-off-by: Masaki Kozuki <[email protected]>

* data_parallel_rank -> data_parallel_size

Signed-off-by: Masaki Kozuki <[email protected]>

* fix typo

Signed-off-by: Masaki Kozuki <[email protected]>

* cosmetic changes

Signed-off-by: Masaki Kozuki <[email protected]>

* Revert "update the signature"

This reverts commit 1c134e5.

Signed-off-by: Masaki Kozuki <[email protected]>

* update batch_sampler arguments

Signed-off-by: Masaki Kozuki <[email protected]>

* change how to slice `batch`

Signed-off-by: Masaki Kozuki <[email protected]>

* update not drop_last path

Signed-off-by: Masaki Kozuki <[email protected]>

* add fr asr ckpt to doc (#3809)

Signed-off-by: Yang Zhang <[email protected]>

* bert_module: fix inputs of exported model (#3815)

Exported ONNX model had to be passed with "attention_mask" and
"token_type_ids" inputs swapped to get correct output.

Changing the input order fixes this issue.
Also return a dict instead of list for correctly passing inputs.

Update relevant tests:
test_TokenClassificationModel_export_to_onnx
test_PunctuationCapitalizationModel_export_to_onnx
test_QAModel_export_to_onnx

Signed-off-by: Viraj Karandikar <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>

* update random batch sampler

Signed-off-by: Masaki Kozuki <[email protected]>

Co-authored-by: Yang Zhang <[email protected]>
Co-authored-by: Viraj Karandikar <[email protected]>
Co-authored-by: Eric Harper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yzhang123 yzhang123 yzhang123 approved these changes

@borisfom borisfom borisfom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@virajkarandikar @ryanleary @yzhang123 @borisfom