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.

Sign up for GitHub

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

Jump to bottom

Misc transformer fixes #14103

Merged
merged 2 commits into from
Jan 3, 2023
Merged

Misc transformer fixes #14103

merged 2 commits into from
Jan 3, 2023

Conversation

hariharans29
Copy link
Member

@hariharans29 hariharans29 commented Dec 31, 2022 •
edited
Loading

Description

  1. SkipLayerNormalization has a new output (Support (Bias)SkipLayerNormalization fusion in GPT2 #13988) and the symbolic shape inference script needs corresponding updates

  2. The greedy sampling op (Sampling op #13426) shouldn't re-use the logits buffer as its corresponding kernel doesn't seem to support it yet.

Motivation and Context

Fix some transformer issues

@hariharans29 hariharans29 requested review from tianleiwu and wangyems and removed request for tianleiwu December 31, 2022 17:09
hariharans29
hariharans29 commented Dec 31, 2022
View reviewed changes
onnxruntime/contrib_ops/cuda/transformers/generation_device_helper.cc
// TODO(hasesh/wy): Support re-using logits buffer for the sampling case.
// Currently, we cannot re-use the logits because the sampling logic expects
// `next_token_scores` to be populated.
auto is_reuse_logits_buffer = !do_sampling && (input_length == 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

CC: @wangyems

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

tianleiwu
tianleiwu previously approved these changes Jan 1, 2023
View reviewed changes
@wangyems
Copy link
Contributor

wangyems commented Jan 3, 2023

Thx for the fix!
One thing I'm not sure about is the exported gpt2 graph:
image
Does this look expected?

Also, the beam search fp32 model seems have parity issue with skiplayernorm fusion(if disable the skiplayernorm the parity is good). Not sure if it's just error accumulation or sth else

@hariharans29
Copy link
Member Author

hariharans29 commented Jan 3, 2023 •
edited
Loading

Thx for the fix! One thing I'm not sure about is the exported gpt2 graph: image Does this look expected?

Also, the beam search fp32 model seems have parity issue with skiplayernorm fusion(if disable the skiplayernorm the parity is good). Not sure if it's just error accumulation or sth else

Thanks Ye for raising this !

  1. Thanks! I think the postprocess() method needed an update to look for SLN - This is where the Shape nodes + Reshape + Gemm get fused to MatMul. Without this update, you see the odd looking Reshapes and Gemms. It should be fixed now

  2. I will debug the BeamSearch fp32 parity issue with SLN fusion

wangyems
wangyems approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@wangyems wangyems left a comment

Choose a reason for hiding this comment

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

lgtm

@wangyems wangyems merged commit d43e0ec into main Jan 3, 2023
@wangyems wangyems deleted the hari/symbolic_fixes branch January 3, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangyems wangyems wangyems approved these changes

@tianleiwu tianleiwu tianleiwu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hariharans29 @wangyems @tianleiwu