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

Enable input_embeds for ChatGLM / ChatGLMForConditionalGeneration #5775

Merged
merged 4 commits into from
May 18, 2023

Conversation

parap1uie-s
Copy link
Contributor

Fix typo and minor bugs to enable the input_embeds input rather than input_ids

PR types

Bug fixes

PR changes

Models / APIs

Description

See changelog / Commit

Fix typo and minor bugs to enable the input_embeds input rather than input_ids
@paddle-bot
Copy link

paddle-bot bot commented Apr 24, 2023

Thanks for your contribution!

wawltor
wawltor previously approved these changes Apr 24, 2023
Copy link
Collaborator

@wawltor wawltor left a comment

Choose a reason for hiding this comment

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

LGTM

@wawltor wawltor self-requested a review April 24, 2023 11:58
@wawltor wawltor dismissed their stale review April 24, 2023 11:58

wrong approve

wawltor
wawltor previously approved these changes Apr 24, 2023
else:
raise ValueError("You have to specify either input_ids or inputs_embeds")

if inputs_embeds is None:
inputs_embeds = self.word_embeddings(input_ids)
inputs_embeds = inputs_embeds.transpose([1, 0, 2])
inputs_embeds = inputs_embeds.transpose([1, 0, 2])
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的一些小坑是在于input_ids是作为默认输出,在export和inference逻辑是默认使用input_ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

其它transformer模型(比如ERNIE)对input_ids都有默认值None。同时PretrainedModel实例也都会检查,要求input_ids和embeds不同时为None,不同时有值。所以这里应该还好

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #5775 (e69f096) into develop (d030c63) will decrease coverage by 0.03%.
The diff coverage is 40.00%.

@@             Coverage Diff             @@
##           develop    #5775      +/-   ##
===========================================
- Coverage    61.60%   61.57%   -0.03%     
===========================================
  Files          489      489              
  Lines        68500    68540      +40     
===========================================
+ Hits         42197    42205       +8     
- Misses       26303    26335      +32     
Impacted Files Coverage Δ
paddlenlp/transformers/chatglm/modeling.py 62.55% <40.00%> (-0.20%) ⬇️

... and 3 files with indirect coverage changes

@@ -826,7 +826,7 @@ def update_model_kwargs_for_generation(

def forward(
self,
input_ids,
input_ids=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

这里 input_ids 默认设为 None 时,需要修改下 ChatGLMModelposition_ids is Noneattention_mask is None 的分支逻辑,因为这两个分支都依赖 input_ids

1)可支持为None 的,在 input_ids is None 时将 input_ids 相关的参数改为从 input_embeds 获取。
2) 不支持为 None 的,需要显式抛出异常,说明原因。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

按照逻辑2)修改了,要求input_ids与attention_mask / position_ids不同时为None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line693

@wawltor wawltor merged commit 140752d into PaddlePaddle:develop May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants