PagedAttention Transformation: Rank alignment for replacements #24690
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_heads dimension broadcasted in SDPA itself and not in the UBR pattern.
…ttention/prev_sequence_length_pattern.cpp Co-authored-by: Ivan Tikhonov <ivan.tikhonov@intel.com>
…tion. Allowed optional Reshape in UBR pattern (appeared in one of MQA cases).
…n matching with Or pattern and multi-output nodes.
github-actions
bot
added
the
category: transformations
ilya-lavrenov
approved these changes
May 27, 2024
itikhono
reviewed
May 27, 2024
| } | ||
| if (replacement->get_output_element_type(0) != target_type) { |
There was a problem hiding this comment.
is it possible that we will have f16 -> fp32, int -> fp conversions here?
Could this cause any potential issues in accuracy?
itikhono
reviewed
May 27, 2024
| replacement = std::make_shared<v0::Convert>(replacement, target_type); | ||
| } | ||
| auto required_shape = gather->get_output_partial_shape(0); | ||
| if (replacement->get_output_partial_shape(0) != required_shape && required_shape.rank().is_static()) { |
There was a problem hiding this comment.
minor: It's better to change order of the checks:
required_shape.rank().is_static() && replacement->get_output_partial_shape(0) != required_shape
itikhono
reviewed
May 27, 2024
| replacement = std::make_shared<v0::Convert>(replacement, target_type); | ||
| } | ||
| auto required_shape = gather->get_output_partial_shape(0); | ||
| if (replacement->get_output_partial_shape(0) != required_shape && required_shape.rank().is_static()) { |
There was a problem hiding this comment.
minor: It's better to change order of the checks:
required_shape.rank().is_static() && replacement->get_output_partial_shape(0) != required_shape
|
@CuriousPanCake did we test these changes via #24634? |
Not yet. I'll test ASAP |
See "Confirm, that expected models are fixed by this PR." above |
itikhono
approved these changes
May 27, 2024
CuriousPanCake
approved these changes
May 27, 2024
ilya-lavrenov
merged commit b0dfa6a
into
openvinotoolkit:master
112 of 116 checks passed
slyalin
added a commit
to slyalin/openvino
that referenced
this pull request
May 27, 2024
…inotoolkit#24690) During the elimination of dependencies from `beam_idx` input and `ReadValue`(s), we are replacing them by the new PA-related inputs and sub-expressions dependent on other remaining inputs. In such replacements we need to guarantee matching shape and element type of old and new nodes. Before this PR it was not guaranteed for shape and sometimes a scalar was replaced by a shape of rank 1 that led to errors like `'start' input is not a scalar`. Now the shape is aligned. --------- Co-authored-by: Ivan Tikhonov <ivan.tikhonov@intel.com> Co-authored-by: Ilya Lavrenov <ilya.lavrenov@intel.com>
ilya-lavrenov
added a commit
that referenced
this pull request
May 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During the elimination of dependencies from
beam_idxinput andReadValue(s), we are replacing them by the new PA-related inputs and sub-expressions dependent on other remaining inputs. In such replacements we need to guarantee matching shape and element type of old and new nodes. Before this PR it was not guaranteed for shape and sometimes a scalar was replaced by a shape of rank 1 that led to errors like'start' input is not a scalar. Now the shape is aligned.