Conversation
|
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: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe GDN BF16 decode module now detects SM properties at runtime for the target device. MTP tile selection and packed-FMA dispatch are device-aware. Subprocess tests verify imports succeed without visible CUDA devices. ChangesGDN device-aware dispatch
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to CUDA device detection is deferred until decode kernels run, allowing flashinfer and GDN imports to succeed when no CUDA device is visible. The CPU-only import regression coverage passes, with no concrete current-head merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Checking in after a week with no review yet. Happy to split the fix further or add more test coverage if that would help; no rush. |
There was a problem hiding this comment.
There is a tests/gdn directory and we'd like to organize our test files.
However, what might be better is to simply put the tests in this file inside a suitable existing file because it covers only two cases.
|
Moved the two import-crash tests into |
|
/bot run tests/gdn |
|
[FAILED] Pipeline #60922613 — 16/18 executed test jobs passed Compared with nightly #60831563. Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 4/6 passed
No individual test or infrastructure failures could be extracted. |
f998a0d to
5347d71
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased past main to resolve a conflict: main added Re-verified in Docker (CPU-only, I don't have GPU hardware here to re-run the actual kernel numerics, so the internal CI pipeline is the source of truth for that (it previously reported 16/18, all unit tests green across every GPU/CUDA combination; the two |
…helpers module-scope torch.cuda.get_device_properties()/get_device_capability() calls (NUM_SMS, _GPU_MAJOR, _USE_PACKED_FMA), which flashinfer-ai#3293 had already removed for the exact same reason (flashinfer-ai#3262): importing flashinfer crashes on any machine without a visible CUDA device. Reapply flashinfer-ai#3293's fix: use the existing get_num_sm()/get_sm_version() helpers from flashinfer.cute_dsl.utils/fp4_common, threading device from the input tensor (q.device, asserted non-None before each use site) instead of querying the device at import time. Fixes flashinfer-ai#4143
Per review: move the two subprocess-based no-CUDA import checks out of a new file and into tests/gdn/test_decode_pretranspose_bf16_padded_pool.py, the existing file that already covers gdn_decode_bf16_state.py.
5347d71 to
88c7feb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased past main again to clear a second conflict in the same file: main's new |
|
[FAILED] Pipeline #60922613 — 16/18 executed test jobs passed No usable JUnit artifact was available; individual tests and nightly comparison could not be recovered. Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 4/6 passed
Failure detailsTimeouts, infrastructure, or incomplete jobs
|
1 similar comment
|
[FAILED] Pipeline #60922613 — 16/18 executed test jobs passed No usable JUnit artifact was available; individual tests and nightly comparison could not be recovered. Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 4/6 passed
Failure detailsTimeouts, infrastructure, or incomplete jobs
|
📌 Description
gdn_decode_bf16_state.pycomputedNUM_SMS/_GPU_MAJOR/_USE_PACKED_FMAvia eagertorch.cuda.get_device_properties(0)/get_device_capability(0)calls at module scope, soimport flashinfercrashed on any machine without a visible CUDA device.#3293 fixed this exact pattern for #3262 by moving the calls into lazy, device-parameterized
helpers (
get_num_sm/get_sm_version, already the convention in every othergdn_kernels/*.pyfile). #3502 added new code to the same file and reintroduced the eager module-scope pattern
without reusing that fix.
git log -Gon the two lines shows the full lifecycle: introduced(#2679) -> fixed (#3293) -> reintroduced (#3502).
This PR reapplies #3293's fix to the code #3502 added:
_select_tile_v_for_mtpand_get_bf16_mtp_confignow take adevicekeyword, threaded fromq.deviceat each of the threecall sites (all three functions
assert q is not Noneearlier in the same function body). Themodule-scope globals are removed.
🔍 Related Issues
Fixes #4143
🚀 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.).Reviewer Notes
Verified in a clean
python:3.12-slimcontainer (CPU-only torch,CUDA_VISIBLE_DEVICES=, noGPU hardware):
import flashinferandimport flashinfer.gdn_decodeboth crash on unpatchedmain(AssertionError: Torch not compiled with CUDA enabledat theNUM_SMSline) and bothsucceed after this change.
tests/test_gdn_decode_cpu_import.pypins this as a subprocess test(fails on main, passes on the branch, confirmed both ways) so a future addition to this file
can't reintroduce the eager pattern a third time.
I have not run this against an actual GPU: the three call sites already had
assert q is not Nonein scope before reaching the changed lines, andget_num_sm/get_sm_versionare thesame helpers already exercised by every sibling kernel file, but the packed-FMA / SM-count
dispatch itself is unverified on real hardware in this session.
Summary by CodeRabbit
Bug Fixes
Tests