TTS Collection

[blisc]

Adds TTS to candidate

Changes outside TTS collection

ASR collection

• rename _AudioDataset to _AudioTextDataset
• _AudioTextDataset's signature changed, it no longer accepts featurizer but accepts sample_rate, int_values, and augmentor so that it can instantiate WaveformFeaturizer itself.
• AudioPreprocessor no longer has a get_seq_len function. get_features is now expected to return processed_signal and processed_length

Core collection

• Adds LogEpochTimeCallback to nemo.collections.core.callbacks

Core

• from_config_dict now accepts *args and **kwargs passthroughs to hydra.utils.instantiate
• Tweaked error messages inside type checking so it is more informative

TODO

• Add dataclasses to most modules
• Merge ASR and TTS data pipeline
• Add unit tests
• Class and function docstrings
• Be NeMo 1.0 Class compliant: Broken into more tasks
• Change loss to inherit from NeMo Loss
• Add typing to models
• Figure out typing and validation for WaveGlow
• Drop experimental decorators
• Use hydra_runner
• Maybe move LogEpochTimeCallback to common
• Merge Flatten pl.trainer to trainer #895

[lgtm-com]

This pull request introduces 1 alert when merging e173943 into b883b31 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 1 alert when merging 3f48fac into 054cc02 - view on LGTM.com

new alerts:

• 1 for Unused import

[okuchaiev]

Overall it looks good, but there are few small issues here and there - please see comments.

[okuchaiev]

you'll only need to implement "list_available_model()" once you have cloud locations

[blisc]

Now I get TypeError: Can't instantiate abstract class Tacotron2Model with abstract methods export

[blisc]

And hydra.errors.HydraException: Error calling 'nemo.collections.tts.modules.tacotron2.Encoder' : Can't instantiate abstract class Encoder with abstract methods restore_from, save_to

[okuchaiev]

"Can't instantiate abstract class Tacotron2Model with abstract method" this should not happen if you merge/rebase latest candidate into your branch

[okuchaiev]

this class should go to nemo/collections/tts/losses/* or to nemo/collections/common/losses/* and also get some docstring

[blisc]

Why move it? This loss is only ever going to be used in this model. It is so redundant that I have to define it as it's own separate class rather than a function of Tacotron2Model.

[blisc]

With the new function-level override of typecheck(), it doesn't even need to inherit from Typing to use NeMo's typing. Tacotron2Loss doesn't have an init nor does it use self, it's a function.

[okuchaiev]

For consistency. We put all losses other losses under collections/collection_name/losses/* . why make exception here?

[blisc]

Moved.
I maintain that collections/collection_name/losses/* shouldn't exist apart from losses inside common. NLP's losses is empty and ASR only has CTC that is only used in CTC models so it should be moved there and losses folder should be removed.

[blisc]

Let me qualify my above statement. If a loss is only ever going to be used in a single model, it should not be in the losses folder. And if the losses folder is empty, it should be deleted.

[titu1994]

Side note, It is incorrect that the class does not need to implement Typing. All of the actual tests lie inside the Typing class, and the decorator simply dispatches these checks. This is by design to support graph level checks. The decor will raise Runtime error if placed on a class that doesn't implement Typing.

[okuchaiev]

what is the problem with TB for training?

[okuchaiev]

@titu1994 @ericharper @tkornuta-nvidia - let's chat (outside of this PR) if we'd like to adapt something like this in general for Models

[okuchaiev]

true - feel free to remove

[titu1994]

Minor comments regarding possible refactor, but overall not necessary to merge. LGTM

[titu1994]

How about "should_shuffle_train_val"

[titu1994]

Mode should be unnecessary after the function override of typecheck if you would like to refactor, but this is fine too.