Fix loading of MagpieTTS and add test for all 3 versions#15941
Conversation
blisc
added
the
r3.0.0
aa2b3ac to
cec400b
Compare
Migrate checkpoints that contain conditioning-token rows while preserving BOS/EOS weights. Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: Jason <jasoli@nvidia.com>
Signed-off-by: Jason <jasoli@nvidia.com>
cec400b to
7c3d29a
Compare
|
/ok to test 7c3d29a |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Jason <jasoli@nvidia.com>
|
/ok to test 6b177e6 |
|
[🤖]: Hi @blisc 👋, We wanted to let you know that a CICD pipeline for this PR just finished successfully. So it might be time to merge this PR or get some approvals. |
| tokenizer_config.locale_specific_punct = True | ||
| # Persist punct_version=2 for HindiCharsTokenizer so .nemo save/restore | ||
| # always uses the expanded punctuation set (with dandas). | ||
| tokenizer_config.locale_specific_punct = not use_legacy_defaults |
There was a problem hiding this comment.
tokenizer_config.locale_specific_punct was default True, but your new change flips to False. Is this behavior expected? was wondering if we should choose the default use_legacy_defaults=False instead?
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure MagpieTTS checkpoints (including multiple published HF revisions) remain restorable with current NeMo code by adjusting tokenizer default persistence and expanding E2E coverage for model restore.
Changes:
- Adds an HF-based restore test that parametrically validates loading MagpieTTS across three published revisions.
- Updates the MagpieTTS E2E nightly runner script to enable download-marked tests.
- Modifies
setup_tokenizers()to persist tokenizer “version/default” fields into configs with a new legacy-defaults switch intended to preserve old token-to-ID mappings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/e2e_nightly/test_model_support_nvidia__magpie_tts_multilingual_357m.py |
Adds HF revision restore coverage for multiple MagpieTTS releases. |
tests/e2e_nightly/L2_Model_Support_nvidia__magpie_tts_multilingual_357m.sh |
Enables --with_downloads so the new HF restore test is executed in this job. |
nemo/collections/tts/data/text_to_speech_dataset_lhotse.py |
Introduces use_legacy_defaults in tokenizer setup to control backfilled tokenizer defaults during save/restore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def setup_tokenizers(all_tokenizers_config, mode='train', use_legacy_defaults=True): | ||
| # Being used in both model and worker_init_fn, so it is defined here | ||
| # Returns two tokenizers: one for TTS transcript and one for conditioning text (if needed) |
| # Persist charset_version so it's saved in .nemo archives and | ||
| # update_config_for_inference can distinguish old checkpoints | ||
| # (missing charset_version → v1) from new ones. | ||
| tokenizer_config.punct_version = 1 if use_legacy_defaults else 2 |
There was a problem hiding this comment.
Same concern as line 69 — new Hindi training now gets punct_version=1 by default.
There was a problem hiding this comment.
Ok how do you want to handle legacy checkpoint loading? #15567 broke loading the 2602 checkpoint because it doesn't have any of these parameters in the config and the tokenizer shape is now a mixmatch.
There was a problem hiding this comment.
these params should be configured with default values when any old model does nt have any.
Let me fix this bug.
What does this PR do ?
Fix loading of MagpieTTS and add test for all 3 versions
Collection: tts
Changelog
PR Type: