-
Notifications
You must be signed in to change notification settings - Fork 197
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
Fix LLM tests #1088
base: dev
Are you sure you want to change the base?
Fix LLM tests #1088
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question about new pattern for layer replacement.
@@ -1,3 +1,3 @@ | |||
# optimum-amd[brevitas] @ git+https://github.com/huggingface/optimum-amd.git@main | |||
tqdm | |||
transformers[sentencepiece]==4.45.2 | |||
transformers[sentencepiece]>=4.46.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you bumped the version here? Does this no longer work with transformers<4.46.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests would fail in 4.45.2.
"act_equalization": "layerwise", | ||
"gptq": True, | ||
"float_ppl": 31056.0, # 31274.05078125 | ||
"quant_ppl": 33056.0},]) # 33139.23046875},]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do the numbers in the comments mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those were the numbers for transformers < 4.46.0. They need to be removed, but I wanted to get confirmation on the preferred way to go regarding the transformers version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - can you add that to the comment?
src/brevitas/nn/quant_mha.py
Outdated
@@ -602,7 +602,8 @@ def forward( | |||
key_padding_mask: Optional[Tensor] = None, | |||
need_weights: bool = True, | |||
attn_mask: Optional[Tensor] = None, | |||
average_attn_weights: bool = True) -> Tuple[Tensor, Optional[Tensor]]: | |||
average_attn_weights: bool = True, | |||
position_ids: Optional[Tensor] = None) -> Tuple[Tensor, Optional[Tensor]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not modify this forward function. position_ids
is not in PyTorch's MHA layer's forward function. If this is argument is ignored, then QuantizeOPTAttention
should handle that.
Reason for this PR
See #1076.
Changes Made in this PR
Since transformers 4.46.0 (https://github.com/huggingface/transformers/releases/tag/v4.46.0), OPT attention mechanism relies, by default, on OPTSdpaAttention, thus resulting in the method replace_mha_with_quantizable_layers (https://github.com/pablomlago/brevitas/blob/master/src/brevitas_examples/llm/llm_quant/prepare_for_quantize.py#L11) not injecting the Brevitas layers.
Additionally, the signature of the forward function in OPTAttention was updated in transformers 4.46.0, thus requiring to update the signature of QuantMultiHeadAttention analogously, even if this attribute is not actually used.
Testing Summary
TODO
Risk Highlight
Checklist
dev
branch.