[NPUW] Serialization fixes #28442

smirnov-alexey · 2025-01-14T18:35:26Z

No description provided.

…openvino into as/npuw_s11n_cfg

…into as/npuw_s11n_fixes

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

…into as/npuw_s11n_fixes

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

dmatveev · 2025-01-15T16:38:39Z

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

+    } else if (var.is<int>()) {
+        write(stream, int(AnyType::INT));
+        write(stream, var.as<int>());


this may be not portable, but let's keep it aside as we don't claim any blob portability at the moment.

dmatveev · 2025-01-15T16:41:18Z

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

+    } else if (type == AnyType::CHARS) {
+        std::string val;
+        read(stream, val);
+        var = std::move(val);


but didn't CHARS stand for const char* in Any when you serialized it?

I serialize it as std::string to keep it simple

dmatveev · 2025-01-15T16:42:31Z

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

    if (iter_device != device_bank.storage.end()) {
-        // Already allocated
+        // Shouldn't be possible
+        NPUW_ASSERT(false);
        return;
    }


It's a change in the contract here, what was OK previously is now not OK, can you please explain more here?

It occurs during bank deserialization. Our storage is an unordered_map with unique UIDs which all have been evaluated and serialized at this point. Thus it shouldn't be possible to encounter a duplicate UIDs during deserialization. Just a sanity check - this if can be removed altogether with no effect

dmatveev · 2025-01-15T16:43:47Z

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

@@ -237,6 +238,10 @@ void Bank::read_and_add_tensor(std::istream& stream, int64_t uid, const std::str
    ov::Tensor allocated_tensor;

    // FIXME: reading not via a dedicated function
+    bool is_intialized = false;
+    read(stream, is_intialized);
+    NPUW_ASSERT(is_intialized);


this too. Haven't you wrote empty tensors before?
if is_initialized must always be true, why write it to the stream at all?

I changed Tensor serialization in some of the previous PRs. It occurred when I was serializing empty scales/zerops. This change is just to align with the previous change

I don't get it. Do you serialize empty tensors now or not?
If you don't, you don't need this bool in the stream either.

If you do, this code breaks.

I do but not for the bank

So this code doesn't break

did I read this right you have a common routine to write a tensor to disk but you read it here in some specific way?
it doesn't sound good tbh.

you'd need to use the same function to read tensors as you write those. If that routine checks for false and does nothing, but you still need to raise an assert, you can make it in the caller code (= HERE) if the resulting tensor is still empty after read.

Discussed on call, this change is caused by optimization (reading from stream directly to the L0 buffer)

Discussed locally

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

dmatveev · 2025-01-15T16:51:00Z

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

+        if (properties.count(ov::cache_dir.name())) {
+            OPENVINO_THROW("Option 'CACHE_DIR' is not supported with NPU_USE_NPUW!");
+        }


As we do have the necessary tools in place now, let's plan it for 25.1

Yeah. This PR is just a support for LLMCompiledModel for now. We will figure out the rest later

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

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

…y/openvino into as/npuw_s11n_fixes

dmatveev · 2025-01-15T17:36:42Z

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

+        // CACHE_DIR isn't supported with NPU_USE_NPUW
+        if (properties.count(ov::cache_dir.name())) {
+            OPENVINO_THROW(
+                "Option 'CACHE_DIR' is not supported with configuration: NPU_USE_NPUW : YES, NPUW_LLM : NO!");


I won't be surprised if people who get this will start putting "NO!" to the config.

dmatveev · 2025-01-15T17:39:36Z

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

@@ -237,6 +238,10 @@ void Bank::read_and_add_tensor(std::istream& stream, int64_t uid, const std::str
    ov::Tensor allocated_tensor;

    // FIXME: reading not via a dedicated function
+    bool is_intialized = false;
+    read(stream, is_intialized);
+    NPUW_ASSERT(is_intialized);


did I read this right you have a common routine to write a tensor to disk but you read it here in some specific way?
it doesn't sound good tbh.

you'd need to use the same function to read tensors as you write those. If that routine checks for false and does nothing, but you still need to raise an assert, you can make it in the caller code (= HERE) if the resulting tensor is still empty after read.

Depends on openvinotoolkit/openvino#28442

smirnov-alexey and others added 6 commits January 13, 2025 20:43

Serialize LLMCompiled model config

839751b

Register properties

9cec59e

Fix tensor serialization

7eee003

Merge branch 'master' into as/npuw_s11n_cfg

1ebbb60

Merge branch 'as/npuw_s11n_cfg' of https://github.com/smirnov-alexey/…

e19ce2d

…openvino into as/npuw_s11n_cfg

Extend and fix s11n

9f033d7

smirnov-alexey added Code Freeze category: NPU OpenVINO NPU plugin category: NPUW NPUW plugin labels Jan 14, 2025

smirnov-alexey added this to the 2025.0 milestone Jan 14, 2025

smirnov-alexey requested review from dmatveev and AsyaPronina January 14, 2025 18:35

smirnov-alexey assigned dmatveev Jan 14, 2025

smirnov-alexey requested review from a team as code owners January 14, 2025 18:35

Merge branch 'master' of https://github.com/openvinotoolkit/openvino …

21a88d8

…into as/npuw_s11n_fixes

AsyaPronina reviewed Jan 14, 2025

View reviewed changes

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

dmatveev reviewed Jan 14, 2025

View reviewed changes

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

Merge branch 'master' of https://github.com/openvinotoolkit/openvino …

aeac6e1

…into as/npuw_s11n_fixes

dmatveev reviewed Jan 14, 2025

View reviewed changes

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

smirnov-alexey added 2 commits January 14, 2025 22:28

Support CACHE_DIR

7d1eb5d

Minor changes

e420198

moslex added the priority: high High piority label Jan 15, 2025

smirnov-alexey added 2 commits January 15, 2025 16:14

Additional fixes

d83f318

Clang

3f7a027

dmatveev reviewed Jan 15, 2025

View reviewed changes

AsyaPronina reviewed Jan 15, 2025

View reviewed changes

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

AsyaPronina reviewed Jan 15, 2025

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/serialization.cpp Show resolved Hide resolved

AsyaPronina reviewed Jan 15, 2025

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/serialization.cpp Show resolved Hide resolved

AsyaPronina approved these changes Jan 15, 2025

View reviewed changes

src/plugins/intel_npu/src/plugin/npuw/serialization.cpp Show resolved Hide resolved

smirnov-alexey added 3 commits January 15, 2025 17:15

Merge branch 'master' into as/npuw_s11n_fixes

74d439d

Address review comments

b22ac08

Merge branch 'as/npuw_s11n_fixes' of https://github.com/smirnov-alexe…

0e582c1

…y/openvino into as/npuw_s11n_fixes

dmatveev reviewed Jan 15, 2025

View reviewed changes

Address review comment

9cfba10

dmatveev approved these changes Jan 15, 2025

View reviewed changes

dmatveev enabled auto-merge January 15, 2025 18:03

Clang

1710f24

dmatveev mentioned this pull request Jan 15, 2025

[LLM] [NPU] StaticLLMPipeline: Enable CACHE_DIR openvinotoolkit/openvino.genai#1558

Merged

dmatveev added this pull request to the merge queue Jan 15, 2025

Merged via the queue into openvinotoolkit:master with commit 40b19c8 Jan 15, 2025
161 checks passed

dmatveev deleted the as/npuw_s11n_fixes branch January 15, 2025 21:34

github-merge-queue bot pushed a commit to openvinotoolkit/openvino.genai that referenced this pull request Jan 15, 2025

[LLM] [NPU] StaticLLMPipeline: Enable CACHE_DIR (#1558)

8ea1414

Depends on openvinotoolkit/openvino#28442

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NPUW] Serialization fixes #28442

[NPUW] Serialization fixes #28442

smirnov-alexey commented Jan 14, 2025

dmatveev Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

smirnov-alexey Jan 15, 2025

dmatveev Jan 15, 2025

[NPUW] Serialization fixes #28442

[NPUW] Serialization fixes #28442

Conversation

smirnov-alexey commented Jan 14, 2025

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

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