Skip to content

Conversation

@ZJY0516
Copy link
Contributor

@ZJY0516 ZJY0516 commented Oct 5, 2025

Purpose

test_causal_conv1d.py::test_causal_conv1d_update was broken due to #25752

Test Plan

pytest -v -s tests/kernels/mamba/test_causal_conv1d.py::test_causal_conv1d_update

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.

Signed-off-by: zjy0516 <[email protected]>
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 addresses a broken test, test_causal_conv1d_update, by adding the conv_state_indices parameter to the causal_conv1d_update function call. The fix is straightforward and correct, providing the necessary indices for the convolution state. The change is well-contained within the test file and appears to resolve the issue as described. I have no further comments or suggestions.

@mergify
Copy link

mergify bot commented Oct 5, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @ZJY0516.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 5, 2025
@mergify mergify bot removed the needs-rebase label Oct 5, 2025
@hmellor hmellor requested a review from tdoublep October 5, 2025 16:23
Copy link
Member

@tdoublep tdoublep left a comment

Choose a reason for hiding this comment

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

Looks good but can we also address why this test is not running in CI when we change code that breaks it?

@ZJY0516
Copy link
Contributor Author

ZJY0516 commented Oct 5, 2025

I'm also curious about it. However, I'm not very familiar with our CI setup.

@hmellor
Copy link
Member

hmellor commented Oct 5, 2025

The test wouldn't have run because no files in source_file_dependencies were changed in the original PR

- label: Kernels Mamba Test # 31min
timeout_in_minutes: 45
mirror_hardwares: [amdexperimental]
source_file_dependencies:
- csrc/mamba/
- tests/kernels/mamba
commands:
- pytest -v -s kernels/mamba

You could add vllm/model_executor/layers/mamba to the list for example

@ZJY0516
Copy link
Contributor Author

ZJY0516 commented Oct 5, 2025

The test wouldn't have run because no files in source_file_dependencies were changed in the original PR

- label: Kernels Mamba Test # 31min
timeout_in_minutes: 45
mirror_hardwares: [amdexperimental]
source_file_dependencies:
- csrc/mamba/
- tests/kernels/mamba
commands:
- pytest -v -s kernels/mamba

You could add vllm/model_executor/layers/mamba to the list for example

Thanks very much!

@hmellor
Copy link
Member

hmellor commented Oct 5, 2025

Not sure if that is the correct file to trigger the Mamba kernel CI though, @tdoublep could you confirm which file from the original PR should have triggered this test?

@mergify mergify bot added the ci/build label Oct 5, 2025
@ZJY0516 ZJY0516 requested a review from tdoublep October 5, 2025 16:56
@tdoublep
Copy link
Member

tdoublep commented Oct 5, 2025

Not sure if that is the correct file to trigger the Mamba kernel CI though, @tdoublep could you confirm which file from the original PR should have triggered this test?

I think we should add vllm/model_executor/layers/mamba/ops which contains the kernel code

Signed-off-by: zjy0516 <[email protected]>
@ZJY0516
Copy link
Contributor Author

ZJY0516 commented Oct 5, 2025

Not sure if that is the correct file to trigger the Mamba kernel CI though, @tdoublep could you confirm which file from the original PR should have triggered this test?

I think we should add vllm/model_executor/layers/mamba/ops which contains the kernel code

I have updated it.

@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 5, 2025
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) October 5, 2025 17:14
Copy link
Member

@tdoublep tdoublep left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@DarkLight1337 DarkLight1337 merged commit 9c3c21c into vllm-project:main Oct 5, 2025
20 checks passed
tomeras91 pushed a commit to tomeras91/vllm that referenced this pull request Oct 6, 2025
@ZJY0516 ZJY0516 deleted the fix-mamba-kernel-test branch October 6, 2025 02:43
karan pushed a commit to karan/vllm that referenced this pull request Oct 6, 2025
southfreebird pushed a commit to southfreebird/vllm that referenced this pull request Oct 7, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build 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.

4 participants