Skip to content

[bnb] Skip moe + bnb test#36896

Merged
hmellor merged 3 commits intovllm-project:mainfrom
SunMarc:fix-moe-bnb-test
Mar 12, 2026
Merged

[bnb] Skip moe + bnb test#36896
hmellor merged 3 commits intovllm-project:mainfrom
SunMarc:fix-moe-bnb-test

Conversation

@SunMarc
Copy link
Copy Markdown
Contributor

@SunMarc SunMarc commented Mar 12, 2026

What does this PR do ?

This PR skips the test_4bit_bnb_moe_model test because in v5, the MoE implementation changed to 3D params and our bnb integration don't support quantizing 3D params yet. So, this is might be why it is failing since the two models are not the same. Note that on H100, the test actually pass but on the CI, it's always failing.

cc @hmellor

Copy link
Copy Markdown
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 skips a failing test related to MoE models with bitsandbytes quantization, which is a reasonable temporary measure. My review includes one suggestion to add a tracking issue to the skip reason. This is an important practice to ensure the test is re-enabled once the underlying issue is resolved, thereby maintaining long-term test coverage and code health.

@@ -138,6 +138,7 @@ def test_load_pp_4bit_bnb_model(model_name, description) -> None:
compare_two_settings(model_name, common_args, pp_args)


@pytest.mark.skip(reason="Need to add support for quantizing MoE experts with bnb in transformers")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

To ensure this skipped test is eventually re-enabled, it's crucial to track the underlying issue. Please create a GitHub issue for the lack of support for quantizing MoE experts with bnb and reference it in the skip reason. This prevents the test skip from becoming permanent and forgotten, which would create a lasting gap in test coverage.

Suggested change
@pytest.mark.skip(reason="Need to add support for quantizing MoE experts with bnb in transformers")
@pytest.mark.skip(reason="BNB integration does not support quantizing MoE with 3D params yet. See issue #XYZ.")

@github-actions
Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors.

You ask your reviewers to trigger select CI tests on top of fastcheck CI.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

🚀

@hmellor hmellor enabled auto-merge (squash) March 12, 2026 16:11
SunMarc added 2 commits March 12, 2026 16:19
Signed-off-by: Marc Sun <marc@huggingface.co>
Signed-off-by: Marc Sun <marc@huggingface.co>
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 12, 2026

Hi @SunMarc, the pre-commit checks have failed. Please run:

uv pip install pre-commit
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

Tip

Is mypy failing?
mypy is run differently in CI. If the failure is related to this check, please use the following command to run it locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10

Signed-off-by: Marc Sun <marc@huggingface.co>
auto-merge was automatically disabled March 12, 2026 16:44

Head branch was pushed to by a user without write access

@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 12, 2026
@hmellor hmellor enabled auto-merge (squash) March 12, 2026 17:05
@hmellor hmellor merged commit c973ecd into vllm-project:main Mar 12, 2026
14 checks passed
Lucaskabela pushed a commit to Lucaskabela/vllm that referenced this pull request Mar 17, 2026
Signed-off-by: Marc Sun <marc@huggingface.co>
wendyliu235 pushed a commit to wendyliu235/vllm-public that referenced this pull request Mar 18, 2026
Signed-off-by: Marc Sun <marc@huggingface.co>
khairulkabir1661 pushed a commit to khairulkabir1661/vllm that referenced this pull request Mar 27, 2026
Signed-off-by: Marc Sun <marc@huggingface.co>
mtparet pushed a commit to blackfuel-ai/vllm that referenced this pull request Apr 9, 2026
Signed-off-by: Marc Sun <marc@huggingface.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants