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

PassManager refactoring and new debug caps #25637

Merged
merged 8 commits into from
Jul 30, 2024
Merged

PassManager refactoring and new debug caps #25637

merged 8 commits into from
Jul 30, 2024

Conversation

itikhono
Copy link
Contributor

@itikhono itikhono commented Jul 19, 2024 •
edited
Loading

Details:

  • Simplified run_passes logic in pass::Manager class
  • Moved debugging logic to a separate Profiler class
  • Added a name for pass::Manager
  • Extended debug caps: added serialization by env variable, added filtring and collection perf statistics in a file

New debug output format:
image_2024-07-25_17-26-54

Tickets:

@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: transformations OpenVINO Runtime library - Transformations category: TF FE OpenVINO TensorFlow FrontEnd category: CPP API OpenVINO CPP API bindings labels Jul 19, 2024
@itikhono itikhono changed the title Itikhono/refactoring/pass manager PassManager refactoring Jul 19, 2024
@github-actions github-actions bot added category: LP transformations OpenVINO Low Precision transformations category: IR FE OpenVINO IR v10 / v11 FrontEnd category: TEMPLATE OpenVINO Template plugin category: ONNX FE OpenVINO ONNX FrontEnd labels Jul 23, 2024
evkotov
evkotov reviewed Jul 23, 2024
View reviewed changes
src/core/src/pass/manager.cpp Outdated Show resolved Hide resolved
src/core/src/pass/manager.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added category: GPU OpenVINO GPU plugin category: PDPD FE OpenVINO PaddlePaddle FrontEnd category: AUTO BATCH OpenVINO Auto Batch plugin category: PyTorch FE OpenVINO PyTorch Frontend category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: NPU OpenVINO NPU plugin labels Jul 24, 2024
@itikhono itikhono marked this pull request as ready for review July 24, 2024 09:35
@itikhono itikhono requested review from a team as code owners July 24, 2024 09:35
@itikhono itikhono requested review from a team as code owners July 24, 2024 09:35
@itikhono itikhono requested review from mvafin and rkazants July 24, 2024 09:35
@itikhono itikhono force-pushed the itikhono/refactoring/pass_manager branch from 96fd022 to 078f168 Compare July 24, 2024 10:50
@itikhono itikhono requested a review from evkotov July 25, 2024 10:49
@itikhono itikhono requested a review from jane-intel July 25, 2024 13:39
@itikhono itikhono changed the title PassManager refactoring PassManager refactoring and new debug caps Jul 25, 2024
praasz
praasz reviewed Jul 26, 2024
View reviewed changes
src/core/include/openvino/pass/manager.hpp Show resolved Hide resolved
src/core/src/pass/manager.cpp Outdated Show resolved Hide resolved
src/core/src/pass/manager.cpp Show resolved Hide resolved
src/core/src/pass/manager.cpp Show resolved Hide resolved
src/core/src/pass/manager.cpp Show resolved Hide resolved
@itikhono itikhono requested a review from praasz July 26, 2024 07:28
jane-intel
jane-intel approved these changes Jul 26, 2024
View reviewed changes
src/core/src/pass/sdpa_to_paged_attention.cpp
@@ -101,7 +101,7 @@ bool ov::pass::SDPAToPagedAttention::run_on_model(const std::shared_ptr<ov::Mode
auto batch_dim =
std::make_shared<v3::ShapeOf>(position_ids); // it is not always required, so will be disposed if not needed

ov::pass::Manager manager;
ov::pass::Manager manager("SDPA to PA");
Copy link
Contributor

Choose a reason for hiding this comment

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

naming style?

src/core/src/pass/manager.cpp

// This checks if we need to skip the graph transformation when the graph pass relies on
// static shape but the model state is dynamic.
if (pass->get_property(PassProperty::REQUIRE_STATIC_SHAPE) && model->is_dynamic()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use it in the project? Asking out of curiosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be honest, I don't know
most probably we don't , I will try to figure it out and might be delete it in the next PR

ilya-lavrenov
ilya-lavrenov reviewed Jul 29, 2024
View reviewed changes
src/core/include/openvino/pass/manager.hpp
@@ -23,8 +23,11 @@ class OPENVINO_API Manager {
Manager();
virtual ~Manager();

//// \brief Construct Manager with a provided name.
explicit Manager(std::string name);
Copy link
Contributor

Choose a reason for hiding this comment

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

why "UnnamedManager" is not used here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We want to collect information about managers/transformations execution time in a separate job. We need this name parameter here to identify manager in debug log

Copy link
Contributor

Choose a reason for hiding this comment

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

Added another PR #25842 to fix it

@andrei-kochin andrei-kochin added this pull request to the merge queue Jul 30, 2024
Merged via the queue into openvinotoolkit:master with commit d253f4f Jul 30, 2024
128 checks passed
@evkotov evkotov mentioned this pull request Jul 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evkotov evkotov evkotov approved these changes

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

@jane-intel jane-intel jane-intel approved these changes

@mvafin mvafin mvafin approved these changes

@rkazants rkazants Awaiting requested review from rkazants rkazants is a code owner automatically assigned from openvinotoolkit/openvino-pytorch-frontend-maintainers

@praasz praasz Awaiting requested review from praasz

Assignees
No one assigned
Labels
category: AUTO BATCH OpenVINO Auto Batch plugin category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: GPU OpenVINO GPU plugin category: IR FE OpenVINO IR v10 / v11 FrontEnd category: LP transformations OpenVINO Low Precision transformations category: NPU OpenVINO NPU plugin category: ONNX FE OpenVINO ONNX FrontEnd category: PDPD FE OpenVINO PaddlePaddle FrontEnd category: PyTorch FE OpenVINO PyTorch Frontend category: TEMPLATE OpenVINO Template plugin category: TF FE OpenVINO TensorFlow FrontEnd category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: transformations OpenVINO Runtime library - Transformations under_perf_check
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@itikhono @ilya-lavrenov @mvafin @jane-intel @evkotov @praasz @andrei-kochin