Update scaled_dot_product_attention to work with >6 inputs in latest torch version #2021
Conversation
|
Thanks for the review @ZachNagengast. I actually have a fix for this issue locally that I was planning to put up for a PR soon. I was waiting to bundle it with a few other small changes. We should raise an error if the 7th value (i.e. the scale) is not |
|
Yea no problem, I'll add it here shortly. |
|
@TobyRoseman This should be all set, although I'm not sure how to trigger CI here. |
|
Thanks @ZachNagengast for the changes. It looks good to me. I have to kick off the CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/1046916275 |
The input issue is now fixed but I am getting this liuliu/swift-diffusion#48 due to AttributeError: 'Namespace' object has no attribute 'merge_chunks_in_pipeline_model' |
|
@TobyRoseman Looks like the CI passed, anything else needed? @Zulqurnain24 This is because |
|
@ZachNagengast - nothing else is needed. I will merge it after the release is finished. |
|
@ZachNagengast - Our release has finished. I would merge this now, but there is a conflict. Can you fix the conflict? Then I'll kick off another CI run. |
|
Ok great, I'll merge in main shortly |
|
@TobyRoseman All set, there was code here that was doing similar but didn't handle the error if |
|
Thanks @ZachNagengast - the change looks good. Update CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/1059479892 |
|
@TobyRoseman Unclear why the previous ci failed, but I just updated from main again, perhaps you can rerun |
|
@ZachNagengast - the CI failures look related to your change. I don't think updating from I suggest you try to reproduce these failures locally. Or let me know if you'd like me to try my previously mentioned fix. |
|
@TobyRoseman I see the issue, there were changes that came from the merge that I didn't notice originally. Updated to restore the pre-merge version. |
|
The current diff makes it very difficult to understand what changes you have actually made. Please do a rebase squash on top of current |
Handle new scaled param in scaled_dot_product_attention Fix param name Update docstring and error type Update format Restore original scaled_dot_product_attention Fix is_causal for scaled dot product attn
ZachNagengast
force-pushed
the
fix-scaled_dot_product_attention-inputs
branch
from
571e162
to
4fa16a0
Compare
|
@TobyRoseman Alright I simplified the whole thing a bit, all that needed changing was a little change in the original implementation where |
Thank you. The diff is much clearer. |
| dropout = 0.0 if len(inputs) < 5 else inputs[4] | ||
| is_causal = False if len(inputs) < 6 else inputs[5].val | ||
| if attn_mask is not None and is_causal: | ||
| inputs = _get_inputs(context, node, expected=[6, 7]) |
There was a problem hiding this comment.
We should keep the current (less strict requirement) of min_expected=3 rather than expected=[6, 7]. We should keep the logic from the above deleted code that set those to default values if they are not present.
|
This looks great. CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/1068616173 |
|
@TobyRoseman Missed one of the is_causal changes from previous causing this error: Fixed now. |
|
CI is green. Thanks for the submission @ZachNagengast. |
In torch 2.1,
scaled_dot_product_attentionnow has 7 inputs, whereas 2.0 has only 6.2.0 docs: https://pytorch.org/docs/2.0/generated/torch.nn.functional.scaled_dot_product_attention.html#torch.nn.functional.scaled_dot_product_attention
2.1 docs: https://pytorch.org/docs/2.1/generated/torch.nn.functional.scaled_dot_product_attention.html#torch.nn.functional.scaled_dot_product_attention
This PR will allow for both interfaces to work, and simply ignore the new one. This issue arises for any conversion that uses this op on Torch 2.1:
ValueError: node hidden_states.11 (scaled_dot_product_attention) got 7 input(s), expected [6]