[OV JS]: Expose ov.saveModel() functionality #27148

hub-bla · 2024-10-20T09:42:40Z

Details:

Created init_function to initialize functions directly in the addon.
Extended type_validation with Napi::Boolean
Exposed ov.saveModel
To make proper tests like in Python API I guess I have to wait for Model.getOps() to be exposed.

Tickets:

Closes - [Good First Issue]: Expose ov.saveModel() functionality #27092

almilosz

Thanks for taking up this task! It's not trivial. I have a few comments :)

almilosz · 2024-10-21T13:47:53Z

src/bindings/js/node/include/addon.hpp

@@ -18,6 +18,7 @@ struct AddonData {
    Napi::FunctionReference partial_shape;
    Napi::FunctionReference ppp;
    Napi::FunctionReference tensor;
+    Napi::FunctionReference save_model;


Those FunctionReferences keep the class prototypes functions and later are used for type validation. I don't see the usecase for save_model and propose to remove it from here and init_function

Thank you for the explanation. Makes sense now!

Quick question regarding init_function. After removing lines related to FunctionReference it will contain only this line

Napi::Function::New(env, func, func_name);

, so I propose removing init_function as well and replace it with the line above inside init_module.

I forgot that exports.Set is also necessary, so I'll keep init_function.

almilosz · 2024-10-21T13:52:49Z

src/bindings/js/node/lib/addon.ts

@@ -675,6 +675,16 @@ export interface NodeAddon {
    resizeAlgorithm: typeof resizeAlgorithm;
    PrePostProcessor: PrePostProcessorConstructor;
  };
+
+  /**
+   * It saves model in a specified path.


Suggested change

* It saves model in a specified path.

* It saves a model into IR files (xml and bin). Floating point weights are compressed to FP16 by default. This method saves a model to IR applying all necessary transformations that usually applied in model conversion flow provided by mo tool. Paricularly, floatting point weights are compressed to FP16, debug information in model nodes are cleaned up, etc.

almilosz · 2024-10-21T13:53:51Z

src/bindings/js/node/lib/addon.ts

+   * @param [model] Model that will be saved.
+   * @param [path] Path for saving the model.
+   * @param [compressToFp16] [OPTIONAL] Whether to compress the model
+   * to float16. Default is set to `true`.


Suggested change

* @param [model] Model that will be saved.

* @param [path] Path for saving the model.

* @param [compressToFp16] [OPTIONAL] Whether to compress the model

* to float16. Default is set to `true`.

* @param model The model which will be converted to IR representation and saved.

* @param path The path for saving the model.

* @param compressToFp16 Whether to compress floating point weights to FP16. Default is set to `true`.

almilosz · 2024-10-21T13:59:49Z

src/bindings/js/node/lib/addon.ts

+   * @param [compressToFp16] [OPTIONAL] Whether to compress the model
+   * to float16. Default is set to `true`.
+   */
+  saveModel(model:Model, path:string, compressToFp16?:boolean) : void;


Suggested change

saveModel(model:Model, path:string, compressToFp16?:boolean) : void;

saveModel(model: Model, path: string, compressToFp16?: boolean): void;

eslint does not fix everything unfortunately :/

almilosz · 2024-10-21T14:15:12Z

For now you don't have to include Model.getOps() method in tests. Please focus on other characteristics e.g. name, number of outputs etc.

vishniakov-nikolai · 2024-10-21T15:11:38Z

src/bindings/js/node/tests/unit/basic.test.js

 const epsilon = 0.5;
+const outDir = 'tests/unit/out';


Suggested change

const outDir = 'tests/unit/out';

const outDir = 'tests/unit/out/';

vishniakov-nikolai · 2024-10-21T15:11:57Z

src/bindings/js/node/tests/unit/basic.test.js

@@ -33,6 +34,24 @@ describe('ov basic tests.', () => {
    assert.ok(devices.includes('CPU'));
  });

+  describe('ov.saveModel()', () => {
+    it('saveModel(model:Model, path:string, compressToFp16:bool=true)', () => {
+      const xmlPath = `${outDir}/${modelLike[0].getName()}_fp16.xml`;


Suggested change

const xmlPath = `${outDir}/${modelLike[0].getName()}_fp16.xml`;

const xmlPath = `${outDir}${modelLike[0].getName()}_fp16.xml`;

vishniakov-nikolai · 2024-10-21T15:16:52Z

src/bindings/js/node/tests/unit/basic.test.js

+  describe('ov.saveModel()', () => {
+    it('saveModel(model:Model, path:string, compressToFp16:bool=true)', () => {
+      const xmlPath = `${outDir}/${modelLike[0].getName()}_fp16.xml`;
+      assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath));


Do you mean? (it('saveModel(model:Model, path:string, compressToFp16:bool=true)',)

Suggested change

assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath));

assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath, true));

vishniakov-nikolai · 2024-10-21T15:17:49Z

src/bindings/js/node/tests/unit/basic.test.js

+
+    it('saveModel(model:Model, path:string, compressToFp16:bool)', () => {
+      const xmlPath = `${outDir}/${modelLike[0].getName()}_fp32.xml`;
+      assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath, false));


Because false by default

Suggested change

assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath, false));

assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath));

