NPUW: Introduce 'last' keyword for dump options #26841

eshiryae · 2024-09-27T16:27:19Z

Details:

The problem is that for different networks the number of NPUW's detected block may be different and it is hard to tell what the last block number will be. Now 'last' option can be used to tell NPUW to dump tail block without setting specific number.

Tickets:

E-139857

dmatveev · 2024-10-10T15:27:13Z

@AsyaPronina please review

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp

src/plugins/intel_npu/src/plugin/npuw/util.hpp

AsyaPronina · 2024-10-21T10:13:38Z

src/plugins/intel_npu/src/al/include/npuw_private_properties.hpp

@@ -307,7 +307,9 @@ static constexpr ov::Property<bool> full{"NPUW_DUMP_FULL"};
 * Type: std::string.
 * Dump the specified subgraph(s) in OpenVINO IR form in the current directory.
 * Possible values: Comma-separated list of subgraph indices or "YES" for all
- * subgraphs, "NO" or just empty value to turn option off. E.g. "0,1" or "YES".
+ * subgraphs, "NO" or just empty value to turn option off. Keyword "last" can
+ * be used for dumping last subgraph without specifying it by specific indice.


Nitpick: indice typo, but comment might be ignored

that's fair to be index, why should this comment be ignored

It's just aligned with comment message above:

Possible values: Comma-separated list of subgraph indices or "YES" for all subgraphs

Should I change it too?

I thought that plural form of index is indices

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp

dmatveev · 2024-10-21T10:14:28Z

src/plugins/intel_npu/src/al/include/npuw_private_properties.hpp

@@ -307,7 +307,9 @@ static constexpr ov::Property<bool> full{"NPUW_DUMP_FULL"};
 * Type: std::string.
 * Dump the specified subgraph(s) in OpenVINO IR form in the current directory.
 * Possible values: Comma-separated list of subgraph indices or "YES" for all
- * subgraphs, "NO" or just empty value to turn option off. E.g. "0,1" or "YES".
+ * subgraphs, "NO" or just empty value to turn option off. Keyword "last" can
+ * be used for dumping last subgraph without specifying it by specific indice.


that's fair to be index, why should this comment be ignored

dmatveev · 2024-10-21T10:15:28Z

src/plugins/intel_npu/src/plugin/npuw/base_sync_infer_request.cpp

