[None][fix] Do not treat a bare fmha_v2_cu directory as completed generation#16743
[None][fix] Do not treat a bare fmha_v2_cu directory as completed generation#16743brnguyen2 wants to merge 3 commits into
Conversation
WalkthroughThe wheel build script tracks FMHA CUDA generation with a ChangesFMHA generation tracking
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/build_wheel.py`:
- Around line 267-272: Rename fmha_generation_stamp to _fmha_generation_stamp,
annotate its fmha_v2_cu_dir parameter and return value with Path, and update all
call sites at the referenced locations to use the private helper name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c055ea9-658b-4781-a213-3e2122362c82
📒 Files selected for processing (1)
scripts/build_wheel.py
|
/bot run |
|
PR_Github #61027 [ run ] triggered by Bot. Commit: |
|
PR_Github #61027 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61062 [ run ] triggered by Bot. Commit: |
|
PR_Github #61062 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61070 [ run ] triggered by Bot. Commit: |
|
PR_Github #61070 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61090 [ run ] triggered by Bot. Commit: |
|
PR_Github #61090 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61292 [ run ] triggered by Bot. Commit: |
|
PR_Github #61292 [ run ] completed with state
|
…eration generate_fmha_cu creates the fmha_v2_cu output directory before running the (minutes-long) fmha_v2 kernel generation, but the build skips generation whenever that directory exists. An interrupted first generation (Ctrl-C, node failure, build timeout) therefore leaves an empty or partial directory that every subsequent non-clean build trusts: the launcher .cu sources are missing while the checked-in fmha_cubin.cpp table still references them, and the build fails much later at link with undefined run_fmha_v2_* symbols. Nothing recovers short of --clean, which is not an obvious remedy for that error. Write a .generation_complete stamp as the last step of generate_fmha_cu (removed again as its first step) and key the skip check on the stamp instead of the directory. Existing checkouts regenerate once to acquire the stamp; the content-compare copy keeps mtimes of unchanged files, so no recompilation results. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
…annotations Rename fmha_generation_stamp to _fmha_generation_stamp and annotate its parameter and return type with Path, per coding guidelines for private helpers and annotation coverage. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
c40ce12 to
7d048a4
Compare
|
/bot run |
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 `@scripts/build_wheel.py`:
- Around line 335-336: Validate generated_files after FMHA generation and before
_fmha_generation_stamp(fmha_v2_cu_dir).touch(): reject empty results and ensure
they contain the expected fmha_v2_cu/*_sm*.cu outputs. Only create the
completion stamp after validation succeeds, preserving regeneration when outputs
are missing or malformed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b371d625-6642-4c42-97be-ef4f44feffe1
📒 Files selected for processing (1)
scripts/build_wheel.py
|
PR_Github #61377 [ run ] triggered by Bot. Commit: |
…tamp Raise RuntimeError if no *_sm*.cu files were produced before touching the stamp, so a silent generation failure doesn't leave an apparently complete stamp that causes subsequent builds to skip regeneration. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
PR_Github #61377 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61391 [ run ] triggered by Bot. Commit: |
|
PR_Github #61391 [ run ] completed with state
|
|
/bot run |
1 similar comment
|
/bot run |
|
PR_Github #61407 [ run ] triggered by Bot. Commit: |
|
PR_Github #61409 [ run ] triggered by Bot. Commit: |
|
PR_Github/16743-b392900 #61407 was force-killed by a newer pipeline run. |
3 participants
@coderabbitai summary
Description
generate_fmha_cucreates thefmha_v2_cuoutput directory before the (minutes-long) kernel generation runs, but the build skips generation whenever that directory exists. An interrupted first generation (Ctrl-C, node failure, build timeout) leaves an empty/partial directory that every subsequent non-clean build trusts — launcher.cusources are missing, and the build fails later at link time with undefinedrun_fmha_v2_*symbols.Fixes this by writing a
.generation_completestamp as the last step ofgenerate_fmha_cu(removed again as its first step) and keying the skip check on the stamp instead of the directory. The stamp is only created after validating that at least one*_sm*.cufile was produced, so a silent failure in the generator won't leave a misleading stamp. Existing checkouts regenerate once to acquire the stamp; the content-compare copy keeps mtimes of unchanged files so no recompilation results.Test Coverage
No automated tests — this code path requires a full FMHA kernel generation run which takes several minutes and depends on CUDA toolchain setup not present in CI. Manual verification:
fmha_v2_cugeneration mid-run, then rebuild without--clean— confirm generation restarts rather than skipping.PR Checklist