-
Notifications
You must be signed in to change notification settings - Fork 794
[Refactor]Refactor of vllm_ascend/distributed module #5719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this 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 is a large-scale refactoring of the vllm_ascend/distributed module, moving files into a more organized directory structure and updating import paths accordingly. While this is a positive change for code organization, I've identified several incorrect import paths that will cause ImportError exceptions at runtime. These are critical issues that need to be fixed. I've provided specific comments and suggestions for each case.
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import ( # noqa: E402 | ||
| _convert_to_bytes, _parse_global_segment_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import for _convert_to_bytes and _parse_global_segment_size is incorrect. They are being imported from the backend package, but the __init__.py for that package is empty, which will cause an ImportError. Please either update the __init__.py to expose these functions or change the import to be direct from their module.
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import ( # noqa: E402 | |
| _convert_to_bytes, _parse_global_segment_size) | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.mooncake_backend import ( # noqa: E402 | |
| _convert_to_bytes, _parse_global_segment_size) |
| from vllm.logger import logger | ||
|
|
||
| from vllm_ascend.distributed.kvpool.backend.backend import Backend | ||
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import Backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import for Backend is incorrect. It is being imported from the backend package, but the __init__.py for that package is empty, which will cause an ImportError. Please either update the __init__.py to expose this class or change the import to be direct from its module.
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import Backend | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.backend import Backend |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import Backend | ||
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.memcache_backend import \ | ||
| MemcacheBackend | ||
| from vllm_ascend.distributed.kvpool.backend.mooncake_backend import \ | ||
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import \ | ||
| MooncakeBackend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The imports for Backend and MooncakeBackend are incorrect. They are being imported from the backend package, but the __init__.py for that package is empty, which will cause an ImportError. Please either update the __init__.py to expose these classes or change the imports to be direct from their respective modules.
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import Backend | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.memcache_backend import \ | |
| MemcacheBackend | |
| from vllm_ascend.distributed.kvpool.backend.mooncake_backend import \ | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend import \ | |
| MooncakeBackend | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.backend import Backend | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.memcache_backend import \ | |
| MemcacheBackend | |
| from vllm_ascend.distributed.kv_transfer.kv_pool.ascend_store.backend.mooncake_backend import \ | |
| MooncakeBackend |
|
|
||
| from vllm_ascend.ascend_config import get_ascend_config | ||
| from vllm_ascend.distributed.utils import fc3_all_gather_and_maybe_unpad_impl | ||
| from vllm_ascend.distributed.kv_transfer.utils.utils import fc3_all_gather_and_maybe_unpad_impl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path for fc3_all_gather_and_maybe_unpad_impl is incorrect. This function was not moved to vllm_ascend.distributed.kv_transfer.utils.utils as part of the refactoring; it remains in vllm_ascend.distributed.utils. This change will cause an ImportError.
| from vllm_ascend.distributed.kv_transfer.utils.utils import fc3_all_gather_and_maybe_unpad_impl | |
| from vllm_ascend.distributed.utils import fc3_all_gather_and_maybe_unpad_impl |
|
The RFC has reached an agreement. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
please fix the merge conflict. |
Signed-off-by: lty <[email protected]>
Signed-off-by: lty <[email protected]>
Signed-off-by: lty <[email protected]>
|
organized by function is ok to me, howerver, cpu_offload_manager in distributed directory is relied by cpu_offload_connector, maybe cpu_offload_manager should be moved into cpu_offload dir after this refactor? |
my bad, moving cpu_offload_manager under cpu_offload dir is exactly what we planed to do, but I missed this part when drawing the table in the RFC, we'll fix this shortly. |
Signed-off-by: lty <[email protected]>
Signed-off-by: lty <[email protected]>
### What this PR does / why we need it? Based on the RFC:vllm-project#5604 This PR is a refactoring of vllm_ascend/distributed, moving all kv_transfer realtaed codes into a dedicated folder, which has already been done in vLLM ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? - vLLM version: v0.13.0 - vLLM main: vllm-project/vllm@2f4e654 --------- Signed-off-by: lty <[email protected]>
|
LGTM |
### What this PR does / why we need it? Based on the RFC:vllm-project#5604 This PR is a refactoring of vllm_ascend/distributed, moving all kv_transfer realtaed codes into a dedicated folder, which has already been done in vLLM ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? - vLLM version: v0.13.0 - vLLM main: vllm-project/vllm@2f4e654 --------- Signed-off-by: lty <[email protected]>
Based on the RFC:vllm-project#5604 This PR is a refactoring of vllm_ascend/distributed, moving all kv_transfer realtaed codes into a dedicated folder, which has already been done in vLLM NA - vLLM version: v0.13.0 - vLLM main: vllm-project/vllm@2f4e654 --------- Signed-off-by: lty <[email protected]>
What this PR does / why we need it?
Based on the RFC:#5604
This PR is a refactoring of vllm_ascend/distributed, moving all kv_transfer realtaed codes into a dedicated folder, which has already been done in vLLM
Does this PR introduce any user-facing change?
NA
How was this patch tested?