[NPU] Blob versioning #27159

alexandruenache1111 · 2024-10-21T10:58:17Z

Details:

A versioned blob would look like this:

Where Blob data size is necessary when importing the blob to offset jump at Metadata.

Rules of thumb when we need to set a new Metadata version:

if we add fields at the end of current Metadata, then Minor is incremented. This means we maintain backward compatibility with old blobs.

Example:
After OV Version we add a new field.

if we remove fields or add fields anywhere but at end, then Major is incremented. This means we break forward compatibility with old blobs.

Examples:
Between Minor and OV Version, we add a new field.
OR
We remove OV Version.

Simplified workflow:

at export: we append the versioning stuff at the end of the blob
at import: we extract the stored Metadata and verify it against the currently supported one. If the imported blob is incompatible, we reject it and print its OV version (where possible).

Tickets:

E-135371

src/plugins/intel_npu/src/plugin/src/model_version.cpp

src/plugins/intel_npu/src/plugin/include/model_version.hpp

src/plugins/intel_npu/src/plugin/src/model_version.cpp

src/plugins/intel_npu/src/plugin/include/metadata.hpp

src/plugins/intel_npu/tests/unit/npu/metadata_version.cpp

StefaniaHergane

Small renaming suggestion for createMetadata().

src/plugins/intel_npu/src/plugin/src/metadata.cpp

src/plugins/intel_npu/tests/unit/npu/metadata_version.cpp

src/plugins/intel_npu/src/plugin/include/metadata.hpp

sbutnari · 2024-11-29T12:29:40Z

build_jenkins

src/plugins/intel_npu/src/plugin/include/metadata.hpp

PatrikStepan · 2024-12-05T16:10:06Z

src/plugins/intel_npu/src/plugin/include/metadata.hpp

+
+    virtual void set_version(uint32_t newVersion) = 0;
+
+    virtual void set_ov_version(OpenvinoVersion& newVersion) = 0;


Do we need this in the base class? I only see it used in a test, where we don't need a reference to the base class.

PatrikStepan · 2024-12-05T16:13:57Z

src/plugins/intel_npu/src/plugin/src/metadata.cpp

+
+void OpenvinoVersion::read(std::istream& stream) {
+    stream.read(reinterpret_cast<char*>(&size), sizeof(size));
+    stream.read(&version[0], size);


No resize before this?

I tried doing it in the past but it impacted the run-time by about 200ms, so I took it out. But, having it tried again now, I don't notice any slowness.

Then I will add it back.

razvanapetroaie · 2024-12-05T14:35:11Z

src/plugins/intel_npu/src/plugin/include/metadata.hpp

+/**
+ * @brief Returns a uint32_t value which represents two uint16_t values concatenated.
+ *
+ * @param major Major version.


Kudos for attempting to document everything, but I find little utility in attaching obvious descriptions to parameters (still, it's better than no documentation at all). For instance, maybe here you could elaborate what the major/minor version represent or even the difference between them (the one established by convention).

Thanks, but not really what I was looking for. First, you may place more verbose descriptions alongside "param" fields as well, otherwise their usefulness is quite low. Second, my comment served as an encouragement to do the same for all function definitions, not only this one.

This stuff holds little-to-no importance anyway, so feel free to disregard it.

src/plugins/intel_npu/src/plugin/src/metadata.cpp

src/plugins/intel_npu/src/plugin/include/metadata.hpp

razvanapetroaie · 2024-12-05T15:15:45Z

src/plugins/intel_npu/src/plugin/src/metadata.cpp

+}
+
+void OpenvinoVersion::read(std::istream& stream) {
+    stream.read(reinterpret_cast<char*>(&size), sizeof(size));


Suggested change

stream.read(reinterpret_cast<char*>(&size), sizeof(size));

stream >> size;

Doesn't this work the same way, in a more C++ way? Please double check if you agree, I'm not 100% sure.

Sadly, it does not. The >> operator writes any value as a string. This is why we need to use the & operator to access the data from the memory in binary representation. This also covers the problem of the size being read for each field inside the blob.

To me it sounds like the issue is you've copied the data from the original stream (a file stream) into your own stringstream in a binary manner. Avoiding to create your own intermediate stream as suggested in other comments should remove this constraint.

@lmielick Just asking for one more opinion. Do you see any issue if we read/write some multi-byte variables in a raw binary manner? Regarding cross-platform compatibility, here we're using data types of fixed byte size, and I'm not aware of any requirement asking us to support big-endian platforms. Also wondering if the big-endian - little-endian compatibility isn't already broken due to the ELF part of the blob.

We're trying to decide whethere to use the read/write or <</>> stream operators in order to attach plugin metdata to the blob. The latter would cost a little bit more storage due to its string-like format, the cost may increase if we decide to expand this feature.

src/plugins/intel_npu/src/plugin/src/metadata.cpp

razvanapetroaie · 2024-12-05T16:43:58Z

src/plugins/intel_npu/src/plugin/src/metadata.cpp

+    return storedMeta;
+}
+
+void Metadata<METADATA_VERSION_1_0>::set_version(uint32_t newVersion) {


Any reason to offer a setter for this?

For my unit tests, since the field is private.

Not a fan of modeling the source code in order to make tests implementation more convenient. Instead, will it work if you define Metadata<DUMMY_VERSION> in the tests file, and have the ctor place an invalid version there?

razvanapetroaie · 2024-12-05T16:44:19Z

src/plugins/intel_npu/src/plugin/src/metadata.cpp

+    version = newVersion;
+}
+
+void Metadata<METADATA_VERSION_1_0>::set_ov_version(OpenvinoVersion& newVersion) {


Same as above.

Same answer as above.

razvanapetroaie · 2024-12-05T16:54:45Z

src/plugins/intel_npu/src/plugin/include/metadata.hpp

+ * @param minor Minor version.
+ * @return Major and minor versions concatenated into one single uint32_t value.
+ */
+constexpr uint32_t make_version(uint16_t major, uint16_t minor) {


Not sure placing constexprhere makes any sense. Could you also consider making this function a static method inside the MetadataBase class? In order to avoid namespace polution. You'll also need to move the version attribute there, but I believe that also makes sense.

Same about the mejor and minor functions below.

I chose constexpr to force the compiler a bit to evaluate the function at compile-time.

But what if I moved those functions outside the namespace?

But what if I moved those functions outside the namespace?

Then you're polluting the global namespace for every other file importing the current one. By "polluting" I mean any other file that intends to use or reimplement a function bearing the same name could run into trouble. "make_version" is not a rare name.

That's why I suggest using a private namespace since this file is the only client (excluding the unit tests, we should not model our source code just to make tests implementation slightly more convenient). Or place it as a static method as mentioned previously, avoids pollution and makes the functions inline.

razvanapetroaie · 2024-12-05T17:11:22Z

src/plugins/intel_npu/src/plugin/src/metadata.cpp

+                         blob.end() - metadataIterator - sizeof(blobDataSize));
+
+    uint32_t metaVersion;
+    metadataStream.read(reinterpret_cast<char*>(&metaVersion), sizeof(metaVersion));


Should we check the major version is compatible before attempting to read the rest?

Also, why you saying in PR's description that removing the OV version would break backward compatibility with the old blobs? Did you actually mean forward compatibility? I.e. old plugin + new blob missing this field.

The major version is already checked inside create_metadata. This implies that any metadata appended to the blob, has its version as the first field.

And yes, I meant forward compatibility in the PR description. I will update it.

Then shouldn't we reinforce forward compatibility in the code as well? As it stands, you're throwing if the plugin sees version 1.1 (a future metadata implementation), but you're description suggests this scenario should work.

razvanapetroaie · 2024-12-05T17:13:04Z

src/plugins/intel_npu/tests/unit/CMakeLists.txt

@@ -29,6 +29,8 @@ ov_add_test_target(
            ${OpenVINO_SOURCE_DIR}/src/plugins/intel_npu/src/utils/include
            ${OpenVINO_SOURCE_DIR}/src/plugins/intel_npu/src/plugin/include
            ${OpenVINO_SOURCE_DIR}/src/plugins/intel_npu/src/al/include
+        OBJECT_FILES
+            ${OpenVINO_SOURCE_DIR}/src/plugins/intel_npu/src/plugin/src/metadata.cpp


My CMake skills suck but I gotta ask: why did we need this? Asking cause it looks a bit out-of-the-blue to me.

The source files from intel_npu/src/plugin/src are not compiled into a static library. That means I cannot add them to the unit tests, without doing some stuff in our root CMakeFiles.
Another solution would imply moving metadata.cpp/hpp to common dir which is already compiled into a static lib.

But, the quickest solution was to hardcode the needed source file for my PR.

@pereanub Could you please check everything is fine here?

Signed-off-by: Alexandru Enache <[email protected]>

github-actions bot added the category: NPU OpenVINO NPU plugin label Oct 21, 2024

alexandruenache1111 self-assigned this Oct 25, 2024

alexandruenache1111 marked this pull request as ready for review October 25, 2024 09:50

alexandruenache1111 requested review from a team as code owners October 25, 2024 09:50

alexandruenache1111 marked this pull request as draft November 11, 2024 17:12

alexandruenache1111 force-pushed the blob_stream_commit branch 2 times, most recently from aaba4a6 to 421d071 Compare November 12, 2024 06:45

alexandruenache1111 marked this pull request as ready for review November 12, 2024 06:46

pereanub reviewed Nov 12, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/src/model_version.cpp Outdated Show resolved Hide resolved

pereanub reviewed Nov 13, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/include/model_version.hpp Outdated Show resolved Hide resolved

src/plugins/intel_npu/src/plugin/src/model_version.cpp Outdated Show resolved Hide resolved

src/plugins/intel_npu/src/plugin/src/model_version.cpp Outdated Show resolved Hide resolved

pereanub reviewed Nov 22, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/include/metadata.hpp Outdated Show resolved Hide resolved

src/plugins/intel_npu/src/plugin/include/metadata.hpp Outdated Show resolved Hide resolved

github-actions bot added the category: build OpenVINO cmake script / infra label Nov 28, 2024

sbutnari reviewed Nov 28, 2024

View reviewed changes

src/plugins/intel_npu/tests/unit/npu/metadata_version.cpp Outdated Show resolved Hide resolved

StefaniaHergane reviewed Nov 28, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/src/metadata.cpp Outdated Show resolved Hide resolved

StefaniaHergane reviewed Nov 29, 2024

View reviewed changes

src/plugins/intel_npu/tests/unit/npu/metadata_version.cpp Outdated Show resolved Hide resolved

alexandruenache1111 force-pushed the blob_stream_commit branch from a49bbba to f1a9f1c Compare November 29, 2024 09:41

pereanub reviewed Nov 29, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/include/metadata.hpp Outdated Show resolved Hide resolved

src/plugins/intel_npu/src/plugin/include/metadata.hpp Outdated Show resolved Hide resolved

alexandruenache1111 requested a review from pereanub December 4, 2024 11:30

alexandruenache1111 force-pushed the blob_stream_commit branch from cb04bf6 to 4832272 Compare December 4, 2024 14:11

PatrikStepan reviewed Dec 5, 2024

View reviewed changes

src/plugins/intel_npu/src/plugin/include/metadata.hpp Outdated Show resolved Hide resolved

PatrikStepan reviewed Dec 5, 2024

View reviewed changes

razvanapetroaie reviewed Dec 5, 2024

View reviewed changes

alexandruenache1111 requested review from a team as code owners December 6, 2024 08:10

github-actions bot added category: tools OpenVINO C++ / Python tools category: OVC OVC tool labels Dec 6, 2024

alexandruenache1111 removed the request for review from a team December 6, 2024 08:12

alexandruenache1111 force-pushed the blob_stream_commit branch from 92c2080 to 448ad5b Compare December 6, 2024 08:13

github-actions bot removed category: tools OpenVINO C++ / Python tools category: OVC OVC tool labels Dec 6, 2024

alexandruenache1111 requested review from lmielick and razvanapetroaie December 9, 2024 14:34

alexandruenache1111 force-pushed the blob_stream_commit branch from 1eb68e2 to 8fb327c Compare December 16, 2024 09:36

Squash commits from rebase

9284582

Signed-off-by: Alexandru Enache <[email protected]>

alexandruenache1111 force-pushed the blob_stream_commit branch from 8fb327c to 9284582 Compare December 16, 2024 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NPU] Blob versioning #27159

[NPU] Blob versioning #27159

alexandruenache1111 commented Oct 21, 2024 •

edited

Loading

StefaniaHergane left a comment

sbutnari commented Nov 29, 2024

PatrikStepan Dec 5, 2024

alexandruenache1111 Dec 6, 2024

PatrikStepan Dec 5, 2024

alexandruenache1111 Dec 6, 2024

alexandruenache1111 Dec 6, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 10, 2024

razvanapetroaie Dec 10, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 6, 2024

razvanapetroaie Dec 10, 2024 •

edited

Loading

razvanapetroaie Dec 12, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 6, 2024

razvanapetroaie Dec 10, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 6, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 9, 2024

razvanapetroaie Dec 10, 2024

razvanapetroaie Dec 5, 2024

alexandruenache1111 Dec 9, 2024 •

edited

Loading

razvanapetroaie Dec 10, 2024

razvanapetroaie Dec 5, 2024 •

edited

Loading

alexandruenache1111 Dec 6, 2024

razvanapetroaie Dec 10, 2024


		virtual void set_version(uint32_t newVersion) = 0;

		virtual void set_ov_version(OpenvinoVersion& newVersion) = 0;

	stream.read(reinterpret_cast<char*>(&size), sizeof(size));
	stream >> size;

[NPU] Blob versioning #27159

Are you sure you want to change the base?

[NPU] Blob versioning #27159

Conversation

alexandruenache1111 commented Oct 21, 2024 • edited Loading

Details:

Tickets:

StefaniaHergane left a comment

Choose a reason for hiding this comment

sbutnari commented Nov 29, 2024

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

razvanapetroaie Dec 10, 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

alexandruenache1111 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razvanapetroaie Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruenache1111 commented Oct 21, 2024 •

edited

Loading

razvanapetroaie Dec 10, 2024 •

edited

Loading

alexandruenache1111 Dec 9, 2024 •

edited

Loading

razvanapetroaie Dec 5, 2024 •

edited

Loading