Skip to content
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

[ZeroPadding] revert zero_padding #8973 #9003

Merged

Conversation

DrownFish19
Copy link
Collaborator

PR types

Bug fixes

PR changes

Others

Description

  1. Revert commit [ZeroPadding] padding to max_length for sequence parallel #8973.
  2. Set padding to max_length for sequence_parallel in llm/run_finetune.py.
  3. Update test cases.

Copy link

paddle-bot bot commented Aug 26, 2024

Thanks for your contribution!

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.

Project coverage is 54.14%. Comparing base (56dba6d) to head (6c8ee06).
Report is 234 commits behind head on develop.

Files with missing lines Patch % Lines
paddlenlp/datasets/zero_padding_dataset.py 11.11% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #9003      +/-   ##
===========================================
- Coverage    54.78%   54.14%   -0.65%     
===========================================
  Files          647      650       +3     
  Lines       102502   103871    +1369     
===========================================
+ Hits         56160    56237      +77     
- Misses       46342    47634    +1292     

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


🚨 Try these New Features:

@@ -53,42 +53,7 @@ class ZeroPadding:
]

@classmethod
def _pad_batch_records_to_max_length(cls, batch_records, max_length, pad_token=0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

这段代码是挪走了,还是彻底不需要了?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不需要了,我们使用tokenzier的pad进行补充

padding = "max_length"
else:
max_length = None
padding = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

额,我们不padding到最大长度是什么情况?是说,不同batch 最大长度改变?(之前一直反馈这种情况容易显存泄漏。)

Copy link
Contributor

Choose a reason for hiding this comment

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

但如果不是zero padding没必要pad到最长

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sequence_parallel应该是padding到最长,否则中间长度变化可能不能被tensor_parallel_degree整除

Copy link
Contributor

@lugimzzz lugimzzz left a comment

Choose a reason for hiding this comment

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

lgtm

padding = "max_length"
else:
max_length = None
padding = True
Copy link
Contributor

Choose a reason for hiding this comment

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

但如果不是zero padding没必要pad到最长

@ZHUI ZHUI merged commit 56d293d into PaddlePaddle:develop Aug 26, 2024
9 of 12 checks passed
@DrownFish19 DrownFish19 deleted the dev_20240822_fix_zero_padding_dpo branch August 27, 2024 00:52
lixcli pushed a commit to lixcli/PaddleNLP that referenced this pull request Aug 28, 2024
Mangodadada pushed a commit to Mangodadada/PaddleNLP that referenced this pull request Sep 10, 2024
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.

3 participants