Add a general composable $import system for YAML configs, and use it to implement composable recipes#1253
Open
shengliangxu wants to merge 32 commits intomainfrom
Open
Add a general composable $import system for YAML configs, and use it to implement composable recipes#1253shengliangxu wants to merge 32 commits intomainfrom
shengliangxu wants to merge 32 commits intomainfrom
Conversation
|
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a composable YAML imports system ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Loader as modelopt.recipe.loader
participant Raw as _load_raw_config
participant Resolver as _resolve_imports
participant FS as Files / Builtins
User->>Loader: load_recipe(path)
Loader->>Raw: _load_raw_config(path)
Raw->>FS: read YAML (file or builtin)
FS-->>Raw: raw documents
Raw-->>Loader: parsed raw config (may include imports)
alt imports present
Loader->>Resolver: _resolve_imports(parsed_config)
Resolver->>Raw: _load_raw_config(import_path)
Raw->>FS: read imported snippet
FS-->>Raw: snippet content
Raw-->>Resolver: snippet (dict/list / _list_content)
Resolver->>Resolver: merge / splice / detect circular refs
Resolver-->>Loader: resolved config
end
Loader-->>User: final resolved recipe
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Commit description:
Introduce an import mechanism that lets recipe YAML files reference reusable
config snippets by name, reducing duplication across recipes.
Syntax:
imports:
fp8: configs/numerics/fp8
base_disable_all: configs/ptq/base_disable_all
quant_cfg:
- base_disable_all # string entry → replaced with imported dict or spliced
list
- quantizer_name: '*weight_quantizer'
cfg: fp8 # string cfg → replaced with imported dict
Features:
- Dict-based imports (keys are names, values are config paths) — no name conflicts
- Three resolution modes: string cfg value, string list entry (dict), string list entry (list
splice)
- Recursive resolution with circular import detection
- Path resolution via load_config (built-in library first, then filesystem)
- Works with both single-file and directory recipe formats
New reusable config snippets (modelopt_recipes/configs/):
- numerics/fp8.yml, nvfp4_dynamic.yml, nvfp4_static.yml
- ptq/base_disable_all.yaml, default_disabled_quantizers.yaml
All 6 built-in PTQ recipes converted to use imports, reducing each by ~30 lines.
Pre-commit hook updated to skip configs/ directory and allow string entries in
quant_cfg. load_config() now accepts YAML lists for list-valued snippets.
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
shengliangxu
force-pushed
the
shengliangx/composable-recipes
branch
from
ea910ae to
82d5a12
Compare
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
shengliangxu
changed the title
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
shengliangxu
requested review from
Edwardf0t1,
cjluo-nv,
h-guo18,
kevalmorabia97,
meenchen and
realAsma
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelopt/recipe/loader.py (1)
126-165:⚠️ Potential issue | 🟠 MajorResolve directory recipes from one import pass.
_load_recipe_from_dir()still readsmetadatafrom unresolvedrecipe.yml, and it resolvesquantize.ymlbefore recipe-level aliases are available. That breaks valid directory recipes that use$importinsidemetadata, and it also fails whenquantize.ymlhas its ownimportsplus references names declared inrecipe.yml. Please build the combined directory recipe first, then resolve imports once before readingmetadata/quantize.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/recipe/loader.py` around lines 126 - 165, The loader currently reads metadata from the unresolved recipe and resolves quantize.yml before recipe-level imports, breaking imports that span both files; change _load_recipe_from_dir so it first loads raw recipe_data via _load_raw_config and—if recipe_type == RecipeType.PTQ—loads quantize_data via _load_raw_config, then build a single combined dict that contains the recipe fields plus a "quantize" key set to quantize_data and a merged "imports" entry (include both recipe_data.get("imports") and quantize_data.get("imports") as appropriate), call _resolve_imports once on that combined dict, and only then read metadata and construct ModelOptPTQRecipe (i.e., use resolved combined["quantize"] and resolved metadata) so that _resolve_imports is applied consistently for _load_recipe_from_dir, _resolve_imports, RecipeType.PTQ, and ModelOptPTQRecipe.
🧹 Nitpick comments (1)
modelopt/torch/opt/config_loader.py (1)
177-187: Cycle detection should use canonical resolved identities, not raw import strings.At Line 180-Line 187,
_loadingtracks the rawconfig_pathvalue. Equivalent aliases (e.g.,a,a.yaml,./a.yaml) can bypass the circular guard and recurse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/opt/config_loader.py` around lines 177 - 187, The cycle detection currently compares raw config_path strings in _loading, which allows equivalent paths (aliases like "a", "a.yaml", "./a.yaml") to bypass the guard; update _resolve_imports (and the loop over imports_dict) to canonicalize/resolve each config_path before checking or inserting into _loading (use the same resolution used by _load_raw_config or a new _canonicalize/resolve function), so replace checks against raw config_path with checks against the canonical identity and pass the canonical identity into recursive calls and into the _loading set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/guides/10_recipes.rst`:
- Around line 598-606: Update the repository tree snippet so the directory paths
match the rest of the guide: change the entries under "configs/ptq/" to
"configs/ptq/units/" (or alternatively adjust examples elsewhere to remove
"units/"); specifically ensure the listed folders (e.g., base_disable_all.yaml,
default_disabled_quantizers.yaml) appear under configs/ptq/units/ in the tree
snippet so it matches the examples and table references throughout the guide.
In `@modelopt_recipes/configs/numerics/nvfp4.yaml`:
- Around line 18-21: The YAML has mistaken nesting: `type` and `scale_bits` are
indented under `block_sizes` and must be sibling keys per the NVFP4 quantizer
schema; fix by dedenting `type` and `scale_bits` so `block_sizes:`, `type:`, and
`scale_bits:` are all at the same indentation level (ensure `block_sizes`
retains its mapping with `-1: 16` and `type: dynamic` and `scale_bits: e4m3` are
siblings).
In `@modelopt_recipes/configs/ptq/presets/README.md`:
- Around line 7-10: Update the README.md text to distinguish full model presets
from partial KV-only fragments: change the paragraph that claims "Each preset is
a complete, self-contained config..." to explain there are two kinds — full
presets (e.g., configs/ptq/presets/model/*) that include an algorithm and
quant_cfg and are ready to pass to mtq.quantize(), and merge-only KV fragments
(e.g., configs/ptq/presets/kv/fp8.yaml) that lack an algorithm and must be
imported/merged into a full preset; reference the $import system and the two
directories (model/ vs kv/) and note that KV files are partial fragments not
suitable alone for mtq.quantize().
In `@modelopt/torch/opt/config_loader.py`:
- Around line 250-260: The current logic only resolves list-splice imports for
keys named "quant_cfg" and "_list_content", leaving other list locations
unresolved; modify the code that currently iterates over containers and calls
_resolve_list to instead recursively walk the full config tree (visiting all
dicts and lists within the top-level data) and call _resolve_list for any list
value that may contain $import markers (not only "quant_cfg" or
"_list_content"); reuse the existing _resolve_list helper and ensure you update
in-place entries discovered during the traversal so other keys/paths besides
quant_cfg are correctly resolved.
- Around line 72-91: The code currently appends local filesystem paths to
paths_to_check before BUILTIN_CONFIG_ROOT, causing local files to shadow
built-ins; update the ordering so BUILTIN_CONFIG_ROOT candidates are inserted
before local filesystem candidates. Specifically: in the str branch, when
config_file lacks .yml/.yaml, append
BUILTIN_CONFIG_ROOT.joinpath(f"{config_file}.yml") and
BUILTIN_CONFIG_ROOT.joinpath(f"{config_file}.yaml") before
Path(f"{config_file}.yml") and Path(f"{config_file}.yaml"); when config_file
already endswith .yml/.yaml, append BUILTIN_CONFIG_ROOT.joinpath(config_file)
before Path(config_file). In the Path branch, when config_file.suffix is
.yml/.yaml and not config_file.is_absolute(), prepend
BUILTIN_CONFIG_ROOT.joinpath(str(config_file)) ahead of config_file; when no
suffix and not absolute, append
BUILTIN_CONFIG_ROOT.joinpath(f"{config_file}.yml") and
BUILTIN_CONFIG_ROOT.joinpath(f"{config_file}.yaml") before the corresponding
Path(f"{config_file}.yml") and Path(f"{config_file}.yaml"). Ensure references to
config_file, paths_to_check, and BUILTIN_CONFIG_ROOT are used to locate these
changes.
In `@tools/precommit/check_modelopt_recipes.py`:
- Around line 58-60: The conditional that skips entries with "$import" should
first validate the payload shape: in the block where you check `if "$import" in
entry:` (in tools/precommit/check_modelopt_recipes.py), verify that
`entry["$import"]` is either a str or a list of str (all elements are str)
before doing `continue`; if the value is not the expected shape, do not skip
validation—raise or report a structural error instead so malformed payloads like
`{"$import": 123}` or `{"$import": ["ok", 1]}` are caught.
---
Outside diff comments:
In `@modelopt/recipe/loader.py`:
- Around line 126-165: The loader currently reads metadata from the unresolved
recipe and resolves quantize.yml before recipe-level imports, breaking imports
that span both files; change _load_recipe_from_dir so it first loads raw
recipe_data via _load_raw_config and—if recipe_type == RecipeType.PTQ—loads
quantize_data via _load_raw_config, then build a single combined dict that
contains the recipe fields plus a "quantize" key set to quantize_data and a
merged "imports" entry (include both recipe_data.get("imports") and
quantize_data.get("imports") as appropriate), call _resolve_imports once on that
combined dict, and only then read metadata and construct ModelOptPTQRecipe
(i.e., use resolved combined["quantize"] and resolved metadata) so that
_resolve_imports is applied consistently for _load_recipe_from_dir,
_resolve_imports, RecipeType.PTQ, and ModelOptPTQRecipe.
---
Nitpick comments:
In `@modelopt/torch/opt/config_loader.py`:
- Around line 177-187: The cycle detection currently compares raw config_path
strings in _loading, which allows equivalent paths (aliases like "a", "a.yaml",
"./a.yaml") to bypass the guard; update _resolve_imports (and the loop over
imports_dict) to canonicalize/resolve each config_path before checking or
inserting into _loading (use the same resolution used by _load_raw_config or a
new _canonicalize/resolve function), so replace checks against raw config_path
with checks against the canonical identity and pass the canonical identity into
recursive calls and into the _loading set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b6ed87d-c72b-42a3-b9b3-6b7a312c52aa
📒 Files selected for processing (27)
.pre-commit-config.yamlCHANGELOG.rstdocs/source/guides/10_recipes.rstmodelopt/recipe/_config_loader.pymodelopt/recipe/loader.pymodelopt/torch/opt/config_loader.pymodelopt/torch/quantization/config.pymodelopt_recipes/configs/numerics/fp8.yamlmodelopt_recipes/configs/numerics/nvfp4.yamlmodelopt_recipes/configs/numerics/nvfp4_static.yamlmodelopt_recipes/configs/ptq/presets/README.mdmodelopt_recipes/configs/ptq/presets/kv/fp8.yamlmodelopt_recipes/configs/ptq/presets/model/fp8.yamlmodelopt_recipes/configs/ptq/units/README.mdmodelopt_recipes/configs/ptq/units/base_disable_all.yamlmodelopt_recipes/configs/ptq/units/default_disabled_quantizers.yamlmodelopt_recipes/configs/ptq/units/fp8_kv.yamlmodelopt_recipes/configs/ptq/units/w4a4_nvfp4_nvfp4.yamlmodelopt_recipes/configs/ptq/units/w8a8_fp8_fp8.yamlmodelopt_recipes/general/ptq/fp8_default-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_default-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_default-none_kv_gptq.yamlmodelopt_recipes/general/ptq/nvfp4_experts_only-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_mlp_only-fp8_kv.yamlmodelopt_recipes/general/ptq/nvfp4_omlp_only-fp8_kv.yamltests/unit/recipe/test_loader.pytools/precommit/check_modelopt_recipes.py
shengliangxu
changed the title
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modelopt/torch/opt/config_loader.py (2)
179-189: Potential false negative in circular import detection with path variants.The circular import check compares raw
config_pathstrings. If the same file is referenced via different path representations (e.g.,"configs/foo"vs"./configs/foo"or absolute vs relative paths), the check may not detect the cycle.Consider normalizing paths before adding to
_loading:🔧 Suggested improvement
for name, config_path in imports_dict.items(): if not config_path: raise ValueError(f"Import {name!r} has an empty config path.") + # Normalize for consistent cycle detection + norm_path = str(config_path) - if config_path in _loading: + if norm_path in _loading: raise ValueError( f"Circular import detected: {config_path!r} is already being loaded. " f"Import chain: {sorted(_loading)}" ) snippet = _load_raw_config(config_path) if isinstance(snippet, dict) and "imports" in snippet: - snippet = _resolve_imports(snippet, _loading | {config_path}) + snippet = _resolve_imports(snippet, _loading | {norm_path})For more robust detection, consider resolving to absolute paths or using the resolved
config_pathfrom_load_raw_config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/opt/config_loader.py` around lines 179 - 189, The circular-import detection is comparing raw config_path strings which can differ in representation; update _resolve_imports to normalize/resolve each config path (e.g., via pathlib.Path(...).resolve()) before checking membership in or adding to _loading and before calling _load_raw_config, and use the resolved path consistently for the error message and when recursing (referencing _resolve_imports, _load_raw_config, _loading, imports_dict, and snippet) so equivalent paths (relative vs absolute or with ./) are treated identically.
252-262: Note:$importlist-splice resolution is scoped to specific keys.The resolution only processes
quant_cfgand_list_contentlists, which matches current recipe usage. However, if future configs need$importin other list-valued fields, this will require extending the resolution logic. Consider documenting this limitation in the docstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/opt/config_loader.py` around lines 252 - 262, The current list-splice resolution only runs _resolve_list for quant_cfg (either at top-level or under the quantize key) and for _list_content, so other list-valued fields won't have $import resolution; update the docstring of the surrounding function (referencing data, container, _resolve_list, quant_cfg, and _list_content) to state this scoping limitation and that any future list fields requiring $import must be added to the resolution logic (or call _resolve_list explicitly), so reviewers and future maintainers know to extend the loop when adding new list keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modelopt/torch/opt/config_loader.py`:
- Around line 179-189: The circular-import detection is comparing raw
config_path strings which can differ in representation; update _resolve_imports
to normalize/resolve each config path (e.g., via pathlib.Path(...).resolve())
before checking membership in or adding to _loading and before calling
_load_raw_config, and use the resolved path consistently for the error message
and when recursing (referencing _resolve_imports, _load_raw_config, _loading,
imports_dict, and snippet) so equivalent paths (relative vs absolute or with ./)
are treated identically.
- Around line 252-262: The current list-splice resolution only runs
_resolve_list for quant_cfg (either at top-level or under the quantize key) and
for _list_content, so other list-valued fields won't have $import resolution;
update the docstring of the surrounding function (referencing data, container,
_resolve_list, quant_cfg, and _list_content) to state this scoping limitation
and that any future list fields requiring $import must be added to the
resolution logic (or call _resolve_list explicitly), so reviewers and future
maintainers know to extend the loop when adding new list keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 073f02cb-4603-407c-92af-400f4092f280
📒 Files selected for processing (4)
docs/source/guides/10_recipes.rstmodelopt/torch/opt/config_loader.pymodelopt/torch/quantization/config.pytools/precommit/check_modelopt_recipes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/source/guides/10_recipes.rst
- modelopt/torch/quantization/config.py
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modelopt/torch/opt/config_loader.py (1)
109-110: Consider defensive limits for file size/import depth/expanded node count.Current loading/import expansion is unbounded. A deeply nested or very large YAML import graph can still cause resource exhaustion even with safe parsing and cycle checks.
I’d recommend adding configurable caps (e.g., max file bytes, max recursive depth, max expanded elements).
As per coding guidelines: "Treat configs/snippets as untrusted inputs: validate shapes/types and enforce limits to avoid crashes or resource exhaustion from malformed/large YAML."
Also applies to: 156-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/torch/opt/config_loader.py` around lines 109 - 110, Before parsing untrusted YAML, enforce configurable defensive limits: check the byte size of config_path.read_text() against MAX_FILE_BYTES (reject if exceeded) before assigning text, then when calling yaml.safe_load_all(text) stop after a configurable MAX_DOCS and raise a clear error if more documents are present; additionally, add a max_import_depth counter to the import/expansion logic (the code that iterates over docs and resolves imports around the later import-expansion block) to prevent unbounded recursion and track a MAX_EXPANDED_NODES counter for total expanded elements/nodes (increment as you expand/import and raise if exceeded). Make all limits configurable via parameters or environment, surface explicit errors, and document which constants control yaml.safe_load_all, text, docs, and the import/expansion logic so callers can tune them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelopt/torch/opt/config_loader.py`:
- Around line 214-221: Validate the types of the $import reference(s) before
calling _lookup: ensure ref is either a string or a list of strings, and if it's
a list verify every element is a string (no mixed or non-string items); if
validation fails, raise a clear ValueError describing the bad $import value.
Update the logic around _IMPORT_KEY/ref/ref_names and the loop that calls
_lookup (the block using ref_names, snippet, and merged) as well as the same
pattern in the later resolution path (lines handling list resolution around
235-243) to perform these checks prior to any call to _lookup.
- Around line 182-189: The cycle check should use a canonical identity for the
config source instead of the raw config_path string: compute a
normalized/canonical source key (e.g., resolve to an absolute Path via
Path(config_path).resolve() or a canonical resource key) before checking
`_loading`, use that canonical key in the membership test and error message,
call `_load_raw_config` with the resolved/canonical path (or map the original to
the canonical key), and pass the canonical key into `_resolve_imports` by adding
it to `_loading` (replace uses of `config_path` with the canonical key in the
circular-detection and recursive `_resolve_imports` call).
---
Nitpick comments:
In `@modelopt/torch/opt/config_loader.py`:
- Around line 109-110: Before parsing untrusted YAML, enforce configurable
defensive limits: check the byte size of config_path.read_text() against
MAX_FILE_BYTES (reject if exceeded) before assigning text, then when calling
yaml.safe_load_all(text) stop after a configurable MAX_DOCS and raise a clear
error if more documents are present; additionally, add a max_import_depth
counter to the import/expansion logic (the code that iterates over docs and
resolves imports around the later import-expansion block) to prevent unbounded
recursion and track a MAX_EXPANDED_NODES counter for total expanded
elements/nodes (increment as you expand/import and raise if exceeded). Make all
limits configurable via parameters or environment, surface explicit errors, and
document which constants control yaml.safe_load_all, text, docs, and the
import/expansion logic so callers can tune them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 202821f4-c1b8-43d1-bfc9-013e8c78d2b0
📒 Files selected for processing (2)
modelopt/torch/opt/config_loader.pytests/unit/recipe/test_loader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/recipe/test_loader.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
- Coverage 75.58% 72.56% -3.03%
==========================================
Files 459 460 +1
Lines 48613 48697 +84
==========================================
- Hits 36745 35336 -1409
- Misses 11868 13361 +1493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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.
What does this PR do?
Type of change: New feature
Add a composable
$importsystem for recipe YAML configs, plus reorganize config snippets and begin migrating hardcoded*_CFGdicts to YAML presets.Problem
*_CFGdicts inconfig.py(e.g.,FP8_DEFAULT_CFG) had no connection to the YAML recipe systemmodelopt.recipe, causing circular imports whenmodelopt.torch.quantization.configneeded to load YAML configsSolution
Composable
$importsystemRecipes declare an
importssection mapping names to config snippet files. The{$import: name}marker is explicitly resolved at load time. For example, a recipe can import shared FP8 attributes and standard exclusions rather than duplicating them inline.$importsemantics:$import: [a, b]) and inline override keys with ordered precedence (later imports override earlier, inline keys override all)---separator (first doc = imports, second doc = list content)Config snippet library (
modelopt_recipes/configs/):numerics/— all numeric format definitions live here (fp8, nvfp4, nvfp4_static). Any new numeric format (e.g., int8, mxfp8) should be added to this directory as the single source of truth for quantizer attributesptq/units/— reusable quant_cfg entries (base_disable_all, default_disabled, fp8_kv, w8a8_fp8_fp8, w4a4_nvfp4_nvfp4)ptq/presets/model/— complete configs replacing hardcoded*_CFGdictsptq/presets/kv/— KV cache config presetsHardcoded config migration:
FP8_DEFAULT_CFGandFP8_KV_CFGnow load from YAML presets viaload_config()modelopt.recipe._config_loadertomodelopt.torch.opt.config_loaderto eliminate circular importsAll 6 built-in PTQ recipes converted to use imports — each reduced by ~30 lines.
Changes by area
Config loading infrastructure:
modelopt/torch/opt/config_loader.py— new home for YAML loading +$importresolution (zeromodeloptimports, no circular dependency risk)modelopt/recipe/_config_loader.py— thin re-export fromtorch.opt.config_loadermodelopt/recipe/loader.py— uses shared_resolve_importsfrom config_loaderQuantization config:
modelopt/torch/quantization/config.py—FP8_DEFAULT_CFGandFP8_KV_CFGloaded from YAML presetsRecipe YAML files:
$importstylePre-commit:
.pre-commit-config.yaml— recipe validator excludesconfigs/directorytools/precommit/check_modelopt_recipes.py— recognizes$importentriesDocumentation:
docs/source/guides/10_recipes.rst— full$importspecification with examples, inline/import style comparison, multi-document snippets, override precedenceTests:
tests/unit/recipe/test_loader.py— 20+ new tests covering all import featuresTesting
All new and existing recipe loader tests pass. Built-in recipe smoke tests pass with converted recipes.
Before your PR is "Ready for review"
FP8_DEFAULT_CFG/FP8_KV_CFGproduce identical dicts)Summary by CodeRabbit
New Features
Documentation
Tests
Chores