Rename modelopt_recipes/huggingface to model_type with backward-compat alias - #2328
Draft
shengliangxu wants to merge 3 commits into
Draft
Rename modelopt_recipes/huggingface to model_type with backward-compat alias#2328shengliangxu wants to merge 3 commits into
shengliangxu wants to merge 3 commits into
Conversation
…t alias Rename the architecture-specific recipe tier from modelopt_recipes/huggingface/ to modelopt_recipes/model_type/ to make clear it holds recipes shared across every checkpoint of a Hugging Face model_type. The old huggingface/ path is retained only as a deprecated backward-compatibility alias. - Loader: generalize the recipe-path alias in modelopt/recipe/loader.py so saved --recipe huggingface/<model_type>/... paths rewrite to model_type/..., next to the existing huggingface/models/... -> models/... rewrite (checked first as the more specific prefix). This keeps old paths working for pip-installed wheels, where the source-tree symlinks don't survive. - Recipes: rewrite internal $import references under model_type/ from huggingface/... to model_type/... so recipes load without the symlink (required for wheels). - Packaging: extend the exclude-package-data globs and MANIFEST.in prunes to cover the huggingface -> model_type and model_type/models -> ../models symlinks so each recipe ships exactly once. - Docs/examples/skills/tests: migrate all internal references to the canonical model_type/ path; huggingface/ remains only in the backward-compat alias tests and explanatory notes. - Add a Deprecations changelog entry and a test covering the huggingface/<model_type>/ -> model_type/ alias. Verified: tests/unit/recipe passes (336); built sdist and wheel ship each recipe once with no huggingface/ or model_type/models/ duplicates; a simulated wheel install (no symlinks) resolves huggingface/..., model_type/..., and huggingface/models/... via the loader alias. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
transformers>=5.9 dropped base_model_prefix from WeightTransform's __slots__ (scoped matching now keys off scope_prefix alone), so the scoped-rule tests in tests/unit/torch/export/test_quant_aware_conversion.py raised AttributeError when assigning transform.base_model_prefix on the now-slotted object. Production _scope_prefixes already reads the attribute via getattr(..., None) and degrades correctly when it is absent (the base-prefixed candidate collapses to the scope_prefix-only one), so there is no runtime behavior change. Set base_model_prefix through a helper that suppresses AttributeError so the tests run across the whole supported transformers range (>=4.57,<5.15), and clarify the version dependence in the _scope_prefixes docstring. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Contributor
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2328 +/- ##
=======================================
Coverage 79.28% 79.29%
=======================================
Files 527 527
Lines 61482 61491 +9
=======================================
+ Hits 48748 48758 +10
+ Misses 12734 12733 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ocal trees Address PR review: the huggingface/ -> model_type/ backward-compat alias only lived in load_recipe(), leaving two gaps. - $import resolution goes through config_loader._resolve_config_path, not the recipe-path alias, so a custom recipe importing a shipped snippet by its old huggingface/... path would fail from a wheel (where the symlink is gone). Move the prefix rewrite into a shared _alias_builtin_recipe_prefix helper in config_loader and apply it to the built-in $import candidates too. - The loader rewrote huggingface/ for the filesystem fallback as well, so a user's own local huggingface/... recipe tree could no longer load by its natural relative name. Built-in candidates now use the alias; the filesystem fallback probes the path exactly as given first, then the aliased form. Also alias model_type/models/ -> models/ (the model_type/models symlink is source-only and pruned from wheels) so that path resolves identically from a checkout and an installed wheel. Adds tests for the $import alias, the local-huggingface no-shadow case, and the model_type/models alias; verified in a symlink-free extracted-wheel tree. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
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
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.
What does this PR do?
Type of change: Refactor + deprecation (recipe-library restructure, backward compatible), plus an unrelated transformers-compat test fix.
Rename the architecture-specific recipe tier
modelopt_recipes/huggingface/tomodelopt_recipes/model_type/, making explicit that it holds recipes shared acrossevery checkpoint of a Hugging Face
model_type— as opposed to the checkpoint-mirrormodels/<org>/<model_id>/tier. The oldhuggingface/path keeps working as adeprecated backward-compat alias (a source-tree symlink plus a loader alias), so no
saved
--recipepath breaks.modelopt/recipe/loader.py): generalized so saved--recipe huggingface/<model_type>/...paths rewrite tomodel_type/..., alongsidethe existing
huggingface/models/... -> models/...rewrite (checked first as the morespecific prefix). This keeps old paths resolving for pip-installed wheels, where the
source-tree symlinks don't survive.
$imports: rewritten fromhuggingface/... -> model_type/...inside theshipped recipes so they resolve without the symlink — mandatory for wheels, since
$importresolution goes throughconfig_loader(no alias there).pyproject.toml,MANIFEST.in): extended the symlink-exclusion globsso the recursive
**/*.yamlpackage-data glob doesn't double-ship recipes through thehuggingface -> model_typeandmodel_type/models -> ../modelssymlinks.model_type/;huggingface/remains only in the deprecated-alias tests and explanatorynotes.
tests/unit/torch/export/test_quant_aware_conversion.pyfailed on transformers>=5.9, which dropped
base_model_prefixfromWeightTransform.__slots__(the scoped-rule tests assigned it on the now-slottedobject). Production
_scope_prefixesalready reads it viagetattr(..., None)anddegrades correctly, so there is no runtime change — the tests now set it through a
helper that suppresses
AttributeErroracross the supported transformers range.Usage
Testing
tests/unit/recipe/— 336 passed, including the newtest_load_recipe_huggingface_arch_backward_compat_aliasand the updatedstructural/doc tests (
test_recipe_docs.py).tests/unit/torch/export/test_quant_aware_conversion.py— 16 passed (was 4 failedon transformers 5.9.0).
once (27
model_type/, 11models/, 153 total) with zerohuggingface/ormodel_type/models/duplicates and no build error on the symlinks.huggingface/<arch>/...,model_type/..., andhuggingface/models/...all resolve viathe loader alias — including a recipe that pulls internal
$imports.Before your PR is "Ready for review"
huggingface/...recipe paths keep resolving via the symlink + loader alias.CONTRIBUTING.md: N/AAdditional Information
The
model_type/models -> ../modelssymlink is kept purely as a backward-compat alias forold
huggingface/models/<org>/<model_id>/...paths;model_type/is otherwisearchitecture-only. If we ever want it strictly architecture-only, that symlink can be
dropped later without breaking anything, since the loader rewrites
huggingface/models/...straight to the top-level
models/tier.