Skip to content

Conversation

@bmwshop
Copy link
Collaborator

@bmwshop bmwshop commented Apr 22, 2023

What does this PR do ?

Adding the conv_pointwise_type parameter that to ConformerEncoder enabling the use of nn.Linear() for a 10% acceleration

Collection: ASR

Changelog

Adding the conv_pointwise_type parameter to

  • nemo/collections/asr/modules/conformer_encoder.py : adding the conv_pointwise_type parameter and propagating it to ConformerLayer
  • nemo/collections/asr/parts/submodules/conformer_modules.py: adding conv_pointwise_type parameter to ConformerLayer and propagating it to ConformerConvolution.
  • in ConformerConvolution, if conv_pointwise_type is 'linear', use nn.Linear() instead of nn.Conv1d()

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this 
+model.encoder.conv_pointwise_type='linear'

Before your PR is "Ready for review"

Pre checks:

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

PR Type:

  • New Feature
  • Bugfix
  • Documentation

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

Who can review?

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)

Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
@github-actions github-actions bot added the ASR label Apr 22, 2023
bmwshop and others added 2 commits April 22, 2023 11:01
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
@bmwshop bmwshop changed the title created conv_pointwise_type parameter Apr 22, 2023
bmwshop and others added 3 commits April 22, 2023 13:57
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
@bmwshop bmwshop marked this pull request as ready for review April 23, 2023 04:18
@bmwshop bmwshop requested a review from titu1994 April 23, 2023 04:18
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 good. But needs to be documented in Nemo ASR docs config section

@bmwshop
Copy link
Collaborator Author

bmwshop commented Apr 25, 2023

Looks good. But needs to be documented in Nemo ASR docs config section

should we add to this PR or do a separate PR for docs?

@titu1994
Copy link
Collaborator

Same PR would be good

bmwshop added 2 commits May 5, 2023 14:42
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
@bmwshop
Copy link
Collaborator Author

bmwshop commented May 5, 2023

Same PR would be good
Added some docs - was not sure how to best do it, ended up appending a section on conformer encoder


The only condition that needs to be met is that **the final layer of the acoustic model must have the hidden dimension defined in ``model_defaults.enc_hidden``**.

Conformer Encoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this for fast conformer ? Some flags are for fast conformer

self.pointwise_conv2 = nn.Conv1d(
in_channels=dw_conv_input_dim, out_channels=d_model, kernel_size=1, stride=1, padding=0, bias=True
)
elif self.conv_pointwise_type == 'linear':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder. Conv1d with kernel 1 is identical to linear layer, so what if we do something like silently convert the conv kernel shape to linear compatible weights and don't use explicitly the Lineae layer but use functional.linear() instead ?

My assumption is we don't want to break checkpoint compatibility - and we don't need to. We can take a model trained on conv1d and use the weight in a F.linear() after weight conversion - this way no checkpoint incompatibility is there for old models but still there is speedup

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.

Minor comments

bmwshop and others added 2 commits May 6, 2023 09:31
Signed-off-by: Dima Rekesh <bmwshop@gmail.com>
@bmwshop bmwshop closed this May 10, 2023
@ArtyomZemlyak
Copy link
Contributor

@bmwshop
Hi!
I would like to ask why you decided to close the pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants