Skip to content

Conversation

MatthewBonanni
Copy link
Contributor

@MatthewBonanni MatthewBonanni commented Sep 2, 2025

Purpose

Quick follow on to #23959 to disable test_silu_nvfp4_quant_fusion and test_silu_mul_quant_fusion in buildkite, fixing error 5 (which arises since no tests from this file are run)

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@mergify mergify bot added the ci/build label Sep 2, 2025
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

This pull request disables the test_silu_nvfp4_quant_fusion test to fix a CI failure. While this resolves the immediate issue by commenting out the test, it also removes test coverage for this feature on platforms where it should be running. I've suggested an alternative approach that fixes the CI failure without disabling the test, ensuring test coverage is maintained.

- pytest -v -s tests/kernels/quantization/test_cutlass_scaled_mm.py -k 'fp8'
- pytest -v -s tests/kernels/quantization/test_nvfp4_quant.py
- pytest -v -s tests/kernels/quantization/test_silu_nvfp4_quant_fusion.py
# - pytest -v -s tests/kernels/quantization/test_silu_nvfp4_quant_fusion.py
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Commenting out this test run disables it for all platforms, including those where it is intended to run. This results in a loss of test coverage for the silu_nvfp4_quant_fusion feature.

A better approach is to allow pytest to succeed even when no tests are collected. If the project uses pytest version 7.0 or newer, you can add the --allow-no-tests flag. This will cause pytest to exit with code 0 (success) instead of 5 when no tests are found, resolving the CI issue without sacrificing test coverage on supported platforms.

If using an older version of pytest, other options could be considered, such as wrapping the command in shell logic to ignore exit code 5.

    - pytest -v -s --allow-no-tests tests/kernels/quantization/test_silu_nvfp4_quant_fusion.py

Copy link
Member

Choose a reason for hiding this comment

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

@MatthewBonanni that sounds like a better option, could we add --allow-no-tests with a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@njhill pytest doesn't recognize --allow-no-tests as an option (I'm on 8.4.1) and I can't find it documented online anywhere. I think gemini may have hallucinated that. There are definitely other workarounds though, like I could add a dummy test that passes or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@njhill note: needed to disable a second test as well (see #23671)

Copy link
Member

Choose a reason for hiding this comment

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

nice gemini hallucination

@MatthewBonanni MatthewBonanni changed the title [CI/Build] Disable test_silu_nvfp4_quant_fusion [CI/Build] Disable SiluMul NVFP4 quant fusion tests Sep 2, 2025
@simon-mo simon-mo merged commit 2fd1a40 into vllm-project:main Sep 2, 2025
12 checks passed
@njhill
Copy link
Member

njhill commented Sep 2, 2025

Thanks @MatthewBonanni. What is the plan for uncommenting these then? Just don't want it to get forgotten...

@MatthewBonanni
Copy link
Contributor Author

@njhill Thanks for the review! Yes, I'm following the permanent fix #23727 and will make sure this get reverted as part of that PR

@MatthewBonanni MatthewBonanni deleted the disable_test_silu_nvfp4_quant_fusion branch September 3, 2025 00:43
@MatthewBonanni
Copy link
Contributor Author

@njhill update: this has been reverted in that PR, thanks @elvischenv!

845473182 pushed a commit to 845473182/vllm that referenced this pull request Sep 3, 2025
* 'main' of https://github.com/845473182/vllm: (457 commits)
  [BugFix] Fix routed_scaling_factor double mul for dots1 and glm4 MoE models (vllm-project#24132)
  [Misc] Add check for dual_chunk_attention (vllm-project#24070)
  [Doc]: fix typos in Python comments (vllm-project#24115)
  [Doc]: fix typos in Python comments (vllm-project#24093)
  [Compile] Fix Compile Warning for `w4a8_mm_entry.cu` (vllm-project#23660)
  fix some typos (vllm-project#24071)
  [V1] Wrapper which plumbs request-level logits processors into vLLM batch-level logits processing (vllm-project#23656)
  Upgrade xgrammar to 0.1.23 (vllm-project#22988)
  Update release pipeline post PyTorch 2.8.0 update (vllm-project#24073)
  [XPU] Fix the bug of LoRA logits on the XPU platform (vllm-project#24081)
  [CI/Build] Disable SiluMul NVFP4 quant fusion tests (vllm-project#24121)
  [Bug] R1 Accuracy: Fix `routed_scaling_factor` Double Mul Issue (vllm-project#24119)
  [AMD][Kernel][Bugfix] Cast offsets tensor bn to tl.int64 to avoid GPU segfault (vllm-project#23692)
  [CI] Enable all hf transformers baselines in test_hybrid (vllm-project#23936)
  [Log] Only Print Profiler Results on Rank 0 (vllm-project#23370)
  Fix weights loading for Apertus (vllm-project#24100)
  [Metrics] Deprecate TPOT in favor of ITL (vllm-project#24110)
  [Bugfix] Fix packed_factor missing attribute error (vllm-project#23902)
  Run ruff format on a few files. (vllm-project#24075)
  [Bugfix] Fix transform_config parsing in Compressed Tensors (vllm-project#23945)
  ...
akaihaoshuai pushed a commit to akaihaoshuai/vllm that referenced this pull request Sep 3, 2025
elvischenv added a commit to elvischenv/vllm that referenced this pull request Sep 3, 2025
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
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.

3 participants