Repository navigation
refactor(model)!: remove retired model support - #6370
Conversation
Signed-off-by: Chen Cui <chcui@nvidia.com>
cuichenx
added
feature
|
Automatic Claude reviews have been retired. To request a pull-request review, post a comment containing: Add |
Signed-off-by: Chen Cui <chcui@nvidia.com>
| getattr(config, "num_hidden_layers", None), | ||
| getattr(config, "intermediate_size", None), | ||
| ) | ||
| if "mistral-7b" in identifier or "mistral-small-24b" in identifier or mistral_shape in _MISTRAL_SHAPES: |
There was a problem hiding this comment.
I'm a bit worried about these name checks now that they raise instead of warn. They match substrings anywhere in the load path, and a name hit here wins before the shape check, so a supported checkpoint can get rejected just because of a folder name. Tried a few with the real HF configs (transformers 5.17):
- Mistral-Nemo-Base-2407 in
/models/mistral-7b-comparison/nemo->ValueError: Support for Mistral 7B and Mistral Small 3 24B has been removed... - Llama-3.1-8B-Instruct in
/data/nemotron-sft/Llama-3.1-8B-Instruct-> rejected as Llama Nemotron - a Nemotron 3 Nano fine-tune named
my-org/nemotron-nano-30b-a3b-sft-v2-> rejected as Nano v2
supports() / can_handle() also return False for these.
It doesn't really work the other way either. The comment on L45 assumes the serialized _name_or_path survives, but AutoConfig.from_pretrained overwrites it with the local path, so Llama-3.1-Nemotron-Nano-8B-v1 copied to e.g. /ckpts/reasoning-8b goes straight through. Not a big deal since it's plain Llama 3.1 underneath, but the joined identifier isn't buying us much.
Could we only raise on what's actually in the config (archs + shape signatures) and keep the name matching as a warning, ideally against the repo id / basename rather than the whole path? A couple of tests with supported models under misleading paths would be nice too.
There was a problem hiding this comment.
i think the right thing to do is remove deprecation.py since these models are already deleted, what do you think
There was a problem hiding this comment.
Yeah agreed, deleting it is cleaner than trying to fix the heuristics, and the new tests cover the cases I was worried about. Thanks!
Two small optional things:
- Removed archs now hit the generic "Model architecture 'Gemma2ForCausalLM' is not yet supported ... create a new bridge" error (auto_bridge.py L2486), which kind of reads like support is coming. Maybe a small map of removed arch names just for the message, e.g. "removed in 0.7.0, use 0.6.x"? Exact names only, no path matching.
- Dense Nano v2 VL:
can_handle()still returns True since the V2 label is registered for the Omni alias, thento_megatron_provider()fails with "Only Nemotron Omni MoE checkpoints use this compatibility bridge." Could that message just say dense Nano v2 VL was removed?
There was a problem hiding this comment.
thanks these are good points
Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>
Report removal in 0.7.0 and direct users to 0.6.x for exact retired architecture names after bridge lookup fails. Keep shared successor dispatch and legacy Omni MoE compatibility unchanged, and identify dense Nano v2 VL explicitly in its rejection message. Validation: 200 targeted unit tests passed; pre-commit --all-files passed; independent follow-up diff review found no issues. Signed-off-by: Chen Cui <chcui@nvidia.com>
3a8db15
The outer 30-second subprocess timeout includes Bridge model registration imports. A fresh local run spent 33.66 seconds importing and completed forked tokenization by 33.78 seconds. Use the existing 180-second import-test budget while preserving the 10-second result.get timeout that detects tokenizer deadlocks. Validation: all 41 GPT SFT dataset tests passed with the pinned MCore dataset helper built in a temporary container copy; pre-commit --all-files passed; independent diff review found no issues. Signed-off-by: Chen Cui <chcui@nvidia.com>
|
@cuichenx this PR deleted the retired-model sources, but |
Resolve the conflict in recipes/nemotronh/h100/__init__.py: keep main's removal of the retired Nemotron Nano v2 and NemotronH entries (#6370) and this branch's rename of the Ultra SFT recipe to nemotron_3_ultra_sft_384gpu_h100_bf16_openmathinstruct2_packed_config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kamran Jafari <kjafarisadeg@nvidia.com>
This branch was successfully deployed
4 participants
What does this PR do ?
Remove retired model implementations, registrations, recipes, examples, and current support documentation while preserving shared infrastructure used by supported successors. Delete the deprecation module and its path/name/shape blacklist, so compatible checkpoints are not rejected because of a directory name or fine-tune suffix.
Primary area:
area:model. This is a breaking change for the removed model APIs and recipes.Changelog
models/_deprecation.pyand every caller. Add regressions for supported checkpoints under misleading local directory names and a Nemotron 3 fine-tune with av2suffix.GitHub Actions CI
Local validation uses the existing NeMo development container, repository-pinned MCore, and isolated lockfile versions of Transformers 5.17.0, tokenizers 0.23.2, and huggingface-hub 1.31.0. No dependency or CI workflow changes.
uv run pre-commit run --all-filesandgit diff --check: passed.The Fern preview for the updated head passed. Other CI checks are not yet verified complete.
Before your PR is "Ready for review"
Pre checks:
Additional Information
Independent complete-diff review and follow-up review found no outstanding issues. The follow-up verified that all 77 retained definitions in the modified mixed performance modules are unchanged and that no stale imports of deleted modules remain.
The repository-wide inventory found no target-only verification cards, goldens, or active functional launchers, and no existing Mixtral implementation/registration to remove or change. No cluster jobs were launched.