Skip to content

fix(cli): match model files by relative path when verifying - #2104

Open
ebarkhordar wants to merge 1 commit into
kvcache-ai:mainfrom
ebarkhordar:fix/2100-model-verify-recursive-relpath
Open

fix(cli): match model files by relative path when verifying#2104
ebarkhordar wants to merge 1 commit into
kvcache-ai:mainfrom
ebarkhordar:fix/2100-model-verify-recursive-relpath

Conversation

@ebarkhordar

Copy link
Copy Markdown

What does this PR do?

Fixes #2100

kt model verify reports every .json and .py file, and every file in a
subdirectory, as missing on a healthy model, then flags the model as
potentially corrupted (sha256_status: failed). In the issue, 16 present files
were called missing (5 top-level configs plus 11 files under encoding/ and
inference/); only the 46 weight shards passed.

Root cause

fetch_model_sha256 returns the remote hash set for *.safetensors, *.json
and *.py at any depth, keyed by repo-relative path (e.g. inference/config.json).
The local side did not mirror that:

  • The first-verify path hashed only *.safetensors (in the verify_model
    command flow it even built the correct three-pattern list, then discarded it via
    files_list=files_to_hash if files_to_verify else None), so no .json/.py
    hash existed to match against.
  • The glob was non-recursive, so files under encoding/ and inference/ were
    never scanned.
  • calculate_local_sha256 keyed results by basename and the comparison matched by
    basename, so config.json and inference/config.json collided into one entry.

The fix

  • list_local_model_files() enumerates local files recursively over the same
    three suffixes the remote set uses.
  • calculate_local_sha256() keys each hash by the path relative to the model
    directory (POSIX), so same-named files in different directories stay distinct.
  • compare_local_to_official() matches each remote entry by its full relative
    path. Both reachable callers (kt model verify and the pre-run/pre-quant
    pre_operation_verification) now use these, replacing three near-identical
    basename-matching loops.

Invariant: a file verifies iff its repo-relative path is present locally with a
matching hash; basename is never used for matching.

Verification

Reproduced and fixed in a clean python:3.11-slim container against HEAD
d1a3ed8. model_verifier.py is pure stdlib + requests, so the tests run
without building the extension.

New tests in kt-kernel/test/per_commit/test_model_verifier_relpath.py (registered
for the CPU default suite):

  • test_calculate_local_sha256_keys_by_relative_path: with config.json and
    inference/config.json present, both relative-path keys are produced with
    distinct hashes. Fails on the unfixed code (the two collapse into one
    config.json entry), passes here.
  • test_list_local_model_files_recursive_all_patterns: the scan finds every
    .safetensors/.json/.py file at any depth.
  • test_healthy_model_verifies_clean: the issue [Bug] 无法正确识别模型文件 #2100 layout (top-level configs +
    encoding/ + inference/) reports zero missing and zero mismatched.
  • test_basename_collision_matched_by_relative_path: config.json and
    inference/config.json each match their own remote entry.
  • test_genuinely_missing_and_corrupt_files_still_detected: a deleted subdir file
    is still reported missing and a tampered file is still reported mismatched, so
    the change does not weaken verification.

What I did not verify: I did not run against a live HuggingFace/ModelScope repo
(no network in the sandbox); the remote side is exercised through the documented
relative-path key format that fetch_model_sha256 returns.

Two unused functions in the same module, verify_model_integrity and
verify_model_integrity_with_progress, are imported but never called; I left them
untouched to keep this change focused on the reachable verify paths.

Before submitting

  • Did you read the contributor guideline?
  • Did you write any new necessary tests?

`kt model verify` fetches the remote SHA256 set for *.safetensors, *.json and
*.py at any depth, keyed by repo-relative path, but the local scan only globbed
*.safetensors non-recursively and keyed hashes by basename. On a healthy model
every config/code file, and every file in a subdirectory, was reported "missing"
and the model was flagged as potentially corrupted.

Enumerate local files recursively over the same three suffixes and match each
remote entry by its full relative path. This also distinguishes files that share
a basename across directories, e.g. config.json vs inference/config.json.

Fixes kvcache-ai#2100
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@ebarkhordar

Copy link
Copy Markdown
Author

No rush on this one. The repo's CI has not run this branch: both workflow runs on the head commit (Book-CI, Deploy) are held pending approval, so the only evidence is local, where kt-kernel/test/per_commit/test_model_verifier_relpath.py fails on unfixed model_verifier.py and passes on this branch. Happy to split the two touched call sites into separate commits, or to narrow the fix to the top-level case only, if either makes review easier.

@ebarkhordar

Copy link
Copy Markdown
Author

#2142 fixes the other half of #2100 in this same file, and the two interact on the reporting side, so the merge order is worth choosing deliberately.

The diffs do not overlap. #2142 changes one loop in _fetch_from_huggingface (old lines 371 to 380); this PR's hunks in model_verifier.py are at 8, 33, 268, 808, 825 and 833.

The interaction, read off the two diffs rather than run: on a first verify both paths hash only *.safetensors, so the .json and .py entries in the official map have no local digest and report (missing). This PR replaces that scan with list_local_model_files, which covers .safetensors, .json and .py. Since _fetch_from_huggingface still returns a git SHA1 blob_id for non-LFS files, those entries would move from (missing) to (hash mismatch) there, which is the repair loop #2142 describes and which today is only reachable through the kt model verify re-verify path.

Merging #2142 first, or both together, avoids that. Happy to rebase this on top of it if that is easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 无法正确识别模型文件

1 participant