Skip to content

Conversation

@wangxiyuan
Copy link
Collaborator

@wangxiyuan wangxiyuan commented Dec 2, 2025

Drop ascend scheduler from test

@github-actions
Copy link

github-actions bot commented Dec 2, 2025

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

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 removes all tests related to the AscendScheduler, including the entire E2E test file and several unit tests. However, the implementation code for the AscendScheduler and its configuration (AscendSchedulerConfig) still exists in the codebase (e.g., in vllm_ascend/ascend_config.py, vllm_ascend/platform.py, and vllm_ascend/core/schedule_config.py). Leaving this feature code without any tests is a critical issue, as it becomes dead code that is hard to maintain and can lead to silent breakages. If the AscendScheduler is being deprecated, its implementation should be removed along with the tests. If it is not being deprecated, the tests should be restored or replaced.

@wangxiyuan wangxiyuan force-pushed the drop_ascend_scheduler_test branch 2 times, most recently from 3e1d5d5 to 5cd62f5 Compare December 2, 2025 02:12
@MengqingCao MengqingCao added ready read for review ready-for-test start test by label for PR labels Dec 2, 2025
@MengqingCao
Copy link
Collaborator

LGTM if all ci pass

@wangxiyuan wangxiyuan force-pushed the drop_ascend_scheduler_test branch from 5cd62f5 to 94a3c82 Compare December 2, 2025 03:03
@wangxiyuan
Copy link
Collaborator Author

@MengqingCao No need to run e2e IMO

@wangxiyuan wangxiyuan merged commit 400af66 into vllm-project:main Dec 2, 2025
17 of 19 checks passed
ChenCangtao pushed a commit to ChenCangtao/vllm-ascend that referenced this pull request Dec 3, 2025
Drop ascend scheduler from test

- vLLM version: v0.11.2

Signed-off-by: wangxiyuan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:tests ready read for review ready-for-test start test by label for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants