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

[TRANSFROMATIONS] Add support for 'inputs_embeds' input in SDPAToPA #27158

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

CuriousPanCake
Copy link
Contributor

@CuriousPanCake CuriousPanCake commented Oct 21, 2024

[TRANSFROMATIONS] Add support for 'inputs_embeds' input in SDPAToPA

Add support for 'input_embeds' input in SDPAToPA transformation.
The input is used in VLM instead of 'input_ids' in text-only models.

The changes enable support of the SDPAToPA transformation for the following models:

  • llava-hf/llava-1.5-7b-hf
  • llava-hf/llava-v1.6-mistral-7b-hf
  • llava-hf/llava-v1.6-vicuna-7b-hf
  • llava-hf/llama3-llava-next-8b-hf
  • openbmb/MiniCPM-V-2_6

Signed-off-by: Andrii Staikov [email protected]

Add support for 'input_embeds' input in SDPAToPA transformation.
The input is used in VLM instead of 'input_ids' in text-only models.

The changes enable support of the SDPAToPA transformation for the
following models:
 * llava-hf/llava-1.5-7b-hf
 * llava-hf/llava-v1.6-mistral-7b-hf
 * llava-hf/llava-v1.6-vicuna-7b-hf
 * llava-hf/llama3-llava-next-8b-hf
 * openbmb/MiniCPM-V-2_6

Signed-off-by: Andrii Staikov <[email protected]>

- Tickets:
 * CVS-152288
@CuriousPanCake CuriousPanCake requested a review from a team as a code owner October 21, 2024 09:09
@CuriousPanCake CuriousPanCake requested review from itikhono and removed request for a team October 21, 2024 09:09
@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Oct 21, 2024
@CuriousPanCake CuriousPanCake changed the title [TRANSFROMATIONS] Add support for 'input_embeds' input in SDPAToPA [TRANSFROMATIONS] Add support for 'inputs_embeds' input in SDPAToPA Oct 21, 2024
@ilya-lavrenov ilya-lavrenov added this to the 2024.5 milestone Oct 22, 2024
@ilya-lavrenov
Copy link
Contributor

should we continue with it?
looks like easy to finalize

@CuriousPanCake CuriousPanCake requested review from a team as code owners November 6, 2024 11:28
@itikhono
Copy link
Contributor

just as a formality: could you create a ticket? probably a bug: SDPA to PA transformation doesn't work for llava and mini cpm models

There's one already

please mention it in the PR description

@github-actions github-actions bot added category: CI OpenVINO public CI github_actions Pull requests that update GitHub Actions code labels Nov 13, 2024
@github-actions github-actions bot removed category: CI OpenVINO public CI github_actions Pull requests that update GitHub Actions code labels Nov 14, 2024
Comment on lines 61 to 67
if (auto casted_param = std::dynamic_pointer_cast<v0::Parameter>(param.get_node_shared_ptr())) {
return casted_param;
} else {
OPENVINO_THROW("The model is in the inconsistent state. Found input '",
name,
"', but couldn't cast it to v0::Parameter.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto casted_param = std::dynamic_pointer_cast<v0::Parameter>(param.get_node_shared_ptr())) {
return casted_param;
} else {
OPENVINO_THROW("The model is in the inconsistent state. Found input '",
name,
"', but couldn't cast it to v0::Parameter.");
}
auto casted_param = ov::as_type_ptr<v0::Paramter>(param.get_node_shared_ptr()));
OPENVINO_ASSERT(casted_param , "The model is in the inconsistent state. Input is not paramter: ", casted_param);
return casted_param;

Use ov::as_type_ptr<> instead of std::dynamic_pointer_cast for operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

const std::string& name) -> std::shared_ptr<v0::Parameter> {
for (auto& param : model->inputs()) {
const auto& names = param.get_names();
if (names.find(name) != names.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (names.find(name) != names.end()) {
if (names.count(name)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -53,8 +53,28 @@ bool ov::pass::SDPAToPagedAttention::run_on_model(const std::shared_ptr<ov::Mode

auto sliding_window = v0::Constant::create(element::i32, Shape{}, {0}); // sliding_window

auto has_parameter = [=](const std::shared_ptr<ov::Model>& model,
const std::string& name) -> std::shared_ptr<v0::Parameter> {
for (auto& param : model->inputs()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (auto& param : model->inputs()) {
for (const auto& param : model->inputs()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -53,8 +53,28 @@ bool ov::pass::SDPAToPagedAttention::run_on_model(const std::shared_ptr<ov::Mode

auto sliding_window = v0::Constant::create(element::i32, Shape{}, {0}); // sliding_window

auto has_parameter = [=](const std::shared_ptr<ov::Model>& model,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rename as it not returns bool value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also don't like the name 100%, but I'd leave it as it is as it's used in cases when want to do something if a model has a parameter.

@CuriousPanCake
Copy link
Contributor Author

I've disabled a few failing models due to the latest version of transformers (4.46.2). The updated version of the package adds regression to the models, but enables the nanollava model. The regression will be fixed in 157416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: JAX FE OpenVINO JAX FrontEnd category: PyTorch FE OpenVINO PyTorch Frontend category: TF FE OpenVINO TensorFlow FrontEnd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants