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

[TRANSFORMATIONS] Remove usages of legacy names in transformations #26855

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

[TRANSFORMATIONS] Remove usages of legacy names in transformations #26855

wants to merge 8 commits into from

Conversation

CuriousPanCake
Copy link
Contributor

@CuriousPanCake CuriousPanCake commented Sep 30, 2024 •
edited by praasz
Loading

[TRANSFORMATIONS] Remove usages of legacy names in transformations

Remove usages of deprecated legacy names in transformations.

Related PR:

Tickets:

@CuriousPanCake CuriousPanCake requested a review from a team as a code owner September 30, 2024 09:14
@CuriousPanCake CuriousPanCake requested review from itikhono and evkotov and removed request for a team September 30, 2024 09:14
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Sep 30, 2024
@CuriousPanCake
Copy link
Contributor Author

CuriousPanCake commented Sep 30, 2024 •
edited
Loading

Not removing the functions themselves as they're still used in
src/inference/src/model_reader.cpp and
src/inference/src/dev/icompiled_model.cpp

@itikhono
Copy link
Contributor

Not removing the functions themselves as they're still used in openvino/src/inference/src/model_reader.cpp and openvino/src/inference/src/dev/icompiled_model.cpp

could you try to delete it from these places?

@CuriousPanCake CuriousPanCake requested a review from a team as a code owner September 30, 2024 09:41
@github-actions github-actions bot added the category: inference OpenVINO Runtime library - Inference label Sep 30, 2024
@CuriousPanCake
Copy link
Contributor Author

Not removing the functions themselves as they're still used in openvino/src/inference/src/model_reader.cpp and openvino/src/inference/src/dev/icompiled_model.cpp

could you try to delete it from these places?

Sure. Not removed entirely, but moved to the corresponding files as static functions per the following comment by @ilya-lavrenov:
// Note, upon removal of 'create_ie_output_name', just move it to this file as a local function
// we still need to add operation names as tensor names for outputs for IR v10

@ilya-lavrenov
Copy link
Contributor

Not removing the functions themselves as they're still used in openvino/src/inference/src/model_reader.cpp and openvino/src/inference/src/dev/icompiled_model.cpp

could you try to delete it from these places?

Sure. Not removed entirely, but moved to the corresponding files as static functions per the following comment by @ilya-lavrenov: // Note, upon removal of 'create_ie_output_name', just move it to this file as a local function // we still need to add operation names as tensor names for outputs for IR v10

right, places where IR v10 is used, still need to keep this legacy logic with names. So, keeping them closer to places where they are used - right strategy :)

@ilya-lavrenov ilya-lavrenov added this to the 2024.5 milestone Oct 7, 2024
@praasz praasz mentioned this pull request Oct 29, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024
@praasz
[core] Remove legacy tensor name (#27307)
2725b16 
### Details:
 - Remove legacy name from tensor descriptor

### Related PR:
- #26855

### Tickets:
 - CVS-156182

---------

Signed-off-by: Pawel Raasz <[email protected]>
itikhono
itikhono reviewed Nov 13, 2024
View reviewed changes
...ommon/transformations/src/transformations/op_conversions/convert_nms9_to_nms_ie_internal.cpp
OPENVINO_SUPPRESS_DEPRECATED_START
output_0.get_node_shared_ptr()->set_friendly_name(op::util::create_ie_output_name(nms_9->output(0)));
OPENVINO_SUPPRESS_DEPRECATED_END
output_0.get_node_shared_ptr()->set_friendly_name(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these friendly names still need to be set?

praasz
praasz approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@praasz praasz left a comment

Choose a reason for hiding this comment

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

Fix the tests

@CuriousPanCake
Copy link
Contributor Author

CuriousPanCake commented Nov 15, 2024 •
edited
Loading

Fix the tests

The tests should pass now, however still blocked by Nvidia plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itikhono itikhono itikhono approved these changes

@ilya-lavrenov ilya-lavrenov ilya-lavrenov left review comments

@praasz praasz praasz approved these changes

@evkotov evkotov Awaiting requested review from evkotov

Assignees

@praasz praasz

Labels
category: inference OpenVINO Runtime library - Inference category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Milestone
2024.5
Development

Successfully merging this pull request may close these issues.
4 participants
@CuriousPanCake @itikhono @ilya-lavrenov @praasz