Preserve HF PTQ checkpoint sidecar files [NV BUG 6491822] - #2060
Preserve HF PTQ checkpoint sidecar files [NV BUG 6491822]#2060jenchen13 wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesCheckpoint sidecar preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant _resolve_model_path
participant snapshot_download
participant copy_custom_model_files
participant copy_non_safetensor_files_from_ckpt
_resolve_model_path->>snapshot_download: request sidecar allowlist
snapshot_download-->>_resolve_model_path: return snapshot path
copy_custom_model_files->>copy_non_safetensor_files_from_ckpt: copy eligible sidecars with exclusions
copy_non_safetensor_files_from_ckpt-->>copy_custom_model_files: return copied filenames
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/hf_ptq/example_utils.py`:
- Around line 64-77: Add "*.gguf" to _CHECKPOINT_WEIGHT_FILE_PATTERNS so
snapshot_download() and copy_custom_model_files() exclude GGUF weights, then
extend tests/examples/hf_ptq/test_example_utils.py with a model.gguf fixture and
assert it is absent from the quantized export using pytest.
- Around line 965-970: Update _should_copy_checkpoint_sidecar and the associated
copy flow to validate symlink targets before shutil.copy2: resolve the target
and reject links escaping the approved checkpoint roots, while permitting
Hugging Face cache snapshot-to-blob symlinks. Preserve existing exclusions for
export-owned files and checkpoint weight/index files, and add a regression test
covering an out-of-tree sidecar symlink.
🪄 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: Enterprise
Run ID: e8c801f1-d39e-4ff7-8709-1bfc89199303
📒 Files selected for processing (3)
examples/hf_ptq/example_utils.pyexamples/hf_ptq/hf_ptq.pytests/examples/hf_ptq/test_example_utils.py
| def _should_copy_checkpoint_sidecar(file_path: Path) -> bool: | ||
| if not file_path.is_file(): | ||
| return False | ||
| if file_path.name in _EXPORT_OWNED_CHECKPOINT_FILES: | ||
| return False | ||
| return not _is_checkpoint_weight_or_index_file(file_path.name) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
mkdir -p "$tmp_dir/source" "$tmp_dir/export"
printf 'sensitive-data\n' > "$tmp_dir/secret.txt"
ln -s "$tmp_dir/secret.txt" "$tmp_dir/source/tokenizer.json"
SOURCE_DIR="$tmp_dir/source" EXPORT_DIR="$tmp_dir/export" python - <<'PY'
import os
import shutil
from pathlib import Path
source = Path(os.environ["SOURCE_DIR"]) / "tokenizer.json"
destination = Path(os.environ["EXPORT_DIR"]) / "tokenizer.json"
assert source.is_file(), "Path.is_file() follows the symlink"
shutil.copy2(source, destination)
assert destination.read_text() == "sensitive-data\n"
print("Symlink dereference reproduced.")
PY
# Inspect the changed copier and its tests before defining allowed symlink targets.
rg -n -C 4 '_should_copy_checkpoint_sidecar|copy2|is_file|copy_custom_model_files' \
examples/hf_ptq/example_utils.py tests/examples/hf_ptq/test_example_utils.pyRepository: NVIDIA/Model-Optimizer
Length of output: 3991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- helper and copier ---'
sed -n '900,1030p' examples/hf_ptq/example_utils.py
echo '--- relevant tests ---'
sed -n '1,180p' tests/examples/hf_ptq/test_example_utils.py
echo '--- symlink and cache references ---'
rg -n -C 3 'symlink|resolve\(|snapshot|cache|copy_custom_model_files|checkpoint sidecar' \
examples tests SECURITY.md 2>/dev/null || trueRepository: NVIDIA/Model-Optimizer
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-Gb9q3G
echo '--- captured source section ---'
line=$(rg -n '^examples/hf_ptq/example_utils.py:965:' "$log" | head -1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-70)); end=$((line+75))
sed -n "${start},${end}p" "$log"
fi
echo '--- captured test section ---'
line=$(rg -n '^tests/examples/hf_ptq/test_example_utils.py:52:' "$log" | head -1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-5)); end=$((line+75))
sed -n "${start},${end}p" "$log"
fi
echo '--- focused repository search ---'
rg -n -C 3 'HF_HUB_CACHE|snapshot_download|local_files_only|is_symlink|readlink|resolve\(\)' \
examples/hf_ptq tests/examples/hf_ptq SECURITY.md || trueRepository: NVIDIA/Model-Optimizer
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- source ---'
sed -n '940,1030p' examples/hf_ptq/example_utils.py
echo '--- tests ---'
sed -n '1,125p' tests/examples/hf_ptq/test_example_utils.py
echo '--- focused references ---'
rg -n -C 3 'HF_HUB_CACHE|snapshot_download|local_files_only|is_symlink|readlink|resolve\(\)' \
examples/hf_ptq tests/examples/hf_ptq SECURITY.md || trueRepository: NVIDIA/Model-Optimizer
Length of output: 13379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- model-path resolution and cache roots ---'
sed -n '830,945p' examples/hf_ptq/example_utils.py
echo '--- security guidance ---'
rg -n -C 4 'checkpoint|sidecar|artifact|symlink|untrusted|deserialization' SECURITY.md
echo '--- call sites and export-path inputs ---'
rg -n -C 4 'copy_custom_model_files\(' examples/hf_ptq tests/examples/hf_ptqRepository: NVIDIA/Model-Optimizer
Length of output: 11606
Validate checkpoint sidecar symlink targets before copying.
Path.is_file() and shutil.copy2() follow symlinks. A local untrusted checkpoint can link a sidecar to any readable file outside the checkpoint and copy it into export_path. Reject targets outside approved checkpoint roots, while allowing Hugging Face cache snapshot-to-blob symlinks. Add an out-of-tree symlink regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/hf_ptq/example_utils.py` around lines 965 - 970, Update
_should_copy_checkpoint_sidecar and the associated copy flow to validate symlink
targets before shutil.copy2: resolve the target and reject links escaping the
approved checkpoint roots, while permitting Hugging Face cache snapshot-to-blob
symlinks. Preserve existing exclusions for export-owned files and checkpoint
weight/index files, and add a regression test covering an out-of-tree sidecar
symlink.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2060 +/- ##
===========================================
+ Coverage 67.15% 78.04% +10.88%
===========================================
Files 521 521
Lines 59857 59872 +15
===========================================
+ Hits 40199 46727 +6528
+ Misses 19658 13145 -6513
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:
|
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
The direction (blanket-copy non-weight sidecars instead of a brittle whitelist) is right and the new test pins the local-dir happy path. A few things to address before merge:
-
Duplicated logic — the repo already has exactly this helper:
modelopt/torch/export/plugins/hf_checkpoint_utils.py::copy_non_safetensor_files_from_ckpt(that is the Megatron-Core behavior the PR body cites, and its docstring already documents the "modelopt owns config.json / generation_config.json / hf_quant_config.json / preprocessor_config.json" convention).examples/hf_ptqalready imports freely frommodelopt.torch.export, so consider reusing/extending it (e.g. add anexclude/extra-patterns arg) rather than adding a second, slightly different implementation that will drift from it. -
_resolve_model_pathnow downloads far more than before — swappingallow_patterns=["*.py", "*.json"]forignore_patterns=<weight patterns>turns a few-KB metadata fetch into "download everything that isn't a recognized weight file".*.gguf(and*.npz,*.pkl,*.tar*,*.zip, media assets) are not in the ignore list, so an HF-ID source can now pull multi-GB artifacts at export time. Please either extend the pattern list or keep an allow-list for the download path while broadening only the local copy path. -
trust_remote_codeno longer gates copying executable*.py— the removed docstring explicitly justified that gating; nowmodeling*.py/configuration_*.py/tokenization_*.pyare copied into the export even for native (non-remote-code) loads, and the argument only affects path resolution. That may well be intentional, but it's a behavior/security-posture change that isn't mentioned in the PR body or covered by a test. -
Stale quantization metadata can now leak —
hf_ptq.pyexplicitly supports already-quantized sources (pack-quantized/compressed-tensors, MXFP4). Those checkpoints ship files likerecipe.yaml/quantize_config.json/quant_config.json, which will now be copied verbatim next to a freshly written NVFP4/FP8hf_quant_config.jsonand can confuse deployment stacks. Worth adding to_EXPORT_OWNED_CHECKPOINT_FILES(and deciding explicitly whethergeneration_config.json/preprocessor_config.jsonshould be source-wins or export-wins, since the Megatron helper treats them as modelopt-owned). -
Test coverage is limited to the local-directory happy path: no coverage for the
snapshot_downloadignore-pattern change, for subdirectory skipping, or for thetrust_remote_code=True/Falseparity that just changed.
| print(f"Copied custom model file: {file_path.name}") | ||
| except Exception as e: | ||
| print(f"Warning: Failed to copy {file_path.name}: {e}") | ||
| for file_path in sorted(source_dir.iterdir()): |
There was a problem hiding this comment.
Bot comment.
This reimplements an existing repo helper: modelopt/torch/export/plugins/hf_checkpoint_utils.py::copy_non_safetensor_files_from_ckpt already does "copy every non-safetensors file from a local HF checkpoint dir verbatim" and is the Megatron-Core path this PR says it wants to match (its docstring even documents which files modelopt owns). Since examples/hf_ptq already imports from modelopt.torch.export, consider calling it (extended with an exclude/extra-pattern argument for hf_quant_config.json + the broader weight patterns) instead of maintaining a second, subtly different copy routine.
| ] | ||
|
|
||
| # Resolve the source path (handles both local paths and HF model IDs) | ||
| resolved_source_path = _resolve_model_path(source_path, trust_remote_code) |
There was a problem hiding this comment.
Bot comment.
The old docstring deliberately gated executable code (modeling*.py, configuration_*.py, tokenization_*.py) behind trust_remote_code; after this change those files are copied into the export unconditionally and trust_remote_code only affects HF-ID resolution. That's likely intentional given the goal, but please call it out in the PR body (it's a behavior change relevant to SECURITY.md guidance) and add a test pinning the new parity between trust_remote_code=True/False.
| assert not (export_dir / "model.safetensors.index.json").exists() | ||
| assert not (export_dir / "model-00001-of-00001.safetensors").exists() | ||
| assert not (export_dir / "pytorch_model.bin").exists() | ||
| assert not (export_dir / "pytorch_model.bin.index.json").exists() |
There was a problem hiding this comment.
Bot comment.
Good happy-path test. Missing cases worth adding: (a) subdirectories in the source (e.g. original/) are silently skipped — pin that so the loss of nested sidecars is a deliberate decision; (b) trust_remote_code=False now copies modeling_*.py, which is the actual behavior change; (c) the _resolve_model_path ignore-pattern change (mock snapshot_download and assert what is/ isn't requested), since that's the riskiest part of the diff.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/torch/export/plugins/hf_checkpoint_utils.py`:
- Around line 319-325: Update the sidecar-copy loop around the os.path.isfile
and shutil.copy2 calls to reject symbolic-link sources before copying, using
os.path.islink(sp) (or an equivalent trusted-root validation) so only regular
files from the snapshot are exported. Preserve the existing exclusions and copy
behavior for valid non-symlink sidecars, and add a regression test covering a
symlinked sidecar.
In `@tests/examples/hf_ptq/test_example_utils.py`:
- Around line 122-131: Remove the duplicate
test_copy_custom_model_files_preserves_python_sidecars_with_trust_remote_code
test because the local source path bypasses trust_remote_code behavior and
duplicates existing coverage. If retaining coverage, replace it with a test that
exercises a remotely resolved model path where trust_remote_code changes the
operation.
🪄 Autofix
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: Enterprise
Run ID: f7acf764-633c-430c-84c8-aaf61c4c5bd1
📒 Files selected for processing (3)
examples/hf_ptq/example_utils.pymodelopt/torch/export/plugins/hf_checkpoint_utils.pytests/examples/hf_ptq/test_example_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/hf_ptq/example_utils.py
|
|
/claude review |
| HF_CHECKPOINT_WEIGHT_FILE_PATTERNS = ( | ||
| "*.safetensors", | ||
| "*.safetensors.index.json", | ||
| "*.bin", | ||
| "*.bin.index.json", | ||
| "*.ckpt", | ||
| "*.gguf", | ||
| "*.h5", | ||
| "*.msgpack", | ||
| "*.npy", | ||
| "*.npz", | ||
| "*.onnx", | ||
| "*.pb", | ||
| "*.pickle", | ||
| "*.pkl", | ||
| "*.pt", | ||
| "*.pth", | ||
| "*.tar", | ||
| "*.tar.bz2", | ||
| "*.tar.gz", | ||
| "*.tar.xz", | ||
| "*.tflite", | ||
| "*.tgz", | ||
| "*.zip", |
There was a problem hiding this comment.
[IMPORTANT Compatibility] This tuple is a new public name (no leading underscore) in a module that is star-exported through modelopt/torch/export/plugins/__init__.py (with import_plugin("hf_checkpoint_utils"): from .hf_checkpoint_utils import *), but the module has no __all__.
Two consequences:
- Per CONTRIBUTING.md ("Define the public API with
__all__and re-export viafrom .module import *"), a new public symbol should be declared in__all__. Without one, the star-import also re-exports every module-level import —fnmatch,json,os,shutil,warnings,Iterable,Path,torch,snapshot_download,tqdm— intomodelopt.torch.export.plugins. That's pre-existing, but this PR is the first to add an intentionally-public constant here, which makes the missing__all__load-bearing rather than cosmetic. - Once exported it's an API surface with a compatibility obligation. If it's meant only for
examples/hf_ptq(the sole consumer), name it_HF_CHECKPOINT_WEIGHT_FILE_PATTERNSor put it in__all__deliberately.
Suggested fix: add __all__ = ["HF_CHECKPOINT_WEIGHT_FILE_PATTERNS", "copy_non_safetensor_files_from_ckpt", ...] naming the module's existing public functions, or rename the constant private if it isn't intended as public API.
Separately, on the list contents: *.npy/*.npz/*.pkl/*.pickle/*.zip/*.tar* aren't HF weight formats in the usual sense, and the name HF_CHECKPOINT_WEIGHT_FILE_PATTERNS oversells what it matches. Some legitimate sidecars are .npy (e.g. precomputed statistics shipped alongside a checkpoint) and would be silently dropped. Not blocking, but a name like ..._LARGE_ARTIFACT_PATTERNS would describe the intent (skip bulk binaries) more honestly than "weight files".
| def test_copy_custom_model_files_preserves_python_sidecars_with_trust_remote_code(tmp_path): | ||
| source_dir = tmp_path / "source" | ||
| export_dir = tmp_path / "export" | ||
| source_dir.mkdir() | ||
| export_dir.mkdir() | ||
| (source_dir / "super_v3_reasoning_parser.py").write_text("parser\n") | ||
|
|
||
| example_utils.copy_custom_model_files(str(source_dir), str(export_dir), trust_remote_code=True) | ||
|
|
||
| assert (export_dir / "super_v3_reasoning_parser.py").read_text() == "parser\n" |
There was a problem hiding this comment.
[SUGGESTION] This test doesn't exercise what its name claims. copy_custom_model_files passes trust_remote_code only to _resolve_model_path, which returns immediately at its first branch (if os.path.isdir(model_name_or_path): return model_name_or_path) for a local source_dir. So the flag is never read, and the assertion is identical to what the previous test already covers for super_v3_reasoning_parser.py with trust_remote_code=False.
That matters more than a redundancy nit here, because the trust_remote_code gate on executable *.py copying is precisely the behavior this PR changes: modeling*.py / configuration_*.py / tokenization_*.py are now copied into the export even for native (non-remote-code) loads, whereas the removed docstring explicitly justified gating them. A test named after trust_remote_code that can't observe the flag gives false confidence that the parity was checked.
Suggested fix: either drop this test (the first one covers .py copying), or make it actually meaningful — parametrize trust_remote_code=[True, False] over a modeling_custom.py fixture and assert the (now intentional) equality, so the behavior change is pinned as deliberate rather than incidental.
There was a problem hiding this comment.
Claude review — 3 IMPORTANT, 3 SUGGESTION
Scope: full review (trigger comment carried no scoping instructions). All 4 changed files reviewed: modelopt/torch/export/plugins/hf_checkpoint_utils.py, examples/hf_ptq/example_utils.py, examples/hf_ptq/hf_ptq.py, tests/examples/hf_ptq/test_example_utils.py.
The direction is right, and consolidating onto the existing copy_non_safetensor_files_from_ckpt helper (rather than adding a second implementation) addresses the duplication flagged in the prior round. Blanket-copy-minus-exclusions is a genuinely better contract than the old whitelist. Three things to resolve before merge.
Most impactful findings
1. The HF-model-ID path still uses an extension allowlist (example_utils.py:62-69) — IMPORTANT
The local-dir path now copies everything that is not a weight/export-owned file. The HF-ID path is still bounded by what snapshot_download fetched: *.json, *.md, *.model, *.py, *.tiktoken, *.txt. So the two paths disagree about what a sidecar is, and the same class of bug this PR fixes survives on the HF-ID path for anything outside that list — most notably chat_template.jinja (transformers >= 4.51 saves chat templates as a standalone .jinja; many current hub repos ship one) and extensionless LICENSE/NOTICE, which the shared helper own docstring names as a file it exists to preserve. The new test asserts accuracy_chart.png is copied — true for a local dir, impossible on the HF-ID path, so that assertion reads as broader coverage than it gives.
2. generation_config.json is dropped outright on the TRT-LLM export path (example_utils.py:70-79) — IMPORTANT
_HF_PTQ_EXPORT_OWNED_FILES excludes it unconditionally, but only one of the two callers writes one:
- Unified HF (
hf_ptq.py:935):save_pretrainedwrites it, so the exclusion is correct. - TRT-LLM (
hf_ptq.py:884):export_tensorrt_llm_checkpointwrites onlyconfig.jsonand (auto-quant)quant_cfg.json— nogeneration_configanywhere inmodel_config_export.py/tensorrt_llm_utils.py. It used to be copied here via the old"*.json"code pattern under--trust_remote_code. Now it is neither copied nor written, so the export loses the source sampling defaults.
Contrast unified_export_megatron.py:319-330, which calls the same helper with no exclude_files and then explicitly re-saves GenerationConfig.from_pretrained(...) — it guarantees the file exists before superseding the source. The TRT-LLM path does neither. (config.json exclusion is correct there: the TRT-LLM schema is unrelated and copying over it would corrupt the export.)
3. New public constant without __all__ (hf_checkpoint_utils.py:34-57) — IMPORTANT
HF_CHECKPOINT_WEIGHT_FILE_PATTERNS is public in a module star-exported via plugins/__init__.py, and the module has no __all__ — contrary to CONTRIBUTING.md "Define the public API with __all__". Pre-existing gap, but this is the first intentionally-public symbol added here, which makes it load-bearing: it becomes an API surface with a compat obligation, and the star-import also re-exports fnmatch/json/os/shutil/torch/etc. Add an __all__, or make the constant private if it is only for examples/hf_ptq.
Suggestions (non-blocking)
hf_checkpoint_utils.py:317-326— the hardcoded.safetensorscheck is redundant on the new path but is still the only guard for the Megatron caller, where it misses index files not named exactlymodel.safetensors.index.json. Also, dropping the old per-filetry/exceptmeans one unreadable file (dangling HF-cache blob symlink) now aborts the entire sidecar copy after weights are written.hf_ptq.py:884— the TRT-LLM copy is not behindif args.dist_state.is_main:(the unified-HF one at line 935 is). Pre-existing, but this PR widens the race window by copying more/larger files.test_example_utils.py:122-131—test_..._with_trust_remote_codecannot observe the flag:_resolve_model_pathreturns at its first branch for a local dir, sotrust_remote_codeis never read. It duplicates the previous test. This matters because thetrust_remote_codegate on executable*.pyis exactly what this PR changes —modeling*.py/configuration_*.pyare now copied for native loads too, and a test named after the flag gives false confidence that parity was checked.
Also worth noting: the docstring at example_utils.py:967-971 says source processor files win, while the shared helper docstring (hf_checkpoint_utils.py:298-299) lists preprocessor_config.json as modelopt-owned. Those now contradict for readers of the shared helper.
Risk
Moderate. Confined to the hf_ptq example plus one backward-compatible signature extension (new keyword-only args; the Megatron caller is unaffected). No mode registration, modelopt_state schema, or quantization-math changes. Findings 1 and 2 are silent-data-loss-shaped — the export completes and reports success over an incomplete directory — which is why they are worth fixing rather than deferring.
I did not run the new tests (the sandbox blocked the pytest invocation), so the assertions are reviewed by reading only.
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review of the sidecar-preservation change. Good progress since the last round — most of the critical feedback is now addressed:
Resolved
- Duplicated logic:
copy_custom_model_filesnow delegates tomodelopt/torch/export/plugins/hf_checkpoint_utils.py::copy_non_safetensor_files_from_ckptwith new keyword-onlyexclude_files/exclude_patternsargs. The Megatron caller (unified_export_megatron.py) passes neither, so its behavior is unchanged — signature extension is backward compatible. - Over-download on the HF-ID path:
snapshot_downloadis back on an allow-list (_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS) and is pinned bytest_resolve_model_path_snapshot_download_stays_allowlisted. - Stale quant metadata:
recipe.yaml/yml,quantize_config.json,quant_config.json,quantization_config.jsonare in_HF_PTQ_EXPORT_OWNED_FILESand asserted absent in the new test. - GGUF / other bulk artifacts: covered by
HF_CHECKPOINT_WEIGHT_FILE_PATTERNSand asserted in the test, which also pins subdirectory skipping.
Still unresolved from the previous round
generation_config.jsonis excluded unconditionally, but only the unified-HF caller writes one. On the TRT-LLM pathexport_tensorrt_llm_checkpointwrites onlyconfig.json(+quant_cfg.json) — I confirmed there is nogeneration_configwrite inmodel_config_export.py— so that file used to be copied (via the old*.jsonpattern under--trust_remote_code) and is now neither copied nor written. Silent loss of the source sampling defaults.- The
trust_remote_codebehavior change (executablemodeling*.py/configuration_*.pynow copied for native loads too) is still not mentioned in the PR body, andtest_copy_custom_model_files_preserves_python_sidecars_with_trust_remote_codecannot observe the flag (local dir →_resolve_model_pathreturns at its first branch), so it duplicates the previous test under a misleading name. Two prior reviewers flagged this. - The HF-ID allow-list omits
*.jinja(standalonechat_template.jinjaon transformers ≥ 4.51) and extensionlessLICENSE/NOTICE, so the two code paths still disagree on what a sidecar is — the exact class of bug this PR fixes survives for hub-ID sources.
Plus two smaller items: dropping the per-file try/except makes one unreadable file abort the whole sidecar copy after weights are written, and the new public HF_CHECKPOINT_WEIGHT_FILE_PATTERNS lands in a star-exported module with no __all__.
Direction and test quality are much improved; please resolve (1) and (2) before merge.
| ] | ||
| _HF_PTQ_EXPORT_OWNED_FILES = { | ||
| "config.json", | ||
| "generation_config.json", |
There was a problem hiding this comment.
Bot comment.
Unresolved from the previous round: generation_config.json is excluded for both callers, but only the unified-HF path writes one (save_pretrained). On the TRT-LLM path, export_tensorrt_llm_checkpoint writes only config.json (and quant_cfg.json for auto-quant) — there is no generation_config write anywhere in model_config_export.py. Before this PR the file was copied here via the old "*.json" pattern under --trust_remote_code; now it is neither copied nor written, so the export silently loses the source sampling defaults.
Compare unified_export_megatron.py, which calls the same helper with no exclude_files and then explicitly re-saves GenerationConfig.from_pretrained(...) — i.e. it guarantees the file exists before superseding the source copy.
Suggest either passing the exclusion set in from the caller (so the TRT-LLM branch keeps generation_config.json) or writing a fresh generation_config.json on that path. Excluding config.json on the TRT-LLM path is correct (different schema), so this needs to be per-caller rather than one shared set.
|
|
||
| example_utils.copy_custom_model_files(str(source_dir), str(export_dir), trust_remote_code=True) | ||
|
|
||
| assert (export_dir / "super_v3_reasoning_parser.py").read_text() == "parser\n" |
There was a problem hiding this comment.
Bot comment.
This test can't observe the flag it's named after: copy_custom_model_files only forwards trust_remote_code to _resolve_model_path, which returns immediately for a local directory. So it's an exact duplicate of the .py-copy assertion in the preceding test.
That matters because the trust_remote_code gate on executable *.py is precisely what this PR changes — modeling*.py/configuration_*.py/tokenization_*.py are now copied for native (non-remote-code) loads, whereas the removed docstring explicitly justified gating them. Either drop this test, or parametrize trust_remote_code=[True, False] over a modeling_custom.py fixture and assert the (now intentional) equality so the change is pinned as deliberate. Please also call the behavior change out in the PR body — it's a security-posture change relative to SECURITY.md guidance.
| continue | ||
| if entry.endswith(".safetensors") or entry == "model.safetensors.index.json": | ||
| continue | ||
| if entry in exclude_files or _matches_any_pattern(entry, exclude_patterns): |
There was a problem hiding this comment.
Bot comment.
The old copy_custom_model_files wrapped each shutil.copy2 in try/except and warned per file. The new loop doesn't, so a single unreadable entry (bad permissions, dangling HF-cache blob symlink) aborts the whole sidecar copy — and since this runs after the quantized weights are written, the user gets a traceback on an otherwise-complete export. A per-file try/except + warning would preserve the previous resilience.
Also, the hardcoded entry.endswith(".safetensors") or entry == "model.safetensors.index.json" check is now a second source of truth: for the Megatron caller (no exclude_patterns) it still misses index files not named exactly model.safetensors.index.json. Consider making HF_CHECKPOINT_WEIGHT_FILE_PATTERNS the always-applied baseline and keeping exclude_patterns purely additive.
| "*.tar.xz", | ||
| "*.tflite", | ||
| "*.tgz", | ||
| "*.zip", |
There was a problem hiding this comment.
Bot comment.
HF_CHECKPOINT_WEIGHT_FILE_PATTERNS is a new public name in a module star-exported via plugins/__init__.py, and this module has no __all__ (so the star-import also re-exports fnmatch/json/os/shutil/torch/...). Per CONTRIBUTING.md, declare the public API with __all__, or make the constant private (_HF_...) if examples/hf_ptq is the only intended consumer — otherwise it becomes an API surface with a compat obligation.
Minor naming nit: *.npy/*.npz/*.pkl/*.zip/*.tar* aren't HF weight formats; some legitimate sidecars are .npy (precomputed stats) and will now be silently dropped. Something like ..._LARGE_ARTIFACT_PATTERNS would describe the intent more honestly.
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
d53d4fc to
da68df5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review #3 (head of hf_ptq_sidecars, 3 files / +177-68). The critical items from the previous rounds are all resolved in the current diff:
- Duplicated logic —
copy_custom_model_filesnow delegates tohf_checkpoint_utils.copy_non_safetensor_files_from_ckptwith a keyword-onlyexclude_files; the Megatron caller passes none, so the signature change is backward compatible. - Over-download on the HF-ID path — back on an allow-list (
_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS), now including*.jinja/LICENSE*/NOTICE*, and pinned bytest_resolve_model_path_snapshot_download_stays_allowlisted. - Stale quant metadata / GGUF & bulk artifacts —
recipe.yaml|yml,quantize_config.json,quant_config.json,quantization_config.jsonin_HF_PTQ_EXPORT_OWNED_FILES; weights/GGUF/archives in_HF_CHECKPOINT_WEIGHT_FILE_PATTERNS; all asserted absent in the new test. - New public symbol without
__all__— constant renamed_HF_CHECKPOINT_WEIGHT_FILE_PATTERNS(private). - Second source of truth for weight skipping — the hardcoded
.safetensors/model.safetensors.index.jsonchecks are gone; the pattern list is now the always-applied baseline, which also closes the Megatron index-file gap. - Misnamed
trust_remote_codetest — dropped (commit d53d4fc), one of the two options both prior reviewers offered.
Two things I'd like the owner to sign off on rather than approve blind, plus small nits. No prompt-injection attempts observed in the PR content.
-
💬
generation_config.json— author resolved the previously-flagged silent loss by removing it from_HF_PTQ_EXPORT_OWNED_FILESand documenting "source generation files intentionally still win" in the docstring, and the new test pins it. Still worth a human look because this also flips behavior on the unified-HF path:save_pretrainedwrites a validatedgeneration_config.jsonand the source copy now overwrites it (previously it was export-wins). That's the deliberate choice the earlier review asked for, but it applies to both callers, so please confirm source-wins is intended for the unified-HF export too — and note the Megatron helper's own path (unified_export_megatron.py) does the opposite (re-savesGenerationConfig.from_pretrainedafter the copy). -
Shared-library behavior change is untested/unmentioned.
copy_non_safetensor_files_from_ckptpreviously skipped only*.safetensors+model.safetensors.index.json; it now also skips*.bin,*.pt,*.pth,*.ckpt,*.h5,*.msgpack,*.npy,*.npz,*.pkl/.pickle,*.onnx,*.pb,*.tflite, and archives. The Megatron export caller passes noexclude_patterns, so its sidecar set shrank — mostly a fix (source weights no longer land in the export), but*.npy/*.npz/*.pklare not always weights (precomputed stats/filters shipped next to a checkpoint) and would now be dropped silently.tests/unit/torch/export/test_hf_checkpoint_utils.pygained no coverage for this function (only the example-level test exercises it indirectly), and neither the PR body nor CHANGELOG mentions the library-side change.
Minor / owner's call:
- Per-file
try/except+ warning aroundshutil.copy2was dropped (flagged in two prior rounds, still open): one unreadable file or dangling HF-cache blob symlink now aborts the whole sidecar copy after the quantized weights are written. - The download allow-list and the copy exclusion list remain two independent sources of truth for "what is a sidecar"; nothing ties them together in a test, so they can drift again.
- The new test no longer covers subdirectory skipping (an earlier round's review noted it as pinned); worth re-adding one
source/original/…fixture assertion since nested sidecars are silently skipped by design. - The
trust_remote_codecopy-parity change (executablemodeling*.py/configuration_*.pynow copied for native loads too) is documented in the new docstring but still not called out in the PR body, and the checkbox list in the description is left unfilled (Changelog / tests / BC). - CodeRabbit's symlink-dereference findings look inapplicable here: HF cache snapshots are symlink-to-blob trees, so rejecting symlink sources would break the cache path, and
shutil.copy2was already used before this PR. Fine to skip, but say so explicitly if you want the bot comments resolved.
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review #4 (head of hf_ptq_sidecars, 5 files / +234-72). Most of the previously-flagged items are now resolved — but this revision introduces a new critical regression in the shared library helper, and the accompanying new unit test appears to assert behavior the implementation no longer has (i.e. it should fail in CI).
Blocking (new in this revision)
copy_non_safetensor_files_from_ckptno longer skips anything by default: this diff deletes theentry.endswith(".safetensors") or entry == "model.safetensors.index.json"guard and replaces it only with caller-suppliedexclude_files/exclude_patterns. The weight-pattern list moved out of the library intoexamples/hf_ptq(_HF_PTQ_WEIGHT_FILE_PATTERNS), so the other caller —unified_export_megatron.py:GPTModelExporter.save_pretrained, which passes no exclusions — will now copy the source BF16*.safetensorsshards andmodel.safetensors.index.jsoninto the Megatron export directory, right beforesave_safetensors_by_layer_indexwrites its own shards/index. That both blows up export size and can leave stale source shards + a stale index next to the quantized ones (differing shard counts → broken/incorrect checkpoint on load). An earlier review round suggested making the pattern list the always-applied baseline withexclude_patternspurely additive; that is the fix here (or restore the hardcoded skip). The function name and docstring ("Copy every non-safetensors file…") are also now inaccurate.tests/unit/torch/export/test_hf_checkpoint_utils.py::test_copy_non_safetensor_files_from_ckpt_supports_additional_exclusionsassertsnot (default_dst / "model.safetensors").exists()for a default (no-exclusion) call — with the current implementationmodel.safetensorsis copied, so this test should fail. Please confirm CI ran; the assertion encodes the behavior the library ought to have, which reinforces the point above.
Resolved since re-review #3
generation_config.jsonis now per-caller:hf_ptq.pypassesexclude_files={"generation_config.json"}only on the unified-HF path (wheresave_pretrainedwrites a validated one) andNoneon the TRT-LLM path (source-wins, restoring the previously-dropped file). That closes the long-standing item 1.- Public constant → example-local private
_HF_PTQ_WEIGHT_FILE_PATTERNS(no__all__obligation), and*.npy/npz/pklare no longer imposed on the Megatron caller. - Download allow-list includes
*.jinja/LICENSE*/NOTICE*and is pinned by a test; stale quant metadata and GGUF/bulk artifacts are excluded and asserted. - The misnamed
trust_remote_codetest was dropped.
Still open / minor (owner's call)
- Per-file
try/except+ warning aroundshutil.copy2is still absent (flagged in three rounds): one unreadable file or dangling HF-cache blob symlink aborts the whole sidecar copy after the quantized weights are written. - Download allow-list vs. copy exclusion list remain two independent sources of truth, untied by any test.
- No coverage for subdirectory skipping (nested sidecars are silently dropped by design).
- PR body still doesn't mention the
trust_remote_codecopy-parity change (executablemodeling*.py/configuration_*.pynow copied for native loads) and the checkbox list (tests / changelog / BC) is unfilled. - CodeRabbit's symlink-dereference findings look inapplicable (HF cache snapshots are symlink-to-blob trees); worth replying to close them out.
No prompt-injection attempts observed in the PR content.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/torch/export/plugins/hf_checkpoint_utils.py`:
- Around line 266-268: Update copy_non_safetensor_files_from_ckpt() so omitted
exclude_patterns defaults to the existing safetensor and safetensor-index
exclusions, while still appending any caller-provided patterns. Preserve
exclude_files handling and ensure model.safetensors and its index are never
copied by default.
🪄 Autofix
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: Enterprise
Run ID: c854edc4-fa57-4cba-b66d-bfd52107b615
📒 Files selected for processing (5)
examples/hf_ptq/example_utils.pyexamples/hf_ptq/hf_ptq.pymodelopt/torch/export/plugins/hf_checkpoint_utils.pytests/examples/hf_ptq/test_example_utils.pytests/unit/torch/export/test_hf_checkpoint_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/examples/hf_ptq/test_example_utils.py
- examples/hf_ptq/example_utils.py
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review #5 (5 files, +236/-70). The blocking regression from the last round is fixed: copy_non_safetensor_files_from_ckpt again applies the hardcoded *.safetensors / model.safetensors.index.json skip as an unconditional baseline, so unified_export_megatron.py::GPTModelExporter.save_pretrained (which passes no exclusions) can no longer copy source BF16 shards + a stale index into the Megatron export, and tests/unit/torch/export/test_hf_checkpoint_utils.py::test_..._supports_additional_exclusions is now consistent with the implementation (default call: safetensors/index skipped, pytorch_model.bin/stats.npy copied; exclude_patterns additive). generation_config.json is correctly per-caller now (exclude_files={"generation_config.json"} only on the unified-HF path where save_pretrained writes a validated one; None on the TRT-LLM path so the source file is preserved), the download allow-list covers *.jinja/LICENSE*/NOTICE* and is pinned by a test, stale quant metadata + GGUF/bulk artifacts are excluded and asserted, and the previously misnamed trust_remote_code test was dropped. Design/dedup concern is settled — the example now delegates to the shared helper rather than maintaining a second copier. No prompt-injection attempts observed in the PR content.
Not blocking, but worth owner eyes before merge:
- Per-file
try/except+ warning aroundshutil.copy2is still absent (flagged in four rounds). The old example code warned and continued; now one unreadable file or dangling HF-cache blob symlink raises after the quantized weights are written, on an otherwise-complete export. - The
trust_remote_codecopy-parity change (executablemodeling*.py/configuration_*.py/tokenization_*.pyare now copied for native, non-remote-code loads too) is documented only in the new docstring — the PR body still doesn't mention it and the checkbox list (tests / changelog / BC) is unfilled. That's a security-posture change relative to SECURITY.md guidance and deserves an explicit human sign-off. - On the TRT-LLM path the sidecar copy now runs twice: once inside the branch (
hf_ptq.py:884, not guarded byargs.dist_state.is_main) and again aftertokenizer.save_pretrained(hf_ptq.py:936,exclude_files=None). Pre-existing, but this PR both widens the set/size of copied files and touches the second call site, so the redundant unguarded multi-rank write is now more costly. - The download allow-list (
_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS) and the copy exclusion list (_HF_PTQ_WEIGHT_FILE_PATTERNS+_HF_PTQ_EXPORT_OWNED_FILES) remain two independent definitions of "what is a sidecar" with no test tying them together, and subdirectory skipping (nested sidecars silently dropped by design) is still uncovered. - CodeRabbit's symlink-dereference findings look inapplicable (HF snapshots are symlink-to-blob trees and
shutil.copy2predates this PR); fine to skip, but a one-line reply would close those threads.
|
/claude review note that the symlinks are not going to be an issue in the BF16 models; TRTLLM duplicate copy existed before this PR anyway. this PR's goal is to only widen the set of sidecar files copied into the PTQ model |
| local_path = snapshot_download( | ||
| repo_id=model_name_or_path, | ||
| allow_patterns=["*.py", "*.json"], # Only download Python files and config | ||
| allow_patterns=_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS, |
There was a problem hiding this comment.
[IMPORTANT Compatibility] For HF-Hub-ID sources, the widening this PR is about is capped by a second, independent whitelist — so sidecars are still silently dropped.
What happens. When --pyt_ckpt_path is a Hub ID, _resolve_model_path reaches this branch (config._name_or_path is the repo ID string, not a dir, so os.path.isdir() is False). The earlier load_model()/from_pretrained() only pulled config + weights + tokenizer files into the snapshot, so this snapshot_download call is what actually fetches the sidecars that copy_custom_model_files will later copy. Anything outside _HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS is never on disk, so the copy step can't preserve it — the file is dropped exactly as before, just via a different whitelist.
Concrete misses (all copied fine for a local source dir, dropped for a Hub ID):
*.yaml/*.yml— non-quantization config sidecars*.jinja2/*.j2— repos that didn't adopt the.jinjaextensiontokenizer.model.v3,tokenizer.model.v7— Mistral-family tokenizers (*.modeldoes not matchtokenizer.model.v3underfnmatch)*.png/*.jpg— assets referenced by theREADME.mdthis PR now copies, so the copied README ends up with broken images
This isn't a regression (the old list was just ["*.py", "*.json"]), but it leaves the stated goal half-done for one of the two supported source forms, and it re-introduces the brittle-whitelist failure mode the PR set out to remove.
Suggested fix. _HF_PTQ_WEIGHT_FILE_PATTERNS is now comprehensive enough (it already covers *.gguf, *.npz, *.pkl, *.tar*, *.zip, *.h5, *.msgpack) to serve as the download's ignore list, which also collapses the two lists into one policy and removes the drift risk between them:
local_path = snapshot_download(
repo_id=model_name_or_path,
ignore_patterns=_HF_PTQ_WEIGHT_FILE_PATTERNS,
)_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS can then be deleted, and test_resolve_model_path_snapshot_download_stays_allowlisted updated to assert on ignore_patterns. If you'd rather keep an allow-list for the download path, please at least add *.yaml, *.yml, *.jinja2, tokenizer.model*, and a comment noting that the two lists must be kept in sync.
| copied_files = copy_non_safetensor_files_from_ckpt( | ||
| source_dir, | ||
| export_dir, | ||
| exclude_files=_HF_PTQ_EXPORT_OWNED_FILES | set(exclude_files or ()), | ||
| exclude_patterns=_HF_PTQ_WEIGHT_FILE_PATTERNS, | ||
| ) |
There was a problem hiding this comment.
[IMPORTANT Compatibility] Delegating to the shared helper drops the per-file try/except, so one unreadable sidecar now aborts the entire export at the final step.
What changed. The old loop isolated failures per file:
try:
shutil.copy2(file_path, dest_path)
copied_files.append(file_path.name)
print(f"Copied custom model file: {file_path.name}")
except Exception as e:
print(f"Warning: Failed to copy {file_path.name}: {e}")copy_non_safetensor_files_from_ckpt has no equivalent guard — shutil.copy2 raises straight out of the loop.
Why it matters more now than before. Two effects compound:
- This PR deliberately widens the copy set from ~11 curated names to every non-weight top-level file, so the number of
copy2calls that can fail grows a lot, and now includes files nobody vetted (.gitattributes, root-owned files in a shared HF cache, README assets, arbitrary vendor sidecars). copy_custom_model_filesis the last thingexport_quantizeddoes (hf_ptq.py:937), after calibration and afterexport_hf_checkpointhas written all the quantized weights. So the failure mode is: hours of PTQ complete, weights are on disk, then the run dies on aREADME.mdcopy and the user sees a traceback instead ofQuantized model exported to: ....
Concrete triggers, all reachable in practice:
shutil.SameFileErrorwhen--export_pathresolves to the same directory as--pyt_ckpt_path(in-place re-export). Previously warned per file and continued; now it's a hard crash.PermissionErrorfromcopy2'scopystaton a destination filesystem that rejectschmod/utime(bind-mounted volumes, NFS, some container overlays) — note the bytes are already written whencopystatfails, so the file lands but the export still dies.PermissionError/OSErrorreading a mode-600file owned by another user in a sharedHF_HOMEcache.
Suggested fix. Keep the shared helper as-is (the Megatron caller wants strictness) and restore per-file tolerance in this example wrapper, since a missing sidecar is not worth discarding a completed quantization run. Either wrap the call:
try:
copied_files = copy_non_safetensor_files_from_ckpt(...)
except Exception as e:
print(f"Warning: Failed to copy checkpoint sidecar files: {e}")
copied_files = []or, better, add an opt-in continue_on_error: bool = False to copy_non_safetensor_files_from_ckpt that wraps just the shutil.copy2 in try/except and warns, and pass continue_on_error=True here. The second form preserves the "copy as much as possible" intent instead of giving up on the first bad file.
| exclude_files = None if is_tensorrt_llm_export else {"generation_config.json"} | ||
| copy_custom_model_files( | ||
| args.pyt_ckpt_path, | ||
| export_path, | ||
| args.trust_remote_code, | ||
| exclude_files=exclude_files, | ||
| ) |
There was a problem hiding this comment.
[IMPORTANT Export] The widened copy now lets the source tokenizer_config.json overwrite the exported one, which can leave the export with two chat templates in conflicting formats.
Why this is new. Before this PR, tokenizer_config.json was only copied when trust_remote_code was set (it matched code_patterns' "*.json", not always_copy_patterns). Now it is copied unconditionally on every export path. The docstring's "source tokenizer and processor files intentionally still win" is a reasonable policy in isolation, but it interacts badly with the transformers 4.x → 5.x chat-template relocation that the removed docstring in this same PR explicitly calls out.
Failure scenario (very common repo layout — Llama/Qwen/Mistral native, no --trust_remote_code):
- Source repo is transformers-4.x era:
tokenizer_config.jsoncarries an embedded"chat_template": "...", and there is nochat_template.jinja. tokenizer.save_pretrained(export_path)athf_ptq.py:929runs under transformers 5.x, which splits the template out: it writeschat_template.jinjaand atokenizer_config.jsonwith thechat_templatekey removed.copy_custom_model_filesthen overwritestokenizer_config.jsonwith the source copy, re-introducing the embeddedchat_template.- The export directory now contains both
chat_template.jinjaandtokenizer_config.json["chat_template"].
Transformers treats a template defined in both places as a conflict rather than a benign duplicate — please verify against the transformers version this example targets, since the outcome is either a hard ValueError on AutoTokenizer.from_pretrained (breaking vLLM/SGLang serving of the exported checkpoint outright) or a version-dependent silent precedence choice. Either way the export has two sources of truth for the template where a plain save_pretrained would have produced one, and this isn't covered by the new tests.
Suggested fix. Make the exclusion decision reflect what the export actually wrote, rather than hardcoding a single filename. generation_config.json is already handled this way in spirit; extend it:
if args.dist_state.is_main:
exclude_files = set()
if not is_tensorrt_llm_export:
# export_hf_checkpoint() writes a sanitized generation_config.json
# (see _sanitize_generation_config_for_save); don't let the source clobber it.
exclude_files.add("generation_config.json")
if (Path(export_path) / "chat_template.jinja").exists():
# tokenizer.save_pretrained() relocated the chat template out of
# tokenizer_config.json; copying the source config back would restore a
# duplicate template in the legacy embedded format.
exclude_files.add("tokenizer_config.json")
copy_custom_model_files(
args.pyt_ckpt_path,
export_path,
args.trust_remote_code,
exclude_files=exclude_files or None,
)If you'd rather keep source-wins for tokenizer_config.json, then also drop the exported chat_template.jinja in that case so exactly one template format survives — and please add a test fixture with an embedded chat_template in the source tokenizer_config.json asserting the export ends up with only one.
| _HF_PTQ_EXPORT_OWNED_FILES = { | ||
| "config.json", | ||
| "hf_quant_config.json", | ||
| "quant_config.json", | ||
| "quantization_config.json", | ||
| "quantize_config.json", | ||
| "recipe.yaml", | ||
| "recipe.yml", | ||
| } |
There was a problem hiding this comment.
[SUGGESTION] The name _HF_PTQ_EXPORT_OWNED_FILES conflates two different policies, which makes the set easy to misextend later.
config.json and hf_quant_config.json really are export-owned — ModelOpt writes them, so skipping the source copy is an overwrite-protection. But ModelOpt never writes quant_config.json, quantization_config.json, quantize_config.json, recipe.yaml, or recipe.yml; for those, skipping means the file is dropped entirely from the export. That's the intended behavior for an already-quantized source (stale compressed-tensors/llm-compressor metadata next to a fresh hf_quant_config.json confuses deployment stacks), but it's the opposite of "owned," and a future reader adding a name here can't tell which semantics they're getting.
Worth splitting so the intent is self-documenting, and so the recipe.yaml breadth is visible — recipe.yaml is a generic filename that a non-quantized source could plausibly use for something unrelated, and it will be silently discarded:
# Written by the export itself; the source copy must not clobber them.
_HF_PTQ_EXPORT_OWNED_FILES = {
"config.json",
"hf_quant_config.json",
}
# Quantization metadata from an already-quantized source (compressed-tensors,
# llm-compressor, MXFP4). Dropped, not overwritten: carrying it next to a freshly
# written hf_quant_config.json makes deployment stacks pick the wrong algorithm.
_HF_PTQ_STALE_QUANT_METADATA_FILES = {
"quant_config.json",
"quantization_config.json",
"quantize_config.json",
"recipe.yaml",
"recipe.yml",
}and at the call site: exclude_files=_HF_PTQ_EXPORT_OWNED_FILES | _HF_PTQ_STALE_QUANT_METADATA_FILES | set(exclude_files or ()).
| Args: | ||
| source_path: Path to the original model directory or HuggingFace model ID | ||
| export_path: Path to the exported model directory | ||
| trust_remote_code: Whether trust_remote_code was used (gates the executable code files) | ||
| trust_remote_code: Whether trust_remote_code was used when resolving HuggingFace model | ||
| IDs | ||
| exclude_files: Additional source file names to skip. |
There was a problem hiding this comment.
[SUGGESTION] The trust_remote_code docstring is now misleading about what the parameter does.
"Whether trust_remote_code was used when resolving HuggingFace model IDs" describes the plumbing but not the consequence, and the consequence is the part that changed in this PR: *.py files (modeling*.py, configuration_*.py, tokenization_*.py, custom reasoning parsers) are now copied into the export regardless of this flag. Previously they were gated behind it, and the removed docstring stated that gating explicitly. Since executable code in the export directory is a security-posture question, a reader checking "does this flag stop remote code from landing in my export?" should get a straight answer here rather than having to diff against main.
trust_remote_code: Forwarded to ``AutoConfig.from_pretrained`` when ``source_path``
is a HuggingFace model ID, so the source snapshot can be resolved. It does
**not** gate which files are copied: remote-code modules present in the source
checkpoint are copied either way, since deployment stacks need them to load a
custom-architecture export.
Same for the sentence "Weight and weight-index files are skipped" — worth noting the skip is top-level only (the helper does not recurse), so a source with weights in a subdirectory is not covered.
| *, | ||
| exclude_files: Iterable[str] | None = None, | ||
| exclude_patterns: Iterable[str] | None = None, | ||
| ) -> list[str]: | ||
| """Copy every non-safetensors file from a local HF checkpoint dir verbatim. | ||
|
|
||
| Use as a baseline so tokenizer files, remote_code ``*.py``, README, LICENSE, etc. | ||
| are preserved from the source. The caller is expected to overwrite the files | ||
| modelopt owns (``config.json``, ``generation_config.json``, ``hf_quant_config.json``, | ||
| ``preprocessor_config.json``) after this step. | ||
| are preserved from the source. Callers can exclude additional files or patterns when |
There was a problem hiding this comment.
[SUGGESTION] The removed sentence documented a contract the Megatron caller still depends on.
The old docstring said "The caller is expected to overwrite the files modelopt owns (config.json, generation_config.json, hf_quant_config.json, preprocessor_config.json) after this step." That is exactly what unified_export_megatron.py:319-330 does — it calls this helper first, then self._hf_config.save_pretrained() and generation_config.save_pretrained() to overwrite. The replacement text ("Callers can exclude additional files or patterns when copying after export-owned metadata has already been written") describes only the new ordering used by hf_ptq, so the two callers now use opposite orderings and the docstring only documents one of them.
Both are valid, but a future caller reading this can't tell which one to follow. Suggest documenting both:
Use as a baseline so tokenizer files, remote_code ``*.py``, README, LICENSE, etc.
are preserved from the source. Two call orderings are supported:
* copy first, then overwrite the files modelopt owns (``config.json``,
``generation_config.json``, ``hf_quant_config.json``) — see
``unified_export_megatron``;
* write modelopt-owned metadata first, then copy with those names passed via
``exclude_files``/``exclude_patterns`` — see ``examples/hf_ptq``.
Copies top-level regular files only; subdirectories are not traversed.
The "top-level only" note is worth adding regardless — it's the pre-existing behavior but it's not stated anywhere, and it's load-bearing now that the exclusion patterns are the only thing keeping source weights out.
There was a problem hiding this comment.
Claude review — 3 IMPORTANT, 3 SUGGESTION
Scope applied. Full review of the 5 changed files (~236/-70). Per the trigger comment I treated the HF-cache symlink question and the pre-existing duplicate copy_custom_model_files call on the TensorRT-LLM path (hf_ptq.py:885 + :937) as out of scope — neither is introduced here. I reviewed against the stated goal: widen the set of sidecar files copied into the PTQ export, nothing more.
The direction is right, and consolidating onto copy_non_safetensor_files_from_ckpt instead of a second parallel implementation addresses the main structural feedback from the earlier round. The exclusion sets are well chosen and the exclude_files/exclude_patterns additions are keyword-only, so the Megatron caller at unified_export_megatron.py:319 is unaffected. Findings below are about the edges of the widening, not the approach.
IMPORTANT
1. The widening doesn't reach HF-Hub-ID sources (example_utils.py:944) — the most impactful finding. When --pyt_ckpt_path is a Hub ID, _resolve_model_path falls through to snapshot_download(allow_patterns=_HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS), and anything outside that list is never on disk for the copy step to preserve. So the brittle whitelist this PR removes from the copy path is still in force on the download path: *.yaml/*.yml, *.jinja2, Mistral-family tokenizer.model.v3/.v7 (*.model doesn't match under fnmatch), and README image assets are all still dropped for Hub-ID sources while being copied fine for local dirs. _HF_PTQ_WEIGHT_FILE_PATTERNS is already comprehensive enough to use as ignore_patterns here, which collapses the two lists into one policy and removes the drift risk.
2. Loss of per-file error tolerance (example_utils.py:1030-1035) — the old loop wrapped each shutil.copy2 in try/except and warned; the shared helper does not. Because this is the last step of export_quantized, after the quantized weights are already written, a single unreadable sidecar now turns a completed PTQ run into a traceback. Reachable triggers: shutil.SameFileError when export path == source path (previously warned and continued, now a hard crash), copystat PermissionError on bind-mounted/NFS destinations, and mode-600 files in a shared HF_HOME. Widening the copy set multiplies the exposure.
3. Possible duplicate chat template in the export (hf_ptq.py:936-942) — tokenizer_config.json was previously copied only under trust_remote_code; it is now copied unconditionally and overwrites what tokenizer.save_pretrained() wrote. For a transformers-4.x-era source loaded under transformers 5.x, save_pretrained splits the template into chat_template.jinja and strips it from tokenizer_config.json — then the source copy puts the embedded chat_template back, leaving both. Transformers treats a template defined in both places as a conflict rather than a benign duplicate, so this is either a hard ValueError on AutoTokenizer.from_pretrained (breaking vLLM/SGLang serving of the export) or a version-dependent silent precedence pick; worth verifying against the transformers version this example targets. Not covered by the new tests. Suggested fix: also exclude tokenizer_config.json when the export wrote a chat_template.jinja.
SUGGESTION
_HF_PTQ_EXPORT_OWNED_FILESmixes overwrite-protection (config.json,hf_quant_config.json— ModelOpt writes these) with outright dropping (recipe.yaml,quantize_config.json— ModelOpt never writes these). Splitting the set makes the intent legible and surfaces that the genericrecipe.yamlname is silently discarded.- The
trust_remote_codedocstring no longer says that*.pycopying is no longer gated by it — a security-posture change the old docstring stated explicitly. copy_non_safetensor_files_from_ckpt's new docstring documents only the hf_ptq ordering (write-then-copy-with-exclusions); the Megatron caller uses the opposite ordering (copy-then-overwrite) and that contract was deleted. Also worth stating "top-level regular files only," which is load-bearing now that exclusion patterns are the only thing keeping source weights out.
Verified as correct
*.safetensors.index.json/*.bin.index.jsonpatterns correctly generalize the helper's hardcoded exact-namemodel.safetensors.index.jsoncheck.- Excluding
generation_config.jsonon the unified path is a real fix, not a regression:export_hf_checkpointruns_sanitize_generation_config_for_save(forcesdo_sample=Truewhentop_k/top_pare set,unified_export_hf.py:1424-1434) beforesave_pretrained, and undertrust_remote_codethe source file previously clobbered that sanitized output. Keepingexclude_files=Nonefor the TensorRT-LLM path is right —export_tensorrt_llm_checkpointdoesn't write one. preprocessor_config.jsonremains source-wins for VLMs, matching pre-PR behavior and theAutoProcessor.save_pretrainedordering athf_ptq.py:851.Iterablewas already imported inexample_utils.py; no lazy-import or plugin-gating regressions (hf_checkpoint_utilsstays behindimport_plugin).
Risk: moderate. No CRITICAL issues — the algorithm and export metadata handling are sound, and the state/mode surface is untouched. The residual risk is concentrated in finding 3 (potentially unloadable tokenizer for a common source layout) and finding 2 (late-stage crash discarding a completed quantization run). Finding 1 means the PR's stated goal is only half-delivered for Hub-ID sources.
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
Signed-off-by: Jennifer Chen <jennifchen@nvidia.com>
What does this PR do?
Type of change: Bug fix
In
hf_ptq.pywhen exporting a PTQ checkpoint, it would drop some files from the original BF16 checkpoint because it uses a whitelist pattern to allow certain files. However that is brittle and can drop files such as reasoning parsers.Now we make hf_ptq.py match Megatron-Core export behavior by copying all non-safe tensor files
Usage
# Add a code snippet demonstrating how to use thisTesting
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: ✅ / ❌ / N/AAdditional Information
Summary by CodeRabbit
Bug Fixes
Tests