Added Tensor.get_size() method to Node.js API #23498

qxprakash · 2024-03-16T16:54:04Z

This Fixes #23440

Details:

Extended Tensor API , with tensor.getSize() method

Implemented tensor.getSize() in the js api
added parameter validation
updated Tensor interface with the getSize() method in addon.ts
added Tests

qxprakash · 2024-03-16T17:00:04Z

hello @vishniakov-nikolai , @almilosz can you please review ? let me know if any changes are required.

almilosz · 2024-03-20T06:42:45Z

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

@@ -51,6 +51,9 @@ class TensorWrap : public Napi::ObjectWrap<TensorWrap> {
    Napi::Value get_shape(const Napi::CallbackInfo& info);
    /** @return Napi::String containing ov::element type. */
    Napi::Value get_element_type(const Napi::CallbackInfo& info);
+    /**@return Napi::Number containing tensor size*/


Suggested change

/**@return Napi::Number containing tensor size*/

/**@return Napi::Number containing tensor size as total number of elements.*/

@almilosz Thanks for the feedback , I have made the requested changes , can you take a look ?

almilosz

The TensorWrap::get_size() method has to be hooked up to the accessor when defining a class in the TensorWrap::get_class().

qxprakash · 2024-03-20T10:22:46Z

The TensorWrap::get_size() method has to be hooked up to the accessor when defining a class in the TensorWrap::get_class().

@almilosz thanks for the feedback , sorry it was a mistake from my end , actually I complied and tested the changes in a seperate local repo , but while making changes for PR in a newly cloned repo , I missed to add the get_size() in TensorWrap::get_class().

almilosz · 2024-03-20T14:00:44Z

src/bindings/js/node/src/tensor.cpp

+        reportError(info.Env(), "getSize() does not accept any arguments.");
+        return info.Env().Null();
+    }
+    size_t size = _tensor.get_size();


Suggested change

size_t size = _tensor.get_size();

const auto size = static_cast<double>(_tensor.get_size());

almilosz · 2024-03-20T14:19:28Z

src/bindings/js/node/src/tensor.cpp

+    }
+    size_t size = _tensor.get_size();
+
+    double jsSize = static_cast<double>(size);


Use snake_case in c++ part

@almilosz Thanks , for the feedback I have made the suggested changes.

almilosz

Looks good! Minor comments

qxprakash · 2024-03-21T15:48:14Z

Looks good! Minor comments

@almilosz Thanks for your feedback , I have made the suggested changes.

qxprakash · 2024-03-22T09:05:35Z

hello @almilosz Code , Style / clang-format test failed , looks like a token issue ?

almilosz

You need to run clang_format_fix_all to fix CI check.
There are some extra empty lines.

qxprakash · 2024-03-22T15:00:40Z

Hi @almilosz , Thanks for the feedback , I have removed empty lines can you please re-run the tests again ? I am on ubuntu 22.04 lts , I was not able to install clang-format-9 which was dependency for clang_format_fix_all.

qxprakash · 2024-03-23T11:54:02Z

Hey @almilosz I set up ubuntu 20.04 env inside a dev container , and was able to successfully ran clang_format_fix_all on my latest commit , I see no diffs , which means the tests should pass now right ?

here is the ouput

