Conversation
flashinfer-ai#4280 added ArtifactPath.DEEPGEMM_RUBIN to the cubin_dirs list walked by get_subdir_file_list(), and registered `responses` mocks for the two other new Rubin pins (TRTLLM_GEN_BMM_RUBIN / TRTLLM_GEN_GEMM_RUBIN) but not for the deep-gemm one. Under @responses.activate the unregistered URL raises ConnectionError, so download_file() exhausts its retries and returns False, and get_checksums() then open()s a checksums.txt that was never written: FileNotFoundError: .../cubins/7ec7ac40.../deep-gemm/checksums.txt The artifact itself is fine -- the live manifest SHA matches the pin in CheckSumHash.DEEPGEMM_RUBIN; only the unit test's mock registry was stale. Changes: - Register the DEEPGEMM_RUBIN directory index and checksums.txt, mirroring the BMM/GEMM Rubin pattern (same index body, distinct per-pin hashes). - Extend the per-pin-hash-collision assertion to a shared deep-gemm kernel name, so deep-gemm gets the coverage the trtllm-gen pins already have. - get_checksums(): raise a RuntimeError naming the pin and URL when the manifest download fails, instead of falling through to a bare FileNotFoundError on a local cache path that hides the real cause. AI-assisted (Claude Code): diagnosis and patch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthrough
ChangesChecksum validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Adds the negative-path coverage the mock fix implies: - test_get_checksums_unreachable_pin_raises: a pin whose manifest cannot be fetched must raise, and the error must name the pin. Stubs download_file instead of mocking HTTP so the test does not pay 4 rounds of exponential backoff (~70s), which is what made the original CI failure so slow. - test_get_checksums_falls_back_to_cached_manifest: pins the case that *should* degrade gracefully -- a failed refresh with a manifest already on disk still works, so the new raise does not break offline / FLASHINFER_NO_DOWNLOAD setups. AI-assisted (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
flashinfer/artifacts.py (1)
229-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the missing-manifest error.
get_checksumsnow has a new failure branch at Lines [229]-[237]. The supplied tests cover successful manifests, but they do not exercise a failed download with no cached file. Add a test that assertsRuntimeErrorcontains the artifact pin and manifest URL. This protects the new diagnostic contract.🤖 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 `@flashinfer/artifacts.py` around lines 229 - 237, Add a regression test for the get_checksums failure path where download_file returns false and no checksum manifest exists in the cache. Mock the download and filesystem dependencies as needed, assert that RuntimeError is raised, and verify its message contains both the artifact pin (subdir) and manifest URL.tests/test_artifacts.py (1)
507-511: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the expected checksum for each DeepGEMM path.
The added case only feeds the existing inequality assertion. That detects a bare-filename collision, but it does not prove that the regular path receives the regular manifest hash and the Rubin path receives the Rubin manifest hash. Add expected hashes to this case and assert both exact values. A swapped mapping could otherwise pass.
🤖 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 `@tests/test_artifacts.py` around lines 507 - 511, Update the DeepGEMM case in the artifact-path test to include the expected regular and Rubin manifest checksums, then assert each resolved path against its corresponding exact hash. Keep the existing inequality assertion, and ensure the assertions distinguish the regular mapping from the Rubin mapping so swapped values cannot pass.
🤖 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.
Nitpick comments:
In `@flashinfer/artifacts.py`:
- Around line 229-237: Add a regression test for the get_checksums failure path
where download_file returns false and no checksum manifest exists in the cache.
Mock the download and filesystem dependencies as needed, assert that
RuntimeError is raised, and verify its message contains both the artifact pin
(subdir) and manifest URL.
In `@tests/test_artifacts.py`:
- Around line 507-511: Update the DeepGEMM case in the artifact-path test to
include the expected regular and Rubin manifest checksums, then assert each
resolved path against its corresponding exact hash. Keep the existing inequality
assertion, and ensure the assertions distinguish the regular mapping from the
Rubin mapping so swapped values cannot pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae502f02-aea5-4b84-92c9-0a9c1f866339
📒 Files selected for processing (2)
flashinfer/artifacts.pytests/test_artifacts.py
There was a problem hiding this comment.
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 `@tests/test_artifacts.py`:
- Around line 317-320: Update the RuntimeError assertion in the checksum test
around artifacts.get_checksums to also construct the expected manifest URI using
the same safe_urljoin expression used by the artifact implementation, then
assert that URI appears in str(excinfo.value) alongside DEEPGEMM_RUBIN.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50c63583-a651-4f4d-997c-4f7dcb439fbe
📒 Files selected for processing (1)
tests/test_artifacts.py
| with pytest.raises(RuntimeError) as excinfo: | ||
| artifacts.get_checksums([artifact_paths.DEEPGEMM_RUBIN]) | ||
| # The pin must be named -- that is the whole point of the error. | ||
| assert artifact_paths.DEEPGEMM_RUBIN in str(excinfo.value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the manifest URL as well as the pin.
The RuntimeError contract includes both values. This test checks only DEEPGEMM_RUBIN. An error that omits the URL still passes. Build the expected URI with the same safe_urljoin expression as flashinfer/artifacts.py:222-249 and assert that it is present.
Proposed test assertion
assert artifact_paths.DEEPGEMM_RUBIN in str(excinfo.value)
+ expected_uri = safe_urljoin(
+ artifacts.FLASHINFER_CUBINS_REPOSITORY,
+ safe_urljoin(artifact_paths.DEEPGEMM_RUBIN, "checksums.txt"),
+ )
+ assert expected_uri in str(excinfo.value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(RuntimeError) as excinfo: | |
| artifacts.get_checksums([artifact_paths.DEEPGEMM_RUBIN]) | |
| # The pin must be named -- that is the whole point of the error. | |
| assert artifact_paths.DEEPGEMM_RUBIN in str(excinfo.value) | |
| with pytest.raises(RuntimeError) as excinfo: | |
| artifacts.get_checksums([artifact_paths.DEEPGEMM_RUBIN]) | |
| # The pin must be named -- that is the whole point of the error. | |
| assert artifact_paths.DEEPGEMM_RUBIN in str(excinfo.value) | |
| expected_uri = safe_urljoin( | |
| artifacts.FLASHINFER_CUBINS_REPOSITORY, | |
| safe_urljoin(artifact_paths.DEEPGEMM_RUBIN, "checksums.txt"), | |
| ) | |
| assert expected_uri in str(excinfo.value) |
🤖 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 `@tests/test_artifacts.py` around lines 317 - 320, Update the RuntimeError
assertion in the checksum test around artifacts.get_checksums to also construct
the expected manifest URI using the same safe_urljoin expression used by the
artifact implementation, then assert that URI appears in str(excinfo.value)
alongside DEEPGEMM_RUBIN.
|
/bot run tests/gemm |
…s + CuTe-DSL MoE device guard) (#4301) <!-- .github/pull_request_template.md --> ## 📌 Description <!-- What does this PR do? Briefly describe the changes and why they’re needed. --> `main` is currently red on multiple test files, all stemming from #4280. This PR bundles the fixes so CI can go green in a single run — the gate requires one passing run, and none of these fixes pass in isolation (each file still fails on the others), so they cannot land separately. **Supersedes #4292** . Current PR includes the commit from #4292 and adds the remaining MoE fix on top to pass the CI ## 🔍 Related Issues <!-- Link any related issues here --> - #4280 - #4292 ## 🚀 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 - [ ] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [ ] I have installed the hooks with `pre-commit install`. - [ ] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [ ] Tests have been added or updated as needed. - [ ] All tests are passing (`unittest`, etc.). ## Reviewer Notes <!-- Optional: anything you'd like reviewers to focus on, concerns, etc. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error messages when checksum manifests cannot be downloaded and are unavailable locally, including details about the affected artifact. * Preserved the use of cached checksum manifests when refresh attempts fail, preventing unnecessary errors when valid local data is available. * Improved reliability when validating artifacts across multiple hardware and software configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Alex Yang <aleyang@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[SUCCESS] Pipeline #60450915: 18/18 executed test jobs passed |
|
closing in favor of #4301 |
Pull request was closed
…s + CuTe-DSL MoE device guard) (#4301) <!-- .github/pull_request_template.md --> ## 📌 Description <!-- What does this PR do? Briefly describe the changes and why they’re needed. --> `main` is currently red on multiple test files, all stemming from #4280. This PR bundles the fixes so CI can go green in a single run — the gate requires one passing run, and none of these fixes pass in isolation (each file still fails on the others), so they cannot land separately. **Supersedes #4292** . Current PR includes the commit from #4292 and adds the remaining MoE fix on top to pass the CI ## 🔍 Related Issues <!-- Link any related issues here --> - #4280 - #4292 ## 🚀 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 - [ ] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [ ] I have installed the hooks with `pre-commit install`. - [ ] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [ ] Tests have been added or updated as needed. - [ ] All tests are passing (`unittest`, etc.). ## Reviewer Notes <!-- Optional: anything you'd like reviewers to focus on, concerns, etc. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error messages when checksum manifests cannot be downloaded and are unavailable locally, including details about the affected artifact. * Preserved the use of cached checksum manifests when refresh attempts fail, preventing unnecessary errors when valid local data is available. * Improved reliability when validating artifacts across multiple hardware and software configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Alex Yang <aleyang@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit d020372)
📌 Description
tests/test_artifacts.py::test_get_subdir_file_listhas been failing onmainsince #4280:Root cause. #4280 added
ArtifactPath.DEEPGEMM_RUBINand appended it to thecubin_dirslist walked by
get_subdir_file_list(). The same PR registeredresponsesmocks for the twoother new Rubin pins (
TRTLLM_GEN_BMM_RUBIN,TRTLLM_GEN_GEMM_RUBIN) but not for thedeep-gemm one. Under
@responses.activatean unregistered URL raisesConnectionError, sodownload_file()exhausts its 4 retries and returnsFalse;get_checksums()ignores thatreturn value and
open()s achecksums.txtthat was never written.The artifact itself is fine — I fetched both deep-gemm manifests from artifactory (HTTP 200)
and the live SHA256 of
7ec7ac40…/deep-gemm/checksums.txtmatches the pin inCheckSumHash.DEEPGEMM_RUBIN(09e961d4…). Only the unit test's mock registry was stale, so noproduction download path is affected.
Changes:
tests/test_artifacts.py: register theDEEPGEMM_RUBINdirectory index andchecksums.txt,mirroring the BMM/GEMM Rubin pattern (same index body, distinct per-pin hashes).
tests/test_artifacts.py: extend the existing per-pin-hash-collision assertion to a shareddeep-gemm kernel name, so deep-gemm gets the same regression coverage the trtllm-gen pins have.
flashinfer/artifacts.py:get_checksums()now raises aRuntimeErrornaming the pin and theURL when the manifest download fails, instead of falling through to a bare
FileNotFoundErroron a local cache path. That failure mode is what made this read as a cache bug rather than
"a pin was added without a mock".
🔍 Related Issues
Regression from #4280. Observed on unrelated PR CI, e.g.
https://github.com/flashinfer-ai/flashinfer/actions/runs/30607427968/job/91082694368
🚀 Pull Request Checklist
✅ 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.) — verification in progress on a dev box; draft untiltests/test_artifacts.pyis confirmed green.Reviewer Notes
CPU-only change; no kernel or GPU behavior is touched. The
get_checksums()error-path change isthe only non-test edit — it converts a silent-download-failure into a diagnosable error and does
not alter the success path.
AI-assisted (Claude Code): diagnosis and patch.
Summary by CodeRabbit