-
Notifications
You must be signed in to change notification settings - Fork 515
[Doc]Add developer guide of eplb. #3759
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: offline0806 <[email protected]>
Signed-off-by: offline0806 <[email protected]>
|
👋 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 introduces a new developer guide for the Expert Parallelism Load Balancer (EPLB). While adding documentation is a valuable contribution, the current version of the guide contains several significant errors that could mislead developers. I have identified issues such as references to non-existent parameters, incorrect file paths, incorrect language-specific terminology (using try-catch for Python), and a typo in a critical environment variable. Correcting these is important for the documentation to be accurate and useful. My review provides specific suggestions for each of these points.
| In other cases, we use the global load balancing policy, which replicates experts globally regardless of expert groups, and packs the replicated experts onto individual GPUs. This policy can be adopted in the decoding stage with a larger expert-parallel size. | ||
|
|
||
| ### Add a New MoE Model | ||
| When adding a new model, inherit or modify `VllmEplbAdaptor`. Add the processing logic for `num_dense_layers`, `global_expert_num`, and `num_roe_layers`, and synchronize the relevant logic within the `model_register` function. |
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 documentation mentions num_roe_layers as a parameter to handle when adding a new model. However, this parameter does not appear to be used in the related implementation files (vllm_ascend/eplb/adaptor/vllm_adaptor.py, vllm_ascend/eplb/utils.py). This is misleading for developers. Please remove it or clarify its purpose.
| When adding a new model, inherit or modify `VllmEplbAdaptor`. Add the processing logic for `num_dense_layers`, `global_expert_num`, and `num_roe_layers`, and synchronize the relevant logic within the `model_register` function. | |
| When adding a new model, inherit or modify `VllmEplbAdaptor`. Add the processing logic for `num_dense_layers` and `global_expert_num`, and synchronize the relevant logic within the `model_register` function. |
|
|
||
| ### Add a New MoE Model | ||
| When adding a new model, inherit or modify `VllmEplbAdaptor`. Add the processing logic for `num_dense_layers`, `global_expert_num`, and `num_roe_layers`, and synchronize the relevant logic within the `model_register` function. | ||
| If you want to add MoE-related processing to the model, add corresponding methods to `VLLM/EPLB/utils` and add patch logic in the `model_register` function. |
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 file path VLLM/EPLB/utils is incorrect. Based on the project structure, the correct path appears to be vllm_ascend/eplb/utils.py. Providing an incorrect path can mislead developers trying to extend the functionality.
| If you want to add MoE-related processing to the model, add corresponding methods to `VLLM/EPLB/utils` and add patch logic in the `model_register` function. | |
| If you want to add MoE-related processing to the model, add corresponding methods to `vllm_ascend/eplb/utils.py` and add patch logic in the `model_register` function. |
| All EPLB parameters must be initialized by default during initialization, with specified parameter types and default values for proper handling. | ||
|
|
||
| #### General Functions | ||
| All method arguments must specify parameter types and default values, and functions must include default return value handling for default arguments. It is recommended to use `try-catch` blocks to handle the function body, specifying the type of exception captured and the failure handling (e.g., logging exceptions or returning a failure status). |
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 documentation recommends using try-catch blocks for exception handling. However, in Python, the correct syntax is try-except. This should be corrected to avoid confusion for Python developers.
| All method arguments must specify parameter types and default values, and functions must include default return value handling for default arguments. It is recommended to use `try-catch` blocks to handle the function body, specifying the type of exception captured and the failure handling (e.g., logging exceptions or returning a failure status). | |
| All method arguments must specify parameter types and default values, and functions must include default return value handling for default arguments. It is recommended to use `try-except` blocks to handle the function body, specifying the type of exception captured and the failure handling (e.g., logging exceptions or returning a failure status). |
|
|
||
| ## Limitation | ||
| Before using EPLB, start the script and add `export DYNAMIC_EPLB="true"`. | ||
| Before performing load data collection (or performance data collection), start the script and add `export EXPORT_MAP_RECORD="true"`. |
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.
There is a typo in the environment variable name. The documentation specifies EXPORT_MAP_RECORD, but the code in vllm_ascend/eplb/core/eplb_utils.py checks for EXPERT_MAP_RECORD. A developer following the documentation would encounter an error. This should be corrected.
| Before performing load data collection (or performance data collection), start the script and add `export EXPORT_MAP_RECORD="true"`. | |
| Before performing load data collection (or performance data collection), start the script and add `export EXPERT_MAP_RECORD="true"`. |
Signed-off-by: offline0806 <[email protected]>
| # Expert Parallelism Load Balancer (EPLB) | ||
|
|
||
| ## Why We Need EPLB? | ||
| When using Expert Parallelism (EP), different experts are assigned to different GPUs/NPUs. Given that the load of various experts may vary depending on the current workload, it is crucial to maintain balanced loads across different GPUs/NPUs. We adopt a redundant experts strategy by duplicating heavily-loaded experts. Then, we heuristically pack these duplicated experts onto GPUs to ensure load balancing across them. Moreover, thanks to the group-limited expert routing used in MoE models, we also attempt to place experts of the same group on the same node to reduce inter-node data traffic, whenever possible. |
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.
change all GPU to NPU
| ## Why We Need EPLB? | ||
| When using Expert Parallelism (EP), different experts are assigned to different GPUs/NPUs. Given that the load of various experts may vary depending on the current workload, it is crucial to maintain balanced loads across different GPUs/NPUs. We adopt a redundant experts strategy by duplicating heavily-loaded experts. Then, we heuristically pack these duplicated experts onto GPUs to ensure load balancing across them. Moreover, thanks to the group-limited expert routing used in MoE models, we also attempt to place experts of the same group on the same node to reduce inter-node data traffic, whenever possible. | ||
|
|
||
| To facilitate reproduction and deployment, we open-source our deployed EP load balancing algorithm in `vllm_ascend/eplb/core/policy`. The algorithm computes a balanced expert replication and placement plan based on the estimated expert loads. Note that the exact method for predicting expert loads is outside the scope of this repository. A common method is to use a moving average of historical statistics. |
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.
no need to mention open-source. It can be somthing like: vLLM Ascend supported xxx
| Please refer to the EPLB section of the user guide for detailed information: [How to Use EPLB](../../user_guide/feature_guide/eplb_swift_balancer.md) | ||
|
|
||
| ## How It Works? | ||
|
|
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.
please add more about the module design. For example, what's EplbUpdator, EplbWorker, etc, and how they work?
| ## How It Works? | ||
|
|
||
| ### Default Algorithm | ||
| #### Hierarchical Load Balancing |
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.
please add a section to descrbie how to register a new algorithm for developers
What this PR does / why we need it?
Add developer guide of eplb
Does this PR introduce any user-facing change?
How was this patch tested?