Skip to content

[do not merge] fmhav2 hang#2813

Open
jimmyzho wants to merge 2 commits intoflashinfer-ai:mainfrom
jimmyzho:fmhav2-fix
Open

[do not merge] fmhav2 hang#2813
jimmyzho wants to merge 2 commits intoflashinfer-ai:mainfrom
jimmyzho:fmhav2-fix

Conversation

@jimmyzho
Copy link
Contributor

@jimmyzho jimmyzho commented Mar 18, 2026

📌 Description

🔍 Related Issues

🚀 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.

🧪 Tests

  • Tests have been added or updated as needed.
  • All tests are passing (unittest, etc.).

Reviewer Notes

Summary by CodeRabbit

  • Tests
    • Re-enabled previously skipped tests to improve coverage and validation of attention logic.
    • Adjusted a skip condition for certain numeric-format combinations so fewer cases are excluded, expanding tested dtype/output-dtype permutations.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request re-enables a test for the fmha_v2_prefill functionality that was previously marked to be skipped due to a known hanging issue. The change suggests that an underlying fix for the hang has been addressed, and this PR aims to allow the test to run and verify the resolution.

Highlights

  • Test Re-enablement: The fmha_v2_prefill test, previously skipped due to a reported hang, has been re-enabled by uncommenting its pytestmark.skip decorator.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1758226e-934c-4afd-8354-f3a8b8b1ac41

📥 Commits

Reviewing files that changed from the base of the PR and between f6835fd and 859505b.

📒 Files selected for processing (1)
  • tests/attention/test_fmha_v2_prefill.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/attention/test_fmha_v2_prefill.py

📝 Walkthrough

Walkthrough

Module-level pytestmark skip in tests/attention/test_fmha_v2_prefill.py was commented out so the file no longer is globally skipped. Inside test_trtllm_fmha_v2_prefill, the fp8 skip condition was broadened to skip whenever dtype == torch.float8_e4m3fn (independent of o_dtype).

Changes

Cohort / File(s) Summary
Test file — skip marker & fp8 logic
tests/attention/test_fmha_v2_prefill.py
Module-level pytestmark skip was commented out (file no longer globally skipped). In test_trtllm_fmha_v2_prefill, the fp8 skip condition changed from dtype == o_dtype == torch.float8_e4m3fn to dtype == torch.float8_e4m3fn (now ignores o_dtype).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • bkryu
  • nvmbreughe
  • yzh119
  • Anerudhan

Poem

🐰 I nudged a skip and set it free,
Tests hop out, come watch and see,
FP8 rules tightened, a tiny tweak,
CI hums softly, answers we seek. 🎈

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely a blank template with no actual content filled in; all sections contain only placeholder text or unchecked boxes. Complete the Description section explaining the hang issue and why the fp8 skip condition was modified, and fill in the Related Issues section with relevant issue links.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is marked '[do not merge]' and vaguely references 'fmhav2 hang' without clearly summarizing the actual changes made to the codebase. Replace with a descriptive title like 'Enable fmha_v2 prefill tests by refining fp8 skip condition' that accurately reflects the substantive changes without the '[do not merge]' marker.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jimmyzho jimmyzho changed the title draft: fix fmhav2 hang [do not merge] fmhav2 hang Mar 18, 2026
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request addresses a fmhav2 hang by re-enabling a previously skipped test. The change involves commenting out the pytest.mark.skip decorator. My feedback focuses on cleaning up the now-obsolete todo comment associated with the skipped test, ensuring clarity and consistency in the codebase.

Comment on lines +6 to +8
# pytestmark = pytest.mark.skip(
# reason="todo(jimmyzho): temporarily skip this test due to hangs"
# )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The pytest.mark.skip decorator was commented out, suggesting the fmhav2 hang is resolved and this test should now pass. If this is the case, the todo comment stating 'temporarily skip this test due to hangs' is obsolete and should be removed to prevent future confusion. Consider removing the entire commented block if the test is intended to run permanently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant