-
Notifications
You must be signed in to change notification settings - Fork 531
Preserve HF PTQ checkpoint sidecar files [NV BUG 6491822] #2060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,6 @@ | |
| import json | ||
| import logging | ||
| import os | ||
| import shutil | ||
| import warnings | ||
| from collections.abc import Callable, Iterable | ||
| from dataclasses import dataclass | ||
|
|
@@ -44,6 +43,7 @@ | |
| ) | ||
|
|
||
| from modelopt.torch.export.model_utils import is_multimodal_model | ||
| from modelopt.torch.export.plugins.hf_checkpoint_utils import copy_non_safetensor_files_from_ckpt | ||
|
|
||
| try: | ||
| from huggingface_hub import snapshot_download | ||
|
|
@@ -56,6 +56,27 @@ | |
|
|
||
| SPECULATIVE_MODEL_LIST = ["Eagle", "Medusa"] | ||
|
|
||
| _HF_SIDECAR_DOWNLOAD_ALLOW_PATTERNS = [ | ||
| "*.jinja", | ||
| "*.json", | ||
| "*.md", | ||
| "*.model", | ||
| "*.py", | ||
| "*.tiktoken", | ||
| "*.txt", | ||
| "LICENSE*", | ||
| "NOTICE*", | ||
| ] | ||
|
Comment on lines
+59
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Compatibility] The HF-ID path still uses an extension allowlist, so it reintroduces exactly the brittleness this PR removes from the local-dir path — and the two paths now disagree about what a "sidecar" is. For a local
Why it matters: the reported bug (dropped reasoning parsers) is fixed for Suggested fix: keep the allowlist only as a size guard and make it complete for non-weight text sidecars, e.g. add |
||
| _HF_PTQ_EXPORT_OWNED_FILES = { | ||
| "config.json", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| "hf_quant_config.json", | ||
| "quant_config.json", | ||
| "quantization_config.json", | ||
| "quantize_config.json", | ||
| "recipe.yaml", | ||
| "recipe.yml", | ||
| } | ||
|
Comment on lines
+70
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Compatibility] Tracing both call sites in
Compare Same reasoning applies to Suggested fix: don't hardcode one exclusion set for both paths. Either pass the exclusions in from the caller so the TRT-LLM branch omits While here: the docstring at line 967-971 claims "Source processor files intentionally still win" — with |
||
|
|
||
|
|
||
| @dataclass | ||
| class DistributedState: | ||
|
|
@@ -895,11 +916,13 @@ def _resolve_model_path(model_name_or_path: str, trust_remote_code: bool = False | |
| try: | ||
| 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, | ||
| ) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes an intentionally tiny fetch (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| return local_path | ||
| except Exception as e: | ||
| print(f"Warning: Could not download model files using snapshot_download: {e}") | ||
| print( | ||
| f"Warning: Could not download checkpoint sidecars using snapshot_download: {e}" | ||
| ) | ||
|
|
||
| # Fallback: try to find in HuggingFace cache | ||
| from transformers.utils import TRANSFORMERS_CACHE | ||
|
|
@@ -935,48 +958,23 @@ def _resolve_model_path(model_name_or_path: str, trust_remote_code: bool = False | |
|
|
||
|
|
||
| def copy_custom_model_files(source_path: str, export_path: str, trust_remote_code: bool = False): | ||
| """Copy processor/tokenizer artifacts (and, with trust_remote_code, custom code) to export. | ||
|
|
||
| Processor and tokenizer *data* artifacts -- e.g. a VLM's ``preprocessor_config.json``, | ||
| ``merges.txt``/``vocab.json``, and the processor helper modules -- are needed by the | ||
| deployment stack (vLLM/SGLang) even when the model itself runs on native (non-remote) | ||
| transformers code. transformers 5.x restructured many VLM configs and no longer | ||
| re-saves these on ``save_pretrained`` for models loaded natively, so without copying | ||
| them a native-path export is missing e.g. ``preprocessor_config.json`` and fails to | ||
| load (``Can't load image processor``). These are copied regardless of | ||
| ``trust_remote_code``. Executable model/config code (``modeling*.py``, | ||
| ``configuration_*.py``, ``tokenization_*.py``, and other custom JSON) is only meaningful | ||
| with ``trust_remote_code`` and is copied only then. ``config.json`` and | ||
| ``model.safetensors.index.json`` are always skipped (handled by the export itself). | ||
| """Copy source checkpoint sidecar files to an HF PTQ export. | ||
|
|
||
| The HF PTQ script writes ModelOpt-owned metadata and quantized weights first, then | ||
| copies source checkpoint sidecars so tokenizer/processor files, remote-code modules, | ||
| README assets, parser plugins, and similar deployment files are preserved for both | ||
| native and ``trust_remote_code`` loads. Weight and weight-index files are skipped | ||
| to avoid copying the unquantized source weights. Export-owned metadata (``config.json``, | ||
| ``hf_quant_config.json``) and stale source quantization metadata are also skipped. | ||
| Source tokenizer, processor, and generation files intentionally still win because | ||
| Transformers may not regenerate all metadata in the source format. | ||
|
|
||
| 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 | ||
| """ | ||
| # Deployment-critical processor/tokenizer artifacts: safe to copy regardless of | ||
| # trust_remote_code (data + processor helpers, not model code). | ||
| always_copy_patterns = [ | ||
| "preprocessor_config.json", | ||
| "processor_config.json", | ||
| "image_processing*.py", | ||
| "processing_*.py", | ||
| "video_processing*.py", | ||
| "feature_extraction_*.py", | ||
| "added_tokens.json", | ||
| "special_tokens_map.json", | ||
| "vocab.json", | ||
| "merges.txt", | ||
| "tokenizer.model", | ||
| ] | ||
| # Executable custom model/config code + other custom JSON: only used with trust_remote_code. | ||
| code_patterns = [ | ||
| "configuration_*.py", | ||
| "modeling*.py", | ||
| "tokenization_*.py", | ||
| "*.json", | ||
| ] | ||
|
|
||
| # Resolve the source path (handles both local paths and HF model IDs) | ||
| resolved_source_path = _resolve_model_path(source_path, trust_remote_code) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old docstring deliberately gated executable code ( |
||
|
|
||
|
|
@@ -997,29 +995,18 @@ def copy_custom_model_files(source_path: str, export_path: str, trust_remote_cod | |
| print(f"Warning: Export directory {export_path} does not exist") | ||
| return | ||
|
|
||
| patterns = [*always_copy_patterns, *(code_patterns if trust_remote_code else [])] | ||
|
|
||
| copied_files: list[str] = [] | ||
| for pattern in patterns: | ||
| for file_path in source_dir.glob(pattern): | ||
| if file_path.is_file(): | ||
| # Skip config.json and model.safetensors.index.json as they're handled separately | ||
| if file_path.name in ["config.json", "model.safetensors.index.json"]: | ||
| continue | ||
| if file_path.name in copied_files: # e.g. matched by both pattern lists | ||
| continue | ||
| dest_path = export_dir / file_path.name | ||
| 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}") | ||
| copied_files = copy_non_safetensor_files_from_ckpt( | ||
| source_dir, | ||
| export_dir, | ||
| exclude_files=_HF_PTQ_EXPORT_OWNED_FILES, | ||
| ) | ||
|
|
||
| if copied_files: | ||
| print(f"Successfully copied {len(copied_files)} custom model files to {export_path}") | ||
| for file_name in copied_files: | ||
| print(f"Copied checkpoint sidecar file: {file_name}") | ||
| print(f"Successfully copied {len(copied_files)} checkpoint sidecar files to {export_path}") | ||
| else: | ||
| print("No custom model files found to copy") | ||
| print("No checkpoint sidecar files found to copy") | ||
|
|
||
|
|
||
| def _layerwise_checkpoint_dir_location(algorithm) -> tuple[str, str] | None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,12 @@ | |
|
|
||
| """Hugging Face checkpoint utility.""" | ||
|
|
||
| import fnmatch | ||
| import json | ||
| import os | ||
| import shutil | ||
| import warnings | ||
| from collections.abc import Iterable | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
|
|
@@ -29,6 +31,31 @@ | |
| from tqdm import tqdm | ||
|
|
||
| _HF_HUB_OFFLINE_TRUE_VALUES = {"1", "ON", "YES", "TRUE"} | ||
| _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", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Minor naming nit: |
||
| ) | ||
|
|
||
|
|
||
| def _as_nonnegative_int(value: Any) -> int | None: | ||
|
|
@@ -253,25 +280,43 @@ def load_multimodal_components( | |
| return multimodal_state_dict | ||
|
|
||
|
|
||
| def copy_non_safetensor_files_from_ckpt(src: str | os.PathLike, dst: str | os.PathLike): | ||
| """Copy every non-safetensors file from a local HF checkpoint dir verbatim. | ||
| def _matches_any_pattern(file_name: str, patterns: tuple[str, ...]) -> bool: | ||
| return any(fnmatch.fnmatchcase(file_name, pattern) for pattern in patterns) | ||
|
|
||
|
|
||
| def copy_non_safetensor_files_from_ckpt( | ||
| src: str | os.PathLike, | ||
| dst: str | os.PathLike, | ||
| *, | ||
| exclude_files: Iterable[str] | None = None, | ||
| ) -> list[str]: | ||
| """Copy every non-weight sidecar 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 pass files through ``exclude_files`` when | ||
| copying after export-owned metadata has already been written. | ||
|
|
||
| Args: | ||
| src: Source HF checkpoint directory. Must be a local path. | ||
| dst: Destination directory; created if missing. | ||
| exclude_files: Exact file names to skip in addition to weights and weight indexes. | ||
|
|
||
| Returns: | ||
| File names copied into ``dst``. | ||
| """ | ||
| if not os.path.isdir(src): | ||
| raise ValueError(f"Invalid source path: {src}. It should be a directory.") | ||
| exclude_files = set(exclude_files or ()) | ||
| copied_files = [] | ||
| os.makedirs(dst, exist_ok=True) | ||
| for entry in os.listdir(src): | ||
| for entry in sorted(os.listdir(src)): | ||
| if entry in exclude_files or _matches_any_pattern( | ||
| entry, _HF_CHECKPOINT_WEIGHT_FILE_PATTERNS | ||
| ): | ||
| continue | ||
| sp = os.path.join(src, entry) | ||
| if not os.path.isfile(sp): | ||
| continue | ||
| if entry.endswith(".safetensors") or entry == "model.safetensors.index.json": | ||
| continue | ||
| shutil.copy2(sp, dst) | ||
|
Comment on lines
318
to
320
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
sed -n '280,330p' modelopt/torch/export/plugins/hf_checkpoint_utils.py
printf '\nRelevant callers:\n'
rg -n -A12 -B4 'copy_non_safetensor_files_from_ckpt|copy_custom_model_files' \
modelopt examples tests
printf '\nPython standard-library behavior for the exact operations:\n'
python3 - <<'PY'
import os
import shutil
import tempfile
from pathlib import Path
with tempfile.TemporaryDirectory() as td:
root = Path(td)
source = root / "source"
destination = root / "destination"
source.mkdir()
destination.mkdir()
secret = root / "host-readable-secret.txt"
secret.write_text("sensitive\n")
sidecar = source / "tokenizer_config.json"
sidecar.symlink_to(secret)
print("isfile_symlink:", os.path.isfile(sidecar))
shutil.copy2(sidecar, destination)
copied = destination / sidecar.name
print("copied_exists:", copied.exists())
print("copied_is_symlink:", copied.is_symlink())
print("copied_contents:", copied.read_text())
PYRepository: NVIDIA/Model-Optimizer Length of output: 19748 Sensitive Data Exposure (CWE-59) Reachability: External Reachability pathReject source symlinks before copying checkpoint sidecars.
🤖 Prompt for AI AgentsSource: Path instructions |
||
| copied_files.append(entry) | ||
|
Comment on lines
+312
to
+321
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] Two small things in the copy loop:
Both are non-blocking, but (2) is a real behavior regression for the HF-cache source case, where dangling blob symlinks do occur. |
||
| return copied_files | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local-dir path is now blanket-copy-minus-exclusions, but the HF-model-ID path is still bounded by this extension allow-list, so the two paths disagree about what counts as a sidecar. Notably missing:
chat_template.jinja(transformers ≥ 4.51 saves chat templates as a standalone file, and many current hub repos ship one) and extensionlessLICENSE/NOTICE— the shared helper's own docstring namesLICENSEas something it exists to preserve. Note the new test'saccuracy_chart.pngassertion can only ever hold for a local source dir, so it reads as broader coverage than it gives.At minimum add
"*.jinja","LICENSE*","NOTICE*"; ideally derive both paths from one source of truth (e.g.ignore_patterns=HF_CHECKPOINT_WEIGHT_FILE_PATTERNSfor the download too, which already covers the bulk-artifact over-download concern) and pin download patterns vs. copy exclusions in a test so they can't drift.