@@ -211,7 +211,8 @@ void ov::npuw::IBaseInferRequest::infer() {

 void ov::npuw::IBaseInferRequest::dump_input_tensors(std::size_t idx) {
    const std::string dump_ios_opt = m_npuw_model->m_cfg.get<::intel_npu::NPUW_DUMP_IO>();
-    if (!ov::npuw::util::is_set(idx, dump_ios_opt)) {
+    const std::size_t last_idx = m_npuw_model->m_compiled_submodels.size() - 1;


maybe it makes sense to make last_idx be end_idx - exclusive, like end() iterator - so that you have -1 in just a single place.

Do you mean to add method to CompiledModel class which return the index of the last submodel?

Might be Dmitry means just to rename last_idx to end_idx and use it without -1 when passing to the ov::npuw::util::is_set(idx, dump_ios_opt, end_idx), where you can substract 1 right inside the is_set function?

Yes, exactly what @AsyaPronina says

dmatveev · 2024-10-21T10:18:11Z

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp

@@ -695,8 +704,9 @@ ov::SoPtr<ov::ICompiledModel> ov::npuw::CompiledModel::compile_submodel(const st

 void ov::npuw::CompiledModel::dump_on_fail(std::size_t id, const std::string& device_to_try, const char* extra) {
    const std::string dof_opt = m_cfg.get<::intel_npu::NPUW_DUMP_SUBS_ON_FAIL>();
+    const std::size_t last_idx = m_compiled_submodels.size() - 1;


surely, it should be end_idx to avoid these recurring -1s.

dmatveev · 2024-10-21T10:21:14Z

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp

+    std::string fsd_opt = m_cfg.get<::intel_npu::NPUW_SUBMODEL_DEVICE>();
+    // Change "last" keyword to tail subgraph number
+    std::size_t last_pos = fsd_opt.find("last");
+    if (last_pos != std::string::npos) {
+        fsd_opt.erase(last_pos, 4);
+        fsd_opt.insert(last_pos, std::to_string(last_sub_idx));
+    }
+
    forced_sub_devices = ::intel_npu ::OptionParser<std::map<std::size_t, std::string>>::parse(fsd_opt);


@AsyaPronina is this OptionParser ours? Can we add extra parameters to parse in this case? I'd prefer that code handle last for all its users (assuming the end of the range is passed to parse()).

This is NPU OptionParser, it is generic callable to parse from string to requested type. However, we can as it to parse to std::map<std::string, std::string> instead of std::map<std::size_t, std::string>, where key will be string, so it may accept "last". But then we need to parse other indices from string to std::size_t on our own.

This is NPU OptionParser, it is generic callable to parse from string to requested type. However, we can as it to parse to std::map<std::string, std::string> instead of std::map<std::size_t, std::string>, where key will be string, so it may accept "last". But then we need to parse other indices from string to std::size_t on our own.

so it means we can also extend it with a new parse() overload for this particular template instantiation, isn't it?

Yes, you are right, thank you!

dmatveev · 2024-10-21T10:21:35Z

src/plugins/intel_npu/src/plugin/npuw/util.cpp

@@ -17,20 +17,28 @@
 #include "openvino/runtime/make_tensor.hpp"  // get_tensor_impl
 #include "util_xarch.hpp"

-bool ov::npuw::util::is_set(const std::size_t sub_idx, const std::string& opt) {
+bool ov::npuw::util::is_set(const std::size_t sub_idx, const std::string& opt, const std::size_t last_idx) {


@AsyaPronina what tests can you recommend here to add?

Something likes this one: https://github.com/openvinotoolkit/openvino/blob/master/src/plugins/intel_npu/tests/functional/behavior/npuw/behavior_tests.cpp#L672
It can be made parametrized for multiple options.

The problem is that for different networks the number of NPUW's detected block may be different and it is hard to tell what the last block number will be. Now 'last' option can be used to tell NPUW to dump tail block without setting specific number.

dmatveev

Let's merge it now but please add tests as a follow-up. Thanks!

dmatveev · 2024-10-25T10:57:50Z

src/plugins/intel_npu/src/plugin/npuw/util.cpp

    std::vector<std::size_t> sub_inds{};
-    sub_inds = ::intel_npu ::OptionParser<std::vector<std::size_t>>::parse(opt);
+    sub_inds = ::intel_npu ::OptionParser<std::vector<std::size_t>>::parse(str);


It's better to cover ::parse() handle this case - it will be one place for all

### Details: - The problem is that for different networks the number of NPUW's detected block may be different and it is hard to tell what the last block number will be. Now 'last' option can be used to tell NPUW to dump tail block without setting specific number. ### Tickets: - E-139857

github-actions bot added category: NPU OpenVINO NPU plugin category: NPUW NPUW plugin labels Sep 27, 2024

eshiryae marked this pull request as ready for review October 4, 2024 12:54

eshiryae requested review from a team as code owners October 4, 2024 12:54

dmatveev self-assigned this Oct 8, 2024

dmatveev added this to the 2024.5 milestone Oct 8, 2024

eshiryae force-pushed the npuw_dumps_option_last branch from a34a1bc to 975ef80 Compare October 10, 2024 13:03

eshiryae requested review from dmatveev and AsyaPronina October 10, 2024 13:04

eshiryae force-pushed the npuw_dumps_option_last branch from 6ed03ee to 1b95b6f Compare October 10, 2024 13:31

AsyaPronina reviewed Oct 14, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp Outdated Show resolved Hide resolved

AsyaPronina reviewed Oct 14, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/util.hpp Outdated Show resolved Hide resolved

eshiryae force-pushed the npuw_dumps_option_last branch from 89c72f6 to 5fe82f9 Compare October 17, 2024 15:27

eshiryae requested a review from AsyaPronina October 18, 2024 10:30

AsyaPronina reviewed Oct 21, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp Outdated Show resolved Hide resolved

AsyaPronina approved these changes Oct 21, 2024

View reviewed changes

dmatveev reviewed Oct 21, 2024

View reviewed changes

eshiryae added 4 commits October 25, 2024 09:26

NPUW: Add 'last' keyword for NPUW_SUBMODEL_DEVICE

89314fd

Fix code style

6f485b8

Address comments and update options description

4911265

eshiryae force-pushed the npuw_dumps_option_last branch from 5fe82f9 to 8af1e7d Compare October 25, 2024 08:36

Address comments and fix typo in options description

8af1e7d

dmatveev approved these changes Oct 25, 2024

View reviewed changes

dmatveev enabled auto-merge October 25, 2024 10:58

dmatveev added this pull request to the merge queue Oct 25, 2024

Merged via the queue into openvinotoolkit:master with commit 8934f90 Oct 25, 2024
134 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NPUW: Introduce 'last' keyword for dump options #26841

NPUW: Introduce 'last' keyword for dump options #26841

eshiryae commented Sep 27, 2024

dmatveev commented Oct 10, 2024

AsyaPronina Oct 21, 2024

dmatveev Oct 21, 2024

eshiryae Oct 21, 2024 •

edited

Loading

AsyaPronina Oct 23, 2024

eshiryae Oct 25, 2024

dmatveev Oct 21, 2024

dmatveev Oct 21, 2024

eshiryae Oct 22, 2024

AsyaPronina Oct 23, 2024 •

edited

Loading

dmatveev Oct 24, 2024

eshiryae Oct 25, 2024

dmatveev Oct 21, 2024

eshiryae Oct 25, 2024

dmatveev Oct 21, 2024

AsyaPronina Oct 21, 2024

dmatveev Oct 24, 2024

AsyaPronina Oct 25, 2024

dmatveev Oct 21, 2024

AsyaPronina Oct 21, 2024

dmatveev left a comment

dmatveev Oct 25, 2024

NPUW: Introduce 'last' keyword for dump options #26841

NPUW: Introduce 'last' keyword for dump options #26841

Conversation

eshiryae commented Sep 27, 2024

Details:

Tickets:

dmatveev commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshiryae Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsyaPronina Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshiryae Oct 21, 2024 •

edited

Loading

AsyaPronina Oct 23, 2024 •

edited

Loading