[Bugfix]: fix pp errors when applying flashcomm1

[zxdukki]

What this PR does / why we need it?

Currently vllm-ascend cannot correctly calculate the size of intermediate tensors in preprocess when enabling PP + SP/flashcomm1 after merging #6191 .

We should replace is_residual_scattered_for_sp with enable_sp in sync_and_slice_intermediate_tensors.

Does this PR introduce any user-facing change?

No

How was this patch tested?

vllm: v0.14.0
vllm-ascend: main

[github-actions]

👋 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.

[gemini-code-assist]

Code Review

The pull request introduces a new method sync_and_slice_intermediate_tensors to correctly handle intermediate tensor synchronization and slicing, particularly when sequence parallelism (flashcomm1) is enabled. This addresses the bug where the intermediate tensor's copy length was incorrectly calculated, leading to errors in pipeline parallelism setups. The implementation correctly uses ceiling division for copy_len to ensure proper sharding across tensor parallel ranks.

[jianzs]

@lidenghui1110 PTAL

[lidenghui1110]

@lidenghui1110 PTAL

It looks like refactor in #6191 missed this part. Maybe we need a test case to cover pp+fc1 situation. And I have some questions here:

1. For we are trying to refactor modelrunner more like GPUModelRunner, shall we use the same logic as vLLM using _determine_batch_execution_and_padding?
2. In this PR only using sync_and_slice_intermediate_tensors in preprocess, can we use same logic in dummy_run using sync_and_slice_intermediate_tensors? The code is duplicated and in vLLM they all using sync_and_slice_intermediate_tensors

[zxdukki]

@lidenghui1110 PTAL

It looks like refactor in #6191 missed this part. Maybe we need a test case to cover pp+fc1 situation. And I have some questions here:

1. For we are trying to refactor modelrunner more like GPUModelRunner, shall we use the same logic as vLLM using _determine_batch_execution_and_padding?
2. In this PR only using sync_and_slice_intermediate_tensors in preprocess, can we use same logic in dummy_run using sync_and_slice_intermediate_tensors? The code is duplicated and in vLLM they all using sync_and_slice_intermediate_tensors

-1. It seems that _determine_batch_execution_and_padding in VLLM includes the logic of (1) padding for sp (2) dispatch_cudagraph (3) ubatch sync/handshake for dp (4) _sync_metadata_across_dp. In my opinion, maybe we can refactor modelrunner using this func after integrating the logic of ubatch/dbo in vllm-ascend? i.e.,use coordinate_batch_across_dp instead of _sync_metadata_across_dp and move these logics into _determine_batch_execution_and_padding?

-2. Yes. i try to remove the duplicated code in dummy_run. please take a look at the latest commit. @lidenghui1110

[lidenghui1110]

@lidenghui1110 PTAL

It looks like refactor in #6191 missed this part. Maybe we need a test case to cover pp+fc1 situation. And I have some questions here:

1. For we are trying to refactor modelrunner more like GPUModelRunner, shall we use the same logic as vLLM using _determine_batch_execution_and_padding?
2. In this PR only using sync_and_slice_intermediate_tensors in preprocess, can we use same logic in dummy_run using sync_and_slice_intermediate_tensors? The code is duplicated and in vLLM they all using sync_and_slice_intermediate_tensors

-1. It seems that _determine_batch_execution_and_padding in VLLM includes the logic of (1) padding for sp (2) dispatch_cudagraph (3) ubatch sync/handshake for dp (4) _sync_metadata_across_dp. In my opinion, maybe we can refactor modelrunner using this func after integrating the logic of ubatch/dbo in vllm-ascend? i.e.,use coordinate_batch_across_dp instead of _sync_metadata_across_dp and move these logics into _determine_batch_execution_and_padding?

-2. Yes. i try to remove the duplicated code in dummy_run. please take a look at the latest commit. @lidenghui1110

LGTM. I'm not familiar with ubatch/dbo, let's see code refactor in the future.

[zxdukki]

Run CI again.

[zxdukki]

It seems that the community has refactor the model runner using _determine_batch_execution_and_padding! @lidenghui1110
So we donot need to pad the tokens anymore. However, we should also replace is_residual_scattered_for_sp with enable_sp in sync_and_slice_intermediate_tensors.

[lidenghui1110]

It seems that the community has refactor the model runner using _determine_batch_execution_and_padding! @lidenghui1110 So we donot need to pad the tokens anymore. However, we should also replace is_residual_scattered_for_sp with enable_sp in sync_and_slice_intermediate_tensors.

Good！That's fine.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[lidenghui1110]

@zxdukki Could U please make this PR forward? PP is not available for a long time in the main branch due to this bug.

[zxdukki]

@zxdukki Could U please make this PR forward? PP is not available for a long time in the main branch due to this bug.

Sorry for the late reply, i will check it today.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[zxdukki]

It should be fixed in #7896 , close it