root ➜ .../ubuntu/openvino-fork/openvino/build (implement_getSize) $ cmake --build . --target clang_format_fix_all
[0%] [clang-format] openvino_template_extension_clang_fix
[ 0%] Built target openvino_template_extension_clang_fix
[ 0%] [clang-format] openvino_runtime_dev_clang_fix
[ 0%] Built target openvino_runtime_dev_clang_fix
[ 0%] [clang-format] openvino_itt_clang_fix
[ 0%] Built target openvino_itt_clang_fix
[ 0%] [clang-format] openvino_conditional_compilation_clang_fix
[ 0%] Built target openvino_conditional_compilation_clang_fix
[ 0%] [clang-format] openvino_util_clang_fix
[ 0%] Built target openvino_util_clang_fix
[ 0%] [clang-format] openvino_transformations_clang_fix
[ 0%] Built target openvino_transformations_clang_fix
[ 0%] [clang-format] openvino_offline_transformations_clang_fix
[ 0%] Built target openvino_offline_transformations_clang_fix
[ 0%] [clang-format] openvino_core_clang_fix
[ 0%] Built target openvino_core_clang_fix
[ 0%] [clang-format] openvino_reference_clang_fix
[ 0%] Built target openvino_reference_clang_fix
[ 0%] [clang-format] openvino_shape_inference_clang_fix
[ 0%] Built target openvino_shape_inference_clang_fix
[ 0%] [clang-format] openvino_frontend_common_clang_fix
[ 0%] Built target openvino_frontend_common_clang_fix
[ 33%] [clang-format] openvino_ir_frontend_clang_fix
[ 33%] Built target openvino_ir_frontend_clang_fix
[ 33%] [clang-format] openvino_onnx_common_clang_fix
[ 33%] Built target openvino_onnx_common_clang_fix
[ 66%] [clang-format] openvino_onnx_frontend_clang_fix
[ 66%] Built target openvino_onnx_frontend_clang_fix
[ 66%] [clang-format] openvino_pytorch_frontend_clang_fix
[ 66%] Built target openvino_pytorch_frontend_clang_fix
[ 66%] [clang-format] openvino_tensorflow_common_clang_fix
[ 66%] Built target openvino_tensorflow_common_clang_fix
[ 66%] [clang-format] openvino_tensorflow_frontend_clang_fix
[ 66%] Built target openvino_tensorflow_frontend_clang_fix
[ 66%] [clang-format] openvino_tensorflow_lite_frontend_clang_fix
[ 66%] Built target openvino_tensorflow_lite_frontend_clang_fix
[ 66%] [clang-format] openvino_paddle_frontend_clang_fix
[ 66%] Built target openvino_paddle_frontend_clang_fix
[ 66%] [clang-format] openvino_auto_batch_plugin_clang_fix
[ 66%] Built target openvino_auto_batch_plugin_clang_fix
[ 66%] [clang-format] openvino_hetero_plugin_clang_fix
[ 66%] Built target openvino_hetero_plugin_clang_fix
[ 66%] [clang-format] openvino_proxy_plugin_obj_clang_fix
[ 66%] Built target openvino_proxy_plugin_obj_clang_fix
[ 66%] [clang-format] openvino_runtime_clang_fix
[ 66%] Built target openvino_runtime_clang_fix
[ 66%] [clang-format] openvino_c_clang_fix
[ 66%] Built target openvino_c_clang_fix
[ 66%] [clang-format] ov_node_addon_clang_fix
[ 66%] Built target ov_node_addon_clang_fix
[ 66%] [clang-format] ie_samples_utils_clang_fix
[ 66%] Built target ie_samples_utils_clang_fix
[ 66%] [clang-format] format_reader_clang_fix
[ 66%] Built target format_reader_clang_fix
[ 66%] [clang-format] sync_benchmark_clang_fix
[ 66%] Built target sync_benchmark_clang_fix
[100%] [clang-format] throughput_benchmark_clang_fix
[100%] Built target throughput_benchmark_clang_fix
[100%] [clang-format] benchmark_app_clang_fix
[100%] Built target benchmark_app_clang_fix
[100%] [clang-format] classification_sample_async_clang_fix
[100%] Built target classification_sample_async_clang_fix
[100%] [clang-format] hello_classification_clang_fix
[100%] Built target hello_classification_clang_fix
[100%] [clang-format] hello_nv12_input_classification_clang_fix
[100%] Built target hello_nv12_input_classification_clang_fix
[100%] [clang-format] hello_query_device_clang_fix
[100%] Built target hello_query_device_clang_fix
[100%] [clang-format] hello_reshape_ssd_clang_fix
[100%] Built target hello_reshape_ssd_clang_fix
[100%] [clang-format] model_creation_sample_clang_fix
[100%] Built target model_creation_sample_clang_fix
[100%] [clang-format] opencv_c_wrapper_clang_fix
[100%] Built target opencv_c_wrapper_clang_fix
[100%] [clang-format] hello_classification_c_clang_fix
[100%] Built target hello_classification_c_clang_fix
[100%] [clang-format] hello_nv12_input_classification_c_clang_fix
[100%] Built target hello_nv12_input_classification_c_clang_fix
[100%] [clang-format] openvino_interpreter_backend_clang_fix
[100%] Built target openvino_interpreter_backend_clang_fix
[100%] [clang-format] openvino_template_plugin_clang_fix
[100%] Built target openvino_template_plugin_clang_fix
[100%] Built target clang_format_fix_all
`

vishniakov-nikolai · 2024-03-25T17:07:52Z

src/bindings/js/node/src/tensor.cpp

+        reportError(info.Env(), "getSize() does not accept any arguments.");
+        return info.Env().Null();
+    }
+    const auto size = static_cast<double>(_tensor.get_size());


Why does it cast to double, not uint?

Hi @vishniakov-nikolai thanks for your feedback , my reasoning behind casting into double Type was because of it's close alignment with Number Type in JS.

vishniakov-nikolai · 2024-03-25T17:11:15Z

src/bindings/js/node/src/tensor.cpp

+
+Napi::Value TensorWrap::get_size(const Napi::CallbackInfo& info) {
+    if (info.Length() > 0) {
+        reportError(info.Env(), "getSize() does not accept any arguments.");


You can extract info.Env() into variable if you use it several times.

vishniakov-nikolai · 2024-03-25T17:20:12Z

src/bindings/js/node/src/tensor.cpp

+Napi::Value TensorWrap::get_size(const Napi::CallbackInfo& info) {
+    if (info.Length() > 0) {
+        reportError(info.Env(), "getSize() does not accept any arguments.");
+        return info.Env().Null();


I propose return info.Env().Undefined() in case of error. (I know that in the sources you can meet both variants, but it could be aligned later).

vishniakov-nikolai · 2024-03-25T17:25:18Z

src/bindings/js/node/tests/tensor.test.js

+  it('calculates size correctly for a small 3D tensor [2, 5, 5]', () => {
+    const shape = [2, 5, 5];
+    const expectedSize = 2*5*5;
+    const tensorData = new Float32Array(expectedSize).fill(0);
+    const tensor = new ov.Tensor(ov.element.f32, shape, tensorData);
+    assert.strictEqual(tensor.getSize(), expectedSize);
+  });


I really like that you've implemented great tests for implemented functionality. Thank you!
But I don't see difference between this test and calculates size correctly for a common image data shape [3, 224, 224]. I propose to remove current test.

Ok @vishniakov-nikolai

vishniakov-nikolai · 2024-03-25T17:26:00Z

src/bindings/js/node/tests/tensor.test.js

+    assert.strictEqual(tensor.getSize(), expectedSize);
+  });
+
+  it('calculates size correctly for a small square matrix [4, 4]', () => {


This is also extra, I propose to remove it.

vishniakov-nikolai · 2024-03-25T17:26:21Z

src/bindings/js/node/tests/tensor.test.js

+    assert.strictEqual(tensor.getSize(), expectedSize);
+  });
+
+  it('calculates size correctly for another small 3D tensor [2, 3, 3]', () => {


And this one, I propose to remove it.

qxprakash · 2024-03-25T18:21:25Z

@vishniakov-nikolai Thanks for your feedback I have made all the proposed changes , except typecasting , into uint , I have stated my reasons for it in the reply to your comment , please let me know if I need to change it.

Best regards,
Prakash

vishniakov-nikolai

Looks great! Let's wait for green checks and then merge it.

qxprakash · 2024-03-26T16:55:01Z

@vishniakov-nikolai Android ARM64 with vcpkg / Build (pull_request) and Android ARM64 with vcpkg / ci/gha_overall_status_android are failing , was this bound to happen or something went wrong ? how can fix this ? :(

mlukasze · 2024-03-27T12:43:06Z

build_jenkins

### This Fixes openvinotoolkit#23440 ### Details: Extended Tensor API , with tensor.getSize() method - Implemented tensor.getSize() in the js api - added parameter validation - updated Tensor interface with the getSize() method in addon.ts - added Tests --------- Co-authored-by: Alicja Miloszewska <alicja.miloszewska@intel.com> Co-authored-by: Vishniakov Nikolai <nikolai.vishniakov@intel.com>

implmented tensor.getSize()

22464be

qxprakash requested a review from a team as a code owner March 16, 2024 16:54

github-actions bot added the category: JS API OpenVino JS API Bindings label Mar 16, 2024

qxprakash changed the title ~~implmented tensor.getSize()~~ Expose Tensor.get_size() method to Node.js API Mar 16, 2024

qxprakash changed the title ~~Expose Tensor.get_size() method to Node.js API~~ Added Tensor.get_size() method to Node.js API Mar 16, 2024

rkazants assigned vishniakov-nikolai Mar 17, 2024

rkazants added the ExternalPR External contributor label Mar 17, 2024

almilosz reviewed Mar 20, 2024

View reviewed changes

almilosz requested changes Mar 20, 2024

View reviewed changes

qxprakash added 2 commits March 20, 2024 15:46

added get_size as instance method in TensorWrap::get_class()

497d0e7

updated comment in tensor.hpp

18177fa

almilosz mentioned this pull request Mar 20, 2024

[Good First Issue]: Expose Model.is_dynamic() method to Node.js API #23565

Closed

almilosz reviewed Mar 20, 2024

View reviewed changes

suggested refactors

81a1212

almilosz approved these changes Mar 21, 2024

View reviewed changes

Merge branch 'master' into implement_getSize

c2bed82

almilosz approved these changes Mar 22, 2024

View reviewed changes

removed empty lines

78012bf

qxprakash requested a review from almilosz March 23, 2024 10:23

almilosz added 2 commits March 25, 2024 08:59

Merge branch 'master' into implement_getSize

43cbfad

Merge branch 'master' into implement_getSize

f9507d0

vishniakov-nikolai reviewed Mar 25, 2024

View reviewed changes

made proposed changes to tensor.cpp and tensor.test.js

c4751cf

qxprakash requested a review from vishniakov-nikolai March 25, 2024 19:21

vishniakov-nikolai approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'master' into implement_getSize

06d9dcf

almilosz approved these changes Mar 27, 2024

View reviewed changes

almilosz added this pull request to the merge queue Mar 28, 2024

Merged via the queue into openvinotoolkit:master with commit 496a5de Mar 28, 2024
108 checks passed

mlukasze added this to the 2024.1 milestone Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Tensor.get_size() method to Node.js API #23498

Added Tensor.get_size() method to Node.js API #23498

qxprakash commented Mar 16, 2024 •

edited

Loading

qxprakash commented Mar 16, 2024 •

edited

Loading

almilosz Mar 20, 2024 •

edited

Loading

qxprakash Mar 20, 2024 •

edited

Loading

almilosz left a comment

qxprakash commented Mar 20, 2024

almilosz Mar 20, 2024 •

edited

Loading

almilosz Mar 20, 2024

qxprakash Mar 20, 2024

almilosz left a comment

qxprakash commented Mar 21, 2024

qxprakash commented Mar 22, 2024 •

edited

Loading

almilosz left a comment •

edited

Loading

qxprakash commented Mar 22, 2024

qxprakash commented Mar 23, 2024 •

edited

Loading

vishniakov-nikolai Mar 25, 2024

qxprakash Mar 25, 2024

vishniakov-nikolai Mar 25, 2024

vishniakov-nikolai Mar 25, 2024

vishniakov-nikolai Mar 25, 2024

qxprakash Mar 25, 2024

vishniakov-nikolai Mar 25, 2024

vishniakov-nikolai Mar 25, 2024

qxprakash commented Mar 25, 2024

vishniakov-nikolai left a comment

qxprakash commented Mar 26, 2024 •

edited

Loading

mlukasze commented Mar 27, 2024

	/*@return Napi::Number containing tensor size/

	/*@return Napi::Number containing tensor size as total number of elements./

	size_t size = _tensor.get_size();
	const auto size = static_cast<double>(_tensor.get_size());

Added Tensor.get_size() method to Node.js API #23498

Added Tensor.get_size() method to Node.js API #23498

Conversation

qxprakash commented Mar 16, 2024 • edited Loading

This Fixes #23440

Details:

qxprakash commented Mar 16, 2024 • edited Loading

almilosz Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

qxprakash Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

almilosz left a comment

Choose a reason for hiding this comment

qxprakash commented Mar 20, 2024

almilosz Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almilosz left a comment

Choose a reason for hiding this comment

qxprakash commented Mar 21, 2024

qxprakash commented Mar 22, 2024 • edited Loading

almilosz left a comment • edited Loading

Choose a reason for hiding this comment

qxprakash commented Mar 22, 2024

qxprakash commented Mar 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

qxprakash commented Mar 25, 2024

vishniakov-nikolai left a comment

Choose a reason for hiding this comment

qxprakash commented Mar 26, 2024 • edited Loading

mlukasze commented Mar 27, 2024

qxprakash commented Mar 16, 2024 •

edited

Loading

qxprakash commented Mar 16, 2024 •

edited

Loading

almilosz Mar 20, 2024 •

edited

Loading

qxprakash Mar 20, 2024 •

edited

Loading

almilosz Mar 20, 2024 •

edited

Loading

qxprakash commented Mar 22, 2024 •

edited

Loading

almilosz left a comment •

edited

Loading

qxprakash commented Mar 23, 2024 •

edited

Loading

qxprakash commented Mar 26, 2024 •

edited

Loading