CherryPick: Fix loading of MagpieTTS and add test for all 3 versions (#15941) - #15979
Merged
Conversation
* Fix legacy MagpieTTS text embedding restore Migrate checkpoints that contain conditioning-token rows while preserving BOS/EOS weights. Signed-off-by: Jason <jasoli@nvidia.com> * Test MagpieTTS release checkpoint restores Signed-off-by: Jason <jasoli@nvidia.com> * Fix v2602 restore without version metadata Signed-off-by: Jason <jasoli@nvidia.com> * revert some code changes Signed-off-by: Jason <jasoli@nvidia.com> * minor Signed-off-by: Jason <jasoli@nvidia.com> * remove unused import Signed-off-by: Jason <jasoli@nvidia.com> * Fix MagpieTTS tokenizer defaults for restore vs fresh training Configs that omit charset_version/punct_version/locale_specific_punct are ambiguous: either a released archive predating those fields (v1 vocabulary) or a config for a new run (current defaults). Forcing either one broke the other — v2602 failed to restore, or new training silently fell back to v1. Tell them apart via the nemo_version ModelPT stamps into saved configs, plus AppState's restore flag, read at the top of __init__ before the nested codec restore clears it. setup_tokenizers' use_legacy_defaults goes back to False. Also dedupes the backfill into persist_versioned_tokenizer_defaults(), which requires moving the charset_version backfill ahead of safe_instantiate() — it previously ran after, so the tokenizer never saw the value. That was harmless while both defaults were 2, but is fatal once the value can be 1. Verified against all three releases: on main, v2602 fails with a text_embedding size mismatch (2362 trained vs 2317 rebuilt, exactly the Hindi v1->v2 charset shrink), while v2512 and v2607 are unaffected. All three restore after the fix. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * Decide legacy tokenizer defaults from config origin, not restore state Being restored does not make a checkpoint legacy, so drop the AppState().is_model_being_restored term (review feedback). It was redundant — every .nemo carries nemo_version — and it was what forced the check to run at the top of __init__, ahead of the nested codec restore that clears the flag. What remains asks only about the config: every NeMo that knows these fields writes them, so a serialized config lacking them must predate them. Renamed is_restored_model_config -> predates_versioned_tokenizer_fields to say that, and moved the call next to setup_tokenizers now that ordering no longer matters. All three releases still restore unchanged. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * Cover the real MagpieTTS call sites and warn on legacy fallback Review follow-up, three fixes. Mutation testing showed the fix itself was untested: inverting or deleting use_legacy_defaults at magpietts.py:442 and easy_magpietts_inference.py:339 reintroduces the v2602 restore failure while the whole TTS unit suite stays green, because the suite drove a test-local ModelPT that re-implemented the wiring rather than exercising it. TestProductionCallSites runs the real constructors, stopped at the tokenizer-setup call so no codec or downloads are needed. Both mutations now fail. The docstring claimed the legacy fallback announces itself with a DeprecationWarning. It does not: IPATokenizer is silent when locale_specific_punct=False drops pt-BR's 11 extra marks, and the Hindi/Arabic warning is swallowed by Python's default filters outside pytest. Make the claim true rather than delete it — persist_versioned_tokenizer_defaults now logs the fields it backfilled, legacy direction only, so ordinary training stays quiet. Also drop the stale 'Must be called inside open_dict(model_cfg)'' from _migrate_versioned_tokenizer_fields, which unlocks each tokenizer node itself now. All three releases still restore unchanged. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * Make the tokenizer version constants the source of truth DEFAULT_CHARSET_VERSION was never read inside tts_tokenizers.py — both tokenizers spelled their default as a literal 2, so the constant only looked authoritative. Add DEFAULT_PUNCT_VERSION, have the signatures read both, and assert the non-legacy backfill equals the class default read off inspect.signature, so a future version bump cannot leave new models pinned a version behind. The validation branches keep their literals: they define the supported set, not the default. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * Date tokenizer defaults from nemo_version instead of config shape A config missing charset_version / punct_version / locale_specific_punct is ambiguous: pre-2.8.0 archives were trained on the v1 values, newer ones on the current defaults. Decide it from the config's nemo_version stamp rather than from whether the config was serialized at all, which mislabelled every modern checkpoint as legacy. - Add VERSIONED_TOKENIZER_FIELDS: one row per field, recording the release its default changed in, next to the DEFAULT_* constants it reads. - setup_tokenizers takes cfg_nemo_version; the resolved values are pinned into the config, so dataloader and worker_init_fn call sites stay version-agnostic and update_config_for_inference needs no migration. - Split CHARSET/PUNCT_VERSIONED_TOKENIZER_TARGETS out of CASELESS_SCRIPT_TOKENIZER_TARGETS: which tokenizers accept an argument is an API fact, not a linguistic one. - Raise an actionable error when a checkpoint's text embedding disagrees with the rebuilt tokenizer, naming the fields to pin. - Describe charset_version=1 / punct_version=1 as frozen for checkpoint compatibility rather than pending removal; v2607 pins charset_version=1. Verified against the released v2512 / v2602 / v2607 configs. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * refine docstring Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * refine tests and codes Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> * removed imported but unused lib. Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> --------- Signed-off-by: Jason <jasoli@nvidia.com> Signed-off-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com> Co-authored-by: Xuesong Yang <1646669+XuesongYang@users.noreply.github.com>
Collaborator
Author
|
/ok to test 673ccf5 |
Signed-off-by: Jason <jasoli@nvidia.com>
Collaborator
Author
|
/ok to test ded346a |
Contributor
|
[🤖]: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.