Skip to content

[cpu][ci] Add CPU Attention Tests for Neon Backend#30347

Merged
bigPYJ1151 merged 1 commit intovllm-project:mainfrom
fadara01:add_neon_attn_tests
Dec 10, 2025
Merged

[cpu][ci] Add CPU Attention Tests for Neon Backend#30347
bigPYJ1151 merged 1 commit intovllm-project:mainfrom
fadara01:add_neon_attn_tests

Conversation

@fadara01
Copy link
Contributor

@fadara01 fadara01 commented Dec 9, 2025

Purpose

Add CPU Attention Tests for Neon Backend
Should really have been part of #29193 but I missed it

Test Plan

Arm CI which includes CPU attention tests.

Test Result


Essential Elements of an Effective PR Description Checklist
  • [Y] The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • [Y] The test plan, such as providing test command.
  • [Y] 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.

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

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 adds CPU attention tests for the Neon backend, which is a good addition for ensuring correctness on ARM platforms. The changes include a new test case specifically for Neon and modifications to existing tests to dynamically select the appropriate instruction set architecture (ISA).

My review identified two critical issues in the implementation of these tests. First, a new helper function get_attn_isa contains a latent bug that would cause a TypeError if called with an argument. Second, the new Neon-specific test has an incorrect skipif condition, causing it to be skipped on the intended ARM hardware and run on other platforms where it is not applicable. I've provided suggestions to fix both issues to ensure the tests function as intended.

@fadara01 fadara01 force-pushed the add_neon_attn_tests branch 2 times, most recently from 86c9591 to 39cf2ab Compare December 9, 2025 15:31
@fadara01
Copy link
Contributor Author

fadara01 commented Dec 9, 2025

@fadara01
Copy link
Contributor Author

fadara01 commented Dec 9, 2025

@mgoin / @bigPYJ1151 could you guys have a look?

@mergify
Copy link

mergify bot commented Dec 9, 2025

Hi @fadara01, 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 or markdownlint failing?
mypy and markdownlint are run differently in CI. If the failure is related to either of these checks, please use the following commands to run them locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10
# For markdownlint
pre-commit run --hook-stage manual markdownlint

Should really have been part of vllm-project#29193 but I missed it

Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
@fadara01 fadara01 force-pushed the add_neon_attn_tests branch from 39cf2ab to 5ff4735 Compare December 9, 2025 15:57
@bigPYJ1151 bigPYJ1151 enabled auto-merge (squash) December 10, 2025 03:23
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 10, 2025
@bigPYJ1151 bigPYJ1151 merged commit 434ac76 into vllm-project:main Dec 10, 2025
20 checks passed
Majid-Taheri pushed a commit to Majid-Taheri/vllm that referenced this pull request Dec 23, 2025
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
dsuhinin pushed a commit to dsuhinin/vllm that referenced this pull request Jan 21, 2026
Signed-off-by: Fadi Arafeh <fadi.arafeh@arm.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
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