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

Adding RNN encoder for LSTM-Transducer and LSTM-CTC models #3886

Merged
merged 40 commits into from
Apr 2, 2022
Merged

Adding RNN encoder for LSTM-Transducer and LSTM-CTC models #3886

merged 40 commits into from
Apr 2, 2022

Conversation

VahidooX
Copy link
Collaborator

@VahidooX VahidooX commented Mar 25, 2022 •
edited
Loading

What does this PR do ?

This PR adds RNN-based encoder to NeMo. It enables us to have LSTM-Transducer (RNN-T) and LSTM-CTC models.

Changelog

  • Added RNN encoder for ASR models (LSTM-T and LSTM-CTC models)
  • Added stacking downsampling
  • Added support for proj_size to Transducer decoders
  • Added skip_nan_grad support for ASR models which skips the gradients when there is a nan or inf in the gradients

Usage

# Add a code snippet demonstrating how to use this

PR Type:

  • [x ] New Feature
  • Bugfix
  • Documentation

VahidooX added 19 commits January 6, 2022 15:41
@VahidooX
added initial rnn encoder.
81ed609 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added rnn encoder and decoder.
f072143 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added stackingdownsampling.
a719cdf 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added stackingdownsampling.
948186a 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added stackingdownsampling.
d303b43 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
fixed the bug for bidirectional.
6781679 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
fixed the bug for bidirectional.
267be30 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added skip_nan_grad.
fd0da28 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added skip_nan_grad.
fdfac60 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added rnn tpype.
e3e9edf 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added rnn tpype.
f2df1d9 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
cleaned the configs.
2d48b86 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
cleaned the configs.
8f0f844 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added docs.
d29af7f 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added docs.
46cb7f7 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added docs.
d01ec23 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
changed proj_out to proj_size
9892388 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
changed proj_out to proj_size
c38f70c 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
changed proj_out to proj_size
a189aa0 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX VahidooX marked this pull request as ready for review March 26, 2022 00:12
@VahidooX VahidooX requested a review from titu1994 March 26, 2022 00:13
@lgtm-com
Copy link

lgtm-com bot commented Mar 26, 2022

This pull request introduces 3 alerts when merging a189aa0 into e188f36 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

VahidooX added 2 commits March 25, 2022 19:44
@VahidooX
set default to bpe.
e9bb886 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
cleaned.
3e7bd6b 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 26, 2022

This pull request introduces 2 alerts when merging e9bb886 into e188f36 - view on LGTM.com

new alerts:

  • 2 for Unused import

titu1994
titu1994 reviewed Mar 29, 2022
View reviewed changes
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, minor comments here and there.

docs/source/asr/configs.rst Outdated Show resolved Hide resolved
docs/source/asr/models.rst Outdated Show resolved Hide resolved
docs/source/asr/models.rst Outdated Show resolved Hide resolved
examples/asr/conf/conformer/conformer_ctc_bpe.yaml Show resolved Hide resolved
examples/asr/conf/rnn/rnn_ctc_bpe.yaml Outdated Show resolved Hide resolved
nemo/collections/asr/modules/rnn_encoder.py Outdated Show resolved Hide resolved
nemo/collections/asr/modules/rnn_encoder.py Show resolved Hide resolved
nemo/collections/asr/modules/rnn_encoder.py Outdated Show resolved Hide resolved
nemo/collections/asr/modules/rnn_encoder.py Outdated Show resolved Hide resolved
nemo/core/classes/modelPT.py Outdated Show resolved Hide resolved
VahidooX added 4 commits March 30, 2022 17:01
@VahidooX
addressed comments.
285d918 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
CHANGED names.
851324f 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
CHANGED names.
75a9984 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
added types.
9166d5b 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX VahidooX requested a review from titu1994 March 31, 2022 00:43
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request fixes 1 alert when merging d1355a0 into 84236ba - view on LGTM.com

fixed alerts:

  • 1 for Unused import

VahidooX added 2 commits March 31, 2022 00:43
@VahidooX
Merge branch 'main' of https://github.com/NVIDIA/NeMo into add_rnn_en…
d6e2db2 
…coder_main
@VahidooX
fixed style.
6649533 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request fixes 1 alert when merging 6649533 into ca8a7e0 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@VahidooX VahidooX changed the title Adding RNN encoder for RNN-Transducer and RNN-CTC models Adding RNN encoder for LSTM-Transducer and LSTM-CTC models Mar 31, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request fixes 1 alert when merging 253d441 into b1b6e5e - view on LGTM.com

fixed alerts:

  • 1 for Unused import

VahidooX added 2 commits March 31, 2022 16:04
@VahidooX
Merge branch 'main' of https://github.com/NVIDIA/NeMo into add_rnn_en…
4a9aec4 
…coder_main
@VahidooX
pull from main.
7ff3717 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request fixes 5 alerts when merging 7ff3717 into 5c88c8d - view on LGTM.com

fixed alerts:

  • 5 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2022

This pull request fixes 5 alerts when merging 20d81ec into 262fe05 - view on LGTM.com

fixed alerts:

  • 5 for Unused import

titu1994
titu1994 requested changes Apr 1, 2022
View reviewed changes
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just reuse model_defaults.pred_hidden instead of add yet another variable proj_size at model_defaults level.

Things in model_defaults are :

  1. Global to a model archetype (not just 1 model like LSTMs here)
  2. Referred to multiple places in the model config (so user can modify the global one and propagate to rest of config).

proj_size is effectively pred_hidden here so I don't see why should split them into two.

nemo/collections/asr/models/rnnt_bpe_models.py Outdated Show resolved Hide resolved
nemo/collections/asr/models/rnnt_models.py Outdated Show resolved Hide resolved
nemo/collections/asr/modules/rnn_encoder.py Show resolved Hide resolved
VahidooX added 3 commits April 1, 2022 16:06
@VahidooX
pulled from main.
6df0349 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
replaced proj_size with pred_hidden.
cee5e0e 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX
replaced proj_size with pred_hidden.
abbff06 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
titu1994
titu1994 previously approved these changes Apr 2, 2022
View reviewed changes
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great ! Merge when ready

@VahidooX
replaced proj_size with pred_hidden.
35a2a5e 
Signed-off-by: Vahid <vnoroozi@nvidia.com>
@VahidooX VahidooX requested a review from titu1994 April 2, 2022 06:57
@titu1994 titu1994 merged commit 087de54 into NVIDIA:main Apr 2, 2022
@VahidooX VahidooX deleted the add_rnn_encoder_main branch April 4, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@titu1994 titu1994 titu1994 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.
2 participants
@VahidooX @titu1994