vishniakov-nikolai · 2024-10-21T15:26:27Z

src/bindings/js/node/tests/unit/utils.js

+  let msg = '';
+  if (compareNames && model1.getFriendlyName() !== model2.getFriendlyName()) {
+    msg += 'Friendly names of models are not equal ';
+    msg += `model_one: ${model1.getFriendlyName()}, 
+           model_two: ${model2.getFriendlyName()}`;
+  }
+
+  if (model1.inputs.length !== model2.inputs.length) {
+    msg += 'Number of models\' inputs are not equal ';
+    msg += `model_one: ${model1.inputs.length}, `;
+    msg += `model_two: ${model2.inputs.length}`;
+    throw new Error(msg);
+  }
+
+  if (model1.outputs.length !== model2.outputs.length) {
+    msg += 'Number of models\' outputs are not equal ';
+    msg += `model_one: ${model1.outputs.length}, 
+           model_two: ${model2.outputs.length}`;
+    throw new Error(msg);
+  }


Didn't catch where compareNames using

Suggested change

let msg = '';

if (compareNames && model1.getFriendlyName() !== model2.getFriendlyName()) {

msg += 'Friendly names of models are not equal ';

msg += `model_one: ${model1.getFriendlyName()},

model_two: ${model2.getFriendlyName()}`;

}

if (model1.inputs.length !== model2.inputs.length) {

msg += 'Number of models\' inputs are not equal ';

msg += `model_one: ${model1.inputs.length}, `;

msg += `model_two: ${model2.inputs.length}`;

throw new Error(msg);

}

if (model1.outputs.length !== model2.outputs.length) {

msg += 'Number of models\' outputs are not equal ';

msg += `model_one: ${model1.outputs.length},

model_two: ${model2.outputs.length}`;

throw new Error(msg);

}

const deviations = [];

if (model1.getFriendlyName() !== model2.getFriendlyName())

deviations.push('Friendly names of models are not equal '

+ `model_one: ${model1.getFriendlyName()}, `

+ `model_two: ${model2.getFriendlyName()}`);

if (model1.inputs.length !== model2.inputs.length)

deviations.push('Number of models\' inputs are not equal '

+ `model_one: ${model1.inputs.length}, `

+ `model_two: ${model2.inputs.length}`);

if (model1.outputs.length !== model2.outputs.length)

deviations.push('Number of models\' outputs are not equal '

+ `model_one: ${model1.outputs.length}, `

+ `model_two: ${model2.outputs.length}`);

if (deviations.length) throw new Error(deviations.join('\n'));

vishniakov-nikolai · 2024-10-21T15:27:28Z

src/bindings/js/node/tests/unit/utils.js

@@ -2,6 +2,7 @@
 // Copyright (C) 2018-2024 Intel Corporation
 // SPDX-License-Identifier: Apache-2.0

+const { addon: ov } = require('../..');


I thought it would be handy in comparison function but it hasn't been necessary so far. I'll delete it.

vishniakov-nikolai · 2024-10-21T15:28:55Z

src/bindings/js/node/tests/unit/basic.test.js

+      const savedModel = core.readModelSync(xmlPath);
+      assert.doesNotThrow(() => compareModels(modelLike[0], savedModel));
+    });
+  });


Propose to add more test that check invalid parameters passing

Absolutely, it's just rough draft for now.

vishniakov-nikolai · 2024-10-21T15:30:04Z

src/bindings/js/node/src/addon.cpp

@@ -41,6 +68,8 @@ Napi::Object init_module(Napi::Env env, Napi::Object exports) {
    init_class(env, exports, "ConstOutput", &Output<const ov::Node>::get_class, addon_data->const_output);
    init_class(env, exports, "PartialShape", &PartialShapeWrap::get_class, addon_data->partial_shape);

+    init_function(env, exports, "saveModel", save_model, addon_data->save_model);


Function that blocks event loop must have postfix Sync:

Suggested change

init_function(env, exports, "saveModel", save_model, addon_data->save_model);

init_function(env, exports, "saveModelSync", save_model, addon_data->save_model);

Should I also make async version?

It is enough to have sync version as the solution of this issue

vishniakov-nikolai · 2024-10-21T15:30:27Z

src/bindings/js/node/src/addon.cpp

@@ -27,6 +30,30 @@ void init_class(Napi::Env env,
    exports.Set(class_name, prototype);
 }

+Napi::Value save_model(const Napi::CallbackInfo& info) {


Suggested change

Napi::Value save_model(const Napi::CallbackInfo& info) {

Napi::Value save_model_sync(const Napi::CallbackInfo& info) {

hub-bla · 2024-10-26T12:30:56Z

I’ve made the proposed changes and added tests to cover cases with invalid arguments. Let me know if there’s anything else you’d like me to adjust!

vishniakov-nikolai

Thanks!
LGTM, couple comments

src/bindings/js/node/lib/addon.ts

vishniakov-nikolai · 2024-10-29T10:13:45Z

src/bindings/js/node/tests/unit/basic.test.js

 const epsilon = 0.5;
+const outDir = 'tests/unit/out/';


Does after unit tests run git has clear state?

Good point, I believe I should add tests/unit/out to .gitignore, similar to how it was done for tests/unit/test_models path.

Please use mkdtemp(). The prefix can be simple. You may also see this usage of os.tempdir()

Adding files to .gitignore won't remove files in the CI's check. So you should make sure they are deleted

Co-authored-by: Vishniakov Nikolai <[email protected]>

almilosz · 2024-10-29T12:42:31Z

src/bindings/js/node/tests/unit/basic.test.js

 const epsilon = 0.5;
+const outDir = 'tests/unit/out/';


Please use mkdtemp(). The prefix can be simple. You may also see this usage of os.tempdir()

vishniakov-nikolai · 2024-10-30T13:22:39Z

@hub-bla to archive what Alicja suggested you may:

generate random folder name using fs.mkdtemp
put this folder by system tmp path (get by os.tmpdir())
use it to store saved model files
add after hook (const { after } = require('node:test');) into unit test to clean up temporary folder after tests execution despite test result

hub-bla · 2024-10-30T20:02:25Z

@almilosz @vishniakov-nikolai Thank you for clarification! I adjusted it based on the guidelines.

almilosz

Thanks for contribution!

vishniakov-nikolai · 2024-11-05T12:03:22Z

build_jenkins

mlukasze · 2024-11-06T06:52:00Z

build_jenkins

vishniakov-nikolai · 2024-11-07T16:57:48Z

Hi @hub-bla!
Something went wrong with tests: https://github.com/openvinotoolkit/openvino/actions/runs/11722356830/job/32653961578?pr=27148#step:6:33
Please, take a look.

Attached screenshot if you cannot see GHA pipeline logs:

This reverts commit 03015d0.

hub-bla · 2024-11-09T14:46:53Z

Hi @vishniakov-nikolai! It seems that the issue is specific to Windows. I initially tested it only on Ubuntu, as that’s the system I’m using. I’ve added a separate before function with mkdtemp, and if that doesn’t resolve the issue, I'll set it up on Windows to investigate further.

vishniakov-nikolai · 2024-11-15T17:29:20Z

src/bindings/js/node/tests/unit/basic.test.js

+  before(() => {
+    fs.mkdtemp(path.join(os.tmpdir(), 'ov_js_out_'), (err, directory) => {
+      if (err) {
+        throw err;
+      }
+      outDir = directory;
+    });
+  });


@hub-bla try to union two types of before in one.
Like:

before(async () => { outDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'ov_js_out_')); await isModelAvailable(testModels.testModelFP32); testXml = getModelPath().xml; });

It should resolve issue that happens on windows

mlukasze · 2024-11-18T06:34:08Z

build_jenkins

vishniakov-nikolai

There is still issue with model file removal

src/bindings/js/node/tests/unit/utils.js

src/bindings/js/node/tests/unit/basic.test.js

Co-authored-by: Vishniakov Nikolai <[email protected]>

src/bindings/js/node/tests/unit/utils.js

Co-authored-by: Vishniakov Nikolai <[email protected]>

vishniakov-nikolai · 2024-11-18T14:41:03Z

build_jenkins

### Details: - Created `init_function` to initialize functions directly in the addon. - Extended `type_validation` with `Napi::Boolean` - Exposed `ov.saveModel` - To make proper tests like in Python API I guess I have to wait for `Model.getOps()` to be exposed. ### Tickets: - Closes - openvinotoolkit#27092 --------- Co-authored-by: Vishniakov Nikolai <[email protected]> Co-authored-by: Michal Lukaszewski <[email protected]>

hub-bla added 3 commits October 20, 2024 07:44

expose save model

0450a2e

add bool type validation

e089757

add docs, simple tests, and type conversion

6d42728

hub-bla requested a review from a team as a code owner October 20, 2024 09:42

hub-bla marked this pull request as draft October 20, 2024 09:42

github-actions bot added the category: JS API OpenVino JS API Bindings label Oct 20, 2024

sys-openvino-ci added the ExternalPR External contributor label Oct 20, 2024

almilosz reviewed Oct 21, 2024

View reviewed changes

vishniakov-nikolai reviewed Oct 21, 2024

View reviewed changes

vishniakov-nikolai added this to the 2024.5 milestone Oct 21, 2024

hub-bla added 6 commits October 26, 2024 13:43

add sync postfix and remove save_model from class prototypes

f3a057d

update doc strings in addon.ts

95c180a

refacotor compareModels func

20aef41

update outDir path

2ba653e

fix eslint

29fb27f

add tests with invalid parameters and fix eslint

c6080a0

hub-bla marked this pull request as ready for review October 26, 2024 12:30

hub-bla requested review from vishniakov-nikolai and almilosz October 26, 2024 12:31

vishniakov-nikolai approved these changes Oct 29, 2024

View reviewed changes

Update src/bindings/js/node/lib/addon.ts

ce4bbc1

Co-authored-by: Vishniakov Nikolai <[email protected]>

almilosz requested changes Oct 29, 2024

View reviewed changes

generate temp directory

c8621f1

hub-bla requested a review from almilosz October 30, 2024 19:57

almilosz approved these changes Oct 31, 2024

View reviewed changes

mlukasze enabled auto-merge November 6, 2024 06:51

Merge branch 'master' into js-expose-save-model

2eb68fd

Merge branch 'master' into js-expose-save-model

fc7bfff

precede mkdtemp with await keyword

03015d0

auto-merge was automatically disabled November 9, 2024 14:13
Head branch was pushed to by a user without write access

hub-bla added 2 commits November 9, 2024 15:26

Revert "precede mkdtemp with await keyword"

3a16900

This reverts commit 03015d0.

remove console.log

677d6fd

vishniakov-nikolai reviewed Nov 15, 2024

View reviewed changes

use async version of mkdtemp

e813198

mlukasze enabled auto-merge November 18, 2024 06:34

mlukasze added this pull request to the merge queue Nov 18, 2024

vishniakov-nikolai reviewed Nov 18, 2024

View reviewed changes

src/bindings/js/node/tests/unit/utils.js Show resolved Hide resolved

src/bindings/js/node/tests/unit/utils.js Show resolved Hide resolved

src/bindings/js/node/tests/unit/basic.test.js Outdated Show resolved Hide resolved

src/bindings/js/node/tests/unit/basic.test.js Show resolved Hide resolved

vishniakov-nikolai removed this pull request from the merge queue due to a manual request Nov 18, 2024

hub-bla and others added 4 commits November 18, 2024 10:56

add sleep func

e1fe2f0

Co-authored-by: Vishniakov Nikolai <[email protected]>

export sleep

8cb2e42

Co-authored-by: Vishniakov Nikolai <[email protected]>

import sleep

7cbd825

Co-authored-by: Vishniakov Nikolai <[email protected]>

refactor after

8d302e6

Co-authored-by: Vishniakov Nikolai <[email protected]>

vishniakov-nikolai reviewed Nov 18, 2024

View reviewed changes

src/bindings/js/node/tests/unit/utils.js Show resolved Hide resolved

format sleep

5773d06

Co-authored-by: Vishniakov Nikolai <[email protected]>

vishniakov-nikolai enabled auto-merge November 18, 2024 14:40

vishniakov-nikolai added this pull request to the merge queue Nov 18, 2024

Merged via the queue into openvinotoolkit:master with commit 58672c3 Nov 18, 2024
170 checks passed

almilosz linked an issue Nov 19, 2024 that may be closed by this pull request

[Good First Issue]: Expose ov.saveModel() functionality #27092

Closed

	* It saves model in a specified path.
	* It saves a model into IR files (xml and bin). Floating point weights are compressed to FP16 by default. This method saves a model to IR applying all necessary transformations that usually applied in model conversion flow provided by mo tool. Paricularly, floatting point weights are compressed to FP16, debug information in model nodes are cleaned up, etc.

	saveModel(model:Model, path:string, compressToFp16?:boolean) : void;
	saveModel(model: Model, path: string, compressToFp16?: boolean): void;

	const outDir = 'tests/unit/out';
	const outDir = 'tests/unit/out/';

	const xmlPath = `${outDir}/${modelLike[0].getName()}_fp16.xml`;
	const xmlPath = `${outDir}${modelLike[0].getName()}_fp16.xml`;

	assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath));
	assert.doesNotThrow(() => ov.saveModel(modelLike[0], xmlPath, true));

	init_function(env, exports, "saveModel", save_model, addon_data->save_model);
	init_function(env, exports, "saveModelSync", save_model, addon_data->save_model);

	Napi::Value save_model(const Napi::CallbackInfo& info) {
	Napi::Value save_model_sync(const Napi::CallbackInfo& info) {

[OV JS]: Expose ov.saveModel() functionality #27148

[OV JS]: Expose ov.saveModel() functionality #27148

Conversation

hub-bla commented Oct 20, 2024

Details:

Tickets:

almilosz left a 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

almilosz commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishniakov-nikolai 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

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

hub-bla commented Oct 26, 2024

vishniakov-nikolai left a 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

almilosz Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishniakov-nikolai commented Oct 30, 2024

hub-bla commented Oct 30, 2024

almilosz left a comment

Choose a reason for hiding this comment

vishniakov-nikolai commented Nov 5, 2024

mlukasze commented Nov 6, 2024

vishniakov-nikolai commented Nov 7, 2024

hub-bla commented Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlukasze commented Nov 18, 2024

vishniakov-nikolai left a comment

Choose a reason for hiding this comment

vishniakov-nikolai commented Nov 18, 2024

vishniakov-nikolai Oct 21, 2024 •

edited

Loading

almilosz Oct 29, 2024 •

edited

Loading

hub-bla commented Nov 9, 2024 •

edited

Loading