[fix](cli): do not compare a git SHA1 against a local SHA256 when verifying - #2142
Open
adityasingh2400 wants to merge 1 commit into
Open
[fix](cli): do not compare a git SHA1 against a local SHA256 when verifying#2142adityasingh2400 wants to merge 1 commit into
adityasingh2400 wants to merge 1 commit into
Conversation
…ifying _fetch_from_huggingface falls back to file_info.blob_id when a file has no lfs entry. huggingface_hub populates RepoFile.blob_id from the Hub's oid, which for a plain git blob is the git SHA1 of the object, 40 hex chars. The returned mapping is then compared against hashlib.sha256 output, 64 hex chars, so any file that is not LFS-tracked can never match. Every HF model repo keeps config.json, generation_config.json, tokenizer_config.json, vocab.json and any *.py as plain git blobs, so those files fail verification unconditionally wherever they are hashed locally. verify_model_integrity does hash them, and so does the kt model verify re-verify path, which then offers a repair download that cannot fix anything because the comparison is between two different algorithms. Return only digests that really are SHA256 and skip the rest, so those files are left unverified instead of reported as corrupt. The sibling ModelScope fetch returns a true SHA256 for every file and is unaffected.
Greptile SummaryThe PR prevents Hugging Face Git blob SHA1 values from being compared with local SHA256 hashes.
Confidence Score: 5/5The PR appears safe to merge because it removes guaranteed false mismatches and its deliberate reduction in non-LFS hash coverage is explicitly documented. The changed fetch path now returns only genuine SHA256 values, and the tests directly cover both retained LFS hashes and omitted Git SHA1 blob identifiers; no unacknowledged actionable defect remains.
|
| Filename | Overview |
|---|---|
| kt-kernel/python/cli/utils/model_verifier.py | Filters out non-LFS Hugging Face metadata rather than treating Git blob SHA1 identifiers as SHA256 digests; the resulting reduction in verification coverage is explicitly acknowledged. |
| kt-kernel/test/per_commit/test_model_verifier_hf_hashes.py | Adds stub-based regression tests covering retained LFS SHA256 values and omitted non-LFS Git blob IDs. |
Reviews (1): Last reviewed commit: "[fix](cli): do not compare a git SHA1 ag..." | Re-trigger Greptile
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #2100.
Summary
_fetch_from_huggingfacebuilds the map of official digests that model verification compares against, but for any file without anlfsentry it falls back toblob_id:huggingface_hubpopulatesRepoFile.blob_idfrom the Hub'soid, and for a plain git blob thatoidis the git SHA1 of the object, 40 hex characters. The map is then compared againsthashlib.sha256output, 64 hex characters, so a non-LFS file can never match.Every HF model repo keeps
config.json,generation_config.json,tokenizer_config.json,vocab.jsonand any*.pyas plain git blobs rather than LFS objects, so those files fail verification unconditionally wherever they are locally hashed.Where it bites
Two paths hash
.jsonand.pylocally and then compare:verify_model_integritycollects*.safetensors,*.jsonand*.pyintolocal_filesand passes them all tocalculate_local_sha256, then compares every entry.kt model verifyre-verify path incli/commands/model.pybuildsfiles_to_hashfrom the same three patterns and passes it asfiles_list, so the.jsonand.pyfiles are hashed and compared.On that second path the user is offered a repair download, the files are re-fetched, re-verified, and mismatch again, because the mismatch is a units error rather than actual corruption. Answering yes cannot terminate.
On the very first
kt model verifyrun the same files instead report(missing), becausecalculate_local_sha256is called withfiles_list=Noneand falls back to the*.safetensorsglob, so there is no local digest to compare at all. That is the(missing)half of #2100 and it is what #2104 addresses. Worth flagging: once #2104 makes the local scan cover.jsonand.py, this bug stops hiding behind(missing)and turns into a hard(hash mismatch)on those files. The two changes are complementary, and they do not overlap. #2104 touchesmodel_verifier.pyat lines 8, 33, 268, 808 and 825, while_fetch_from_huggingfacesits at 330 to 390 and is untouched.Fix
Return only digests that really are SHA256 and skip the rest, so those files are left unverified rather than reported as corrupt. This is confined to the one loop.
The sibling
_fetch_from_modelscopereturns a genuine SHA256 for every file, LFS or not, so the ModelScope path is unaffected and stays fully verified.Verification
Confirmed against the live Hub before writing the fix, using this repo's own function on
hf-internal-testing/tiny-random-gpt2, downloading each file and computing both digests locally:Five of the six files can never pass. Each non-LFS value matched the git blob SHA1 of the same bytes exactly, which is what identifies the digest being returned.
model.safetensors, the only LFS object, matched its local SHA256.Test
kt-kernel/test/per_commit/test_model_verifier_hf_hashes.py, registered viaregister_cpu_ci(est_time=0.1, suite="default"). It follows thetest_port_checker.pypattern of loading the module by file path withimportlib, so it pulls in no torch and no network.huggingface_hubis stubbed insys.modulesbecause the real import is lazy and happens inside the function. The stubRepoFilemirrors the real shape, anlfs.sha256for the weight file and ablob_idonly for the rest, using the real digests observed above.Three assertions: every returned digest is 64 characters, the LFS file keeps its SHA256, and the non-LFS files are absent rather than present with a git SHA1.
Verified against the base ref rather than by stashing:
per_commit.test_port_checkerstill passes, as a check that the CI registration and module loading are intact.Formatted with
blackat line-length 120 fromkt-kernel/pyproject.toml. Note that black 26.x wants to reformat the existingtest_port_checker.pytoo, so I matched the version the tree is actually formatted with, 24.10.0, under which both changed files and that sibling are all clean. Commit message follows the[type](scope): subjectform the commit-msg hook enforces.Possible follow-up
If losing hash coverage on config and tokenizer files is not acceptable, the alternative is to verify non-LFS files with the algorithm the Hub actually gives us, since the git blob SHA1 is cheap to reproduce locally as
sha1(b"blob " + str(len(data)) + b"\0" + data). That needs the comparison site to know which algorithm applies per file, which reaches into the code #2104 is already refactoring, so I kept this change narrow. Happy to do it that way instead if you prefer.