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.

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

Make nemo text processing optional in TTS #10584

Merged
merged 8 commits into from
Oct 21, 2024
Merged

Conversation

blisc
Copy link
Collaborator

@blisc blisc commented Sep 23, 2024

What does this PR do ?

Moves common normalizer code into base class. Makes error soft rather than throwing hard ImportError

Collection: tts

Changelog

  • NeMo TN no longer throws errors in TTS collection

GitHub Actions CI

The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.

The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".

PR Type:

  • New Feature
  • Bugfix
  • Documentation

…er than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>
@github-actions github-actions bot added the TTS label Sep 23, 2024
Signed-off-by: blisc <blisc@users.noreply.github.com>
@blisc blisc added the Run CICD label Sep 23, 2024
@@ -28,9 +28,38 @@
from nemo.core.neural_types.neural_type import NeuralType
from nemo.utils import logging, model_utils

PYNINI_AVAILABLE = True
try:
import nemo_text_processing

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'nemo_text_processing' is not used.
rlangman
rlangman previously approved these changes Sep 23, 2024
Copy link
Collaborator

@rlangman rlangman left a comment

Choose a reason for hiding this comment

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

Looks alright. Left a few optional questions/comments.

Comment on lines +43 to +48
if not PYNINI_AVAILABLE:
logging.error(
"`nemo_text_processing` not installed, see https://github.com/NVIDIA/NeMo-text-processing for more details."
)
logging.error("The normalizer will be disabled.")
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be safer to throw an error if user supplies the text normalizer config?

Comment on lines +41 to +42
def _setup_normalizer(self, cfg):
if "text_normalizer" in cfg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: It might be cleaner to have _setup_normalizer() directly in the SpectrogramGenerator, with a constructor argument that lets the model class decide whether it is required or not. But not required, since most of this is legacy code anyways.

@blisc blisc added Run CICD and removed Run CICD labels Sep 26, 2024
@blisc blisc added Run CICD and removed Run CICD labels Sep 26, 2024
Signed-off-by: blisc <blisc@users.noreply.github.com>
@blisc blisc added Run CICD and removed Run CICD labels Sep 26, 2024
@blisc blisc added Run CICD and removed Run CICD labels Sep 30, 2024
Copy link
Contributor

This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Oct 15, 2024
@blisc blisc merged commit ff4e519 into NVIDIA:main Oct 21, 2024
292 of 300 checks passed
@blisc blisc deleted the nemo_tn_betterguard branch October 21, 2024 14:37
artbataev pushed a commit to artbataev/NeMo that referenced this pull request Oct 22, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
akoumpa pushed a commit that referenced this pull request Oct 24, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
yashaswikarnati pushed a commit that referenced this pull request Oct 24, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
titu1994 pushed a commit that referenced this pull request Oct 28, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
hainan-xv pushed a commit to hainan-xv/NeMo that referenced this pull request Nov 5, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
Signed-off-by: Hainan Xu <hainanx@nvidia.com>
HuiyingLi pushed a commit to HuiyingLi/NeMo that referenced this pull request Nov 15, 2024
* move TN guard to better location; make guard print error message rather than throwing error

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

* Forgot to add the actual normalizer

Signed-off-by: Jason <jasoli@nvidia.com>

* Apply isort and black reformatting

Signed-off-by: blisc <blisc@users.noreply.github.com>

---------

Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: blisc <blisc@users.noreply.github.com>
Co-authored-by: blisc <blisc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants