MagpieTTS decoder model on top of NeMo main branch

[paarthneekhara]

No description provided.

[blisc]

Some more comments from WIP review

[blisc]

@XuesongYang Can you take a look at the dataset setup and validation logging especially since you recently made changes to these inside of the magpie class

[XuesongYang]

I checked the model class and the Lhotse YAML config, and I can confirm that this model doesn’t currently support multiple validation dataloaders.

Since this PR is already quite large and aggregates a significant number of changes, let’s avoid adding multiple dataloader support here. Instead, we can plan to implement that in a separate PR (similar to #15348).

[subhankar-ghosh]

Why do we have these imports in the constructor? These imports might cause issues during model loading while doing inference. IMO we should not include evaluation metrics in the class. Keep it separate like we do in MagpieTTS.

[paarthneekhara]

Moved them above.

[subhankar-ghosh]

Is it possible to reuse the compute_loss method from MagpieTTS? I think this is calculating the same loss if we pass frame_stacking_factor=1.

[paarthneekhara]

I am not wanting to reuse it because it uses several self variables. @rlangman suggested to dedupe code only if the function is reusable across other models as well. I dont think it is very reusable and it's just additive cross-entropy loss. Also the function is fairly small (~20 lines) so I am not too inclined towards a shared implementation.

[subhankar-ghosh]

This change can be part of a later PR. Can we move log_audio_* and log_plot_* to helper files and reuse them across models? I think we can reuse a lot of these plot and log methods from MagpieTTS.

[paarthneekhara]

That makes sense to me. I'll make another PR once this is merged to unify the audio/image logging.

[subhankar-ghosh]

do we really need cer/wer and ssim during validation step? It might make sense for experimentation but for open sourcing it does not make sense especially after we have a working recipe. Makes val step heavier.

[paarthneekhara]

I find it quite helpful to track CER/SSIM during training to know if the model is working and also comparing different experiments. Val loss comparison often doesnt translate to which experiment is doing well. The overhead of this step is not much since it is spread across 4 nodes, so less than 10 iterations of the infer_batch cover the entire val set.

[XuesongYang]

any reason why subclassing inference class for pretraining? aren't we expecting inference subclassing pretraining class?

[XuesongYang]

Generally work through the general design, the new class hierarchy (ModelPT -> EasyMagpieTTSInferenceModel -> EasyMagpieTTSModel  -> EasyMagpieTTSModelOnlinePO) operates independently from the existing MagpieTTS class.

Because it doesn’t subclass MagpieTTS, any features added to MagpieTTS won’t be inherited without careful cherry-picking, and I already noticed a few manually copy-pasted code blocks. If this implementation is meant to be a backbone for future models rather than a one-off recipe, it is worth rethinking the architecture now to ensure better long-term maintainability.

[XuesongYang]

pls add the two latest changes:

1. [MagpieTTS][bugfix] defaults to force_map_dataset=True for validation datast to avoid duplicates by a factor of num_workers. #15387
2. [MagpieTTS] set seed and shard_seed=randomized for val dataloader to ensure deterministic multi-GPU eval #15427

[blisc]

@paarthneekhara Let's make these changes.

[paarthneekhara]

Thanks for pointing these. I have made these changes.

[XuesongYang]

I checked the model class and the Lhotse YAML config, and I can confirm that this model doesn’t currently support multiple validation dataloaders.

Since this PR is already quite large and aggregates a significant number of changes, let’s avoid adding multiple dataloader support here. Instead, we can plan to implement that in a separate PR (similar to #15348).