Skip to content

Conversation

@yuanlehome
Copy link
Collaborator

@yuanlehome yuanlehome commented Nov 28, 2025

Motivation

💡 If this PR is a Cherry Pick, the PR title needs to follow the format by adding the [Cherry-Pick] label at the very beginning and appending the original PR ID at the end. For example, [Cherry-Pick][CI] Add check trigger and logic(#5191)

💡 如若此PR是Cherry Pick,PR标题需遵循格式,在最开始加上[Cherry-Pick]标签,以及最后面加上原PR ID,例如[Cherry-Pick][CI] Add check trigger and logic(#5191)

V0 loader下,存在bias的模型,在TSP下不能被除以 tp_size

Modifications

Usage or Command

Accuracy Tests

Checklist

  • Add at least a tag in the PR title.
    • Tag list: [[FDConfig],[APIServer],[Engine], [Scheduler], [PD Disaggregation], [Executor], [Graph Optimization], [Speculative Decoding], [RL], [Models], [Quantization], [Loader], [OP], [KVCache], [DataProcessor], [BugFix], [Docs], [CI], [Optimization], [Feature], [Benchmark], [Others], [XPU], [HPU], [GCU], [DCU], [Iluvatar], [Metax]]
    • You can add new tags based on the PR content, but the semantics must be clear.
  • Format your code, run pre-commit before commit.
  • Add unit tests. Please write the reason in this PR if no unit tests.
  • Provide accuracy results.
  • If the current PR is submitting to the release branch, make sure the PR has been submitted to the develop branch, then cherry-pick it to the release branch with the [Cherry-Pick] PR tag.

Copilot AI review requested due to automatic review settings November 28, 2025 02:26
@paddle-bot
Copy link

paddle-bot bot commented Nov 28, 2025

Thanks for your contribution!

Copilot finished reviewing on behalf of yuanlehome November 28, 2025 02:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR attempts to fix an issue related to bias handling in tensor sequence parallelism (TSP) for the o_proj layer. However, the implementation contains a critical bug that needs to be addressed.

Key Issues:

  • The PR title lacks the required tag format (should be [BugFix] or similar)
  • The PR description is incomplete - it doesn't explain what problem is being fixed or why these modifications are needed
  • The code change introduces a bug where variable k is assigned twice consecutively, causing the first assignment to be ineffective
Comments suppressed due to low confidence (1)

fastdeploy/model_executor/load_weight_utils.py:291

  • This assignment to 'k' is unnecessary as it is redefined before this value is used.
                k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.weight"

Comment on lines 291 to 294
k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.weight"
k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.bias"
if k in weight_list:
no_tp_action_keys.append(k)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable k is assigned twice consecutively. The first assignment on line 291 (self_attn.o_proj.weight) is immediately overwritten by line 292 (self_attn.o_proj.weight.bias), making line 291 ineffective.

If both weight and bias keys need to be added to no_tp_action_keys, consider handling them separately:

k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.weight"
if k in weight_list:
    no_tp_action_keys.append(k)
k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.bias"
if k in weight_list:
    no_tp_action_keys.append(k)

Note: The key path also appears incorrect - it should likely be o_proj.bias not o_proj.weight.bias.

Suggested change
k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.weight"
k = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.bias"
if k in weight_list:
no_tp_action_keys.append(k)
k_weight = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.weight"
if k_weight in weight_list:
no_tp_action_keys.append(k_weight)
k_bias = f"ernie.{prefix_layer_name}.{i}.self_attn.o_proj.bias"
if k_bias in weight_list:
no_tp_action_keys.append(k_bias)

Copilot uses AI. Check for mistakes.
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@ef5aa5c). Learn more about missing BASE report.

Files with missing lines Patch % Lines
fastdeploy/model_executor/load_weight_utils.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #5284   +/-   ##
==========================================
  Coverage           ?   60.45%           
==========================================
  Files              ?      322           
  Lines              ?    39176           
  Branches           ?     5885           
==========================================
  Hits               ?    23683           
  Misses             ?    13627           
  Partials           ?     1866           
Flag Coverage Δ
GPU 60.45% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yuanlehome yuanlehome changed the title fix tsp bias add [BugFix] fix tsp o_proj bias add Nov 28, 2025
@Jiang-Jia-Jun Jiang-Jia-Jun merged commit 35479b6 into PaddlePaddle:develop Nov 28, 2025
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants