Conversation
get_checksums() flattens every subdir's checksums.txt into one dict keyed by bare filename, namespacing only ".h" entries. Bare filenames are not unique across subdirs: per-arch cubin pins publish the same sm100f/sm103a kernel names built from different sources, so whichever subdir is processed last silently overwrites the earlier one's hashes. Every shared name is then verified against the wrong pin's hash and download_artifacts() fails with "Failed to download cubins: checksum mismatch". main cannot trigger this today because it defines only a single BMM/GEMM pin. It did trigger on release-v0.6.16, which carries a second, Rubin-specific pin: all 2534 shared BMM filenames and 105 of 108 shared GEMM filenames resolved to the wrong hash, taking down build-flashinfer-cubin for the whole release. This is a latent landmine on main that arms itself the moment a second pin lands. Key every entry by full path, and look cubins up the same way headers already were. Add a pin-independent regression test that feeds get_checksums() two subdirs publishing the same kernel name with different hashes; against the unfixed version it fails on the bare filename still being present as a key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChecksum parsing and cubin lookup now consistently use full relative paths, preventing collisions for identical filenames across subdirectories. Tests verify namespaced kernel and header checksum entries. ChangesChecksum path namespacing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📌 Description
get_checksums()flattens every subdir'schecksums.txtinto one dict keyed by bare filename, namespacing only.hentries:Bare filenames are not unique across subdirs. Per-arch cubin pins publish the same
sm100f/sm103akernel names built from different sources, so whichever subdir is processed last silently overwrites the earlier one's hashes. Every shared name is then verified against the wrong pin's hash, anddownload_artifacts()fails withFailed to download cubins: checksum mismatch.get_subdir_file_list()compounded it by looking cubins up aschecksums[name]while headers already used the full path.maincannot trigger this today — it defines only a single BMM/GEMM pin. This PR is therefore a latent-bug fix, not a live one.It did trigger on
release-v0.6.16, which carries a second, Rubin-specific pin. Measured against the live published manifests:TRTLLM_GEN_BMMvs_RUBINTRTLLM_GEN_GEMMvs_RUBIN2640 of 8055 files resolved to the wrong hash, taking down
build-flashinfer-cubinfor the entire release. Fixed there in #4200. Porting it here so the landmine does not re-arm the moment a second pin lands onmain(e.g. when SM107 support returns).Fix: key every entry by full path, and look cubins up the same way headers already were.
🔍 Related Issues
download_artifacts()fails on a real non-Rubin BMM cubin whose expected hash is the Rubin pin's (expected c31c3573… actual 41c8c4fc…); with the fix all 21,839 files download and verify.🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).tests/test_artifacts.py: 5 passed (4 existing + 1 new).The new test is deliberately pin-independent — it feeds
get_checksums()two synthetic subdirs publishing the same kernel name with different hashes, so it does not depend onmainever defining a secondArtifactPath. Against the unfixed version it fails:Reviewer Notes
ArtifactPath.TRTLLM_GEN_BMM_RUBIN, which does not exist onmainand wouldAttributeErrorat collection. Hence the different, synthetic-subdir test here.Sm107aguard from release-v0.6.16: fix cubin checksum collision and unguarded Sm107a build break #4200 is intentionally not included —mainhas zeroSm107areferences since Adds SM107 support #4122 was reverted. Whoever re-lands SM107 support here must bring the#ifdef TLLM_RUBIN_FEATURESguard with it, ormainwill break exactly as the release branch did..hentries already did.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests