Conversation
PR flashinfer-ai#3468 moved the trtllmGen_bmm_export symlink from FLASHINFER_CUBIN_DIR to FLASHINFER_GEN_SRC_DIR without updating the race-condition test, which still redirected only FLASHINFER_CUBIN_DIR and asserted the symlink under it: AssertionError: Expected /tmp/.../flashinfer/trtllm/batched_gemm/ trtllmGen_bmm_export to be a symlink Because FLASHINFER_GEN_SRC_DIR was never redirected, the symlink was also created in the real workspace directory and pointed into the test's temp directory, which is deleted on teardown. The between-iteration unlink() targeted the temp path too, so it never removed the symlink actually in use and the race stopped being re-triggered after the first iteration. Redirect FLASHINFER_GEN_SRC_DIR alongside FLASHINFER_CUBIN_DIR and assert the symlink there. The two are kept as separate temp subdirectories so the assertion still fails if the symlink moves back under FLASHINFER_CUBIN_DIR. Also correct the ensure_symlink() docstring, which still referred to the pre-flashinfer-ai#3468 CUBIN_DIR location.
|
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)
📝 WalkthroughWalkthroughThe change corrects the ChangesSymlink race test
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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 |
|
/bot run tests/utils |
….6.16rc2 (#4197) Cherry-picks for the `release-v0.6.16` branch, plus the version bump to `0.6.16rc2`. ## Commits | Commit | Source | Status | | --- | --- | --- | | `fix(norm): convert float2 to e4m3 directly in packed cast` | #4167 (`e683e307` on `main`) | merged upstream | | `feat(xqa): ragged Q and per-row sliding-window masking for speculative decode` | #4137 (`60783fb9` on `main`) | merged upstream | | `test(jit): assert BMM export symlink under GEN_SRC_DIR` | #4187 (`417bbd29` on `main`) | merged upstream | | `feat(mla): support packed low-head and variable-Q decode` | #4178 (PR head `0774943c`) | **not yet merged** to `main` | | `bump version to 0.6.16rc2` | — | — | Applied in `main` merge order; #4187 sits directly on top of #4137 upstream, so that ordering is preserved. ## Notes - **#4189 was not cherry-picked** — it was merged directly into `release-v0.6.16` (`34368112`) and is already the base of this branch. - **#4178 is still open against `main`.** It was cherry-picked at the request of the release owner. Its two head commits (`b6cc59491`, `0774943c2`) are squashed into one commit here, mirroring how it will land upstream. If the PR changes before merge, this commit should be dropped and re-picked. - The three merged cherry-picks apply cleanly with patch-ids identical to their sources. The #4178 squash is byte-identical to the PR's combined diff. - The #4178 commit was committed with `--no-verify`: the `mypy` pre-commit hook reports two pre-existing `[no-redef]` errors in `flashinfer/mla/_core.py` (conditional `if/else` imports of `_check_can_implement` / `_get_split_kv_and_workspace_size`). These reproduce identically on PR #4178's own branch and on the `release-v0.6.16` base — they are not introduced by this cherry-pick, and surface only when those modules are checked together. Left unfixed here to keep the cherry-pick faithful. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Lee Yongjun <jqueen.astro@gmail.com> Co-authored-by: yichengj <yichengj@nvidia.com> Co-authored-by: bryu <bryu@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Ka-Hyun Nam <knam@nvidia.com> Co-authored-by: mingyangw <mingyangw@nvidia.com>
…4225) ## Summary Fixes two test regressions introduced by #4191 (release-only, not on `main`): - **#4208** — `test_make_tuning_config_reuses_topk_ids_initializer` called `get_trtllm_moe_sm100_module.cache_clear()`, but #4191 moved `@functools.cache` to `_get_trtllm_moe_sm100_module_impl`. - **#4166 (re-open)** — #4187 updated the symlink test to assert under `GEN_SRC_DIR/flashinfer/...`, but #4191 moved the export symlink to `GEN_SRC_DIR/trtllm_export/fused_moe_trtllm_sm100/flashinfer/...`. Both failures appeared on all Blackwell jobs in Test Cycle 2 (pipeline 59912149, rc3). ## Test plan - [x] `pytest tests/autotuner/test_autotuner_core.py::test_make_tuning_config_reuses_topk_ids_initializer` — pass - [x] `pytest tests/utils/test_gen_module_symlink_race_condition.py` — pass - [ ] B300/GB200 CI rerun to confirm cycle-2 green on these two files
📌 Description
Fixes #4166.
tests/utils/test_gen_module_symlink_race_condition.pyfails onrelease-v0.6.16rc1 (unit_test_b300, cu129/cu130):Root cause
#3468 ("move JIT symlinks to writable gen dir",
b9890c85) moved thetrtllmGen_bmm_exportsymlink fromFLASHINFER_CUBIN_DIRtoFLASHINFER_GEN_SRC_DIRinflashinfer/jit/fused_moe.py,flashinfer/jit/moe_utils.py, andflashinfer/jit/gemm/core.py. That PR touched three production files and no tests.The test redirects only
FLASHINFER_CUBIN_DIR, then asserts the symlink under it, so it now checks a location the code no longer writes to.There is a second, quieter consequence. Since
FLASHINFER_GEN_SRC_DIRis never redirected, the symlink is created in the real workspace directory and points into the test's temp directory, whichshutil.rmtreedeletes on teardown. The between-iterationsymlink_path.unlink()also targeted the temp path, so it never removed the symlink actually in use — meaning the race stopped being re-triggered after the first iteration, and the test would have silently under-tested even with the assertion path corrected.Fix
FLASHINFER_GEN_SRC_DIRalongsideFLASHINFER_CUBIN_DIRin the worker, and assert the symlink there.tests/jit/test_jit_cpp_ext.pyalready redirectsFLASHINFER_GEN_SRC_DIRthis way, andflashinfer/aot.pyreassigns the same attribute, so this is the established pattern.FLASHINFER_CUBIN_DIR. Pointing both at one directory would make the assertion pass either way.ensure_symlink()docstring, which still cited the pre-fix: move JIT symlinks to writable gen dir #3468CUBIN_DIRlocation.No production behavior changes; the only non-test edit is a docstring.
🔍 Related Issues
🚀 Pull Request Checklist
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used my preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.All hooks pass on the changed files (
ruff check,ruff format,mypy, whitespace/EOL).🧪 Tests
The test requires SM100/SM12x and returns early elsewhere, so it cannot be executed end-to-end on the SM89/90 host available to me. It still needs a B300/GB200 run to confirm the CI failure is cleared.
To verify the path logic without that hardware, I exercised the real
gen_trtllm_gen_fused_moe_sm100_module()with only artifact download/checksum verification stubbed (the symlink is created before the nvcc-arch check raises), and recorded theensure_symlink()link path. A stand-in directory represents the shared workspace dir so nothing touches the real cache.Redirecting
FLASHINFER_CUBIN_DIRonly, as the test did before:Redirecting both, as this PR does:
Collection and the non-SM100 early-return path were also confirmed green:
pytest -q tests/utils/test_gen_module_symlink_race_condition.py→1 passed.Reviewer Notes
gen_fused_moe_worker_processnow takes a(cubin_dir, gen_src_dir)tuple, sincePool.mappasses a single argument.flashinfer/jit/moe_utils.pyandflashinfer/jit/gemm/core.pyhave the same symlink layout but no test asserts their paths, so nothing else needed updating.Summary by CodeRabbit
Documentation
Tests