-
Notifications
You must be signed in to change notification settings - Fork 3k
[JS API] Add import_model(tensor) to Node.js API #33401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JS API] Add import_model(tensor) to Node.js API #33401
Conversation
- Implement tensor overload in CoreWrap::import_model() - Implement tensor overload in CoreWrap::import_model_async() - Add TypeScript definitions for Tensor parameter - Add comprehensive tests for sync and async variants Fixes openvinotoolkit#32725
almilosz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
please run npm run format. Codestyle failed:
/__w/openvino/openvino/src/bindings/js/node/tests/unit/basic.test.js
Error: 403:46 error Replace `⏎········ov.element.u8,⏎········[userStream.length],⏎········uint8Array,⏎······` with `ov.element.u8,·[userStream.length],·uint8Array` prettier/prettier
Error: 420:46 error Replace `⏎········ov.element.u8,⏎········[userStream.length],⏎········uint8Array,⏎······` with `ov.element.u8,·[userStream.length],·uint8Array` prettier/prettier
Error: 439:46 error Replace `⏎········ov.element.u8,⏎········[userStream.length],⏎········uint8Array,⏎······` with `ov.element.u8,·[userStream.length],·uint8Array` prettier/prettier
Error: 453:46 error Replace `⏎········ov.element.u8,⏎········[userStream.length],⏎········uint8Array,⏎······` with `ov.element.u8,·[userStream.length],·uint8Array` prettier/prettier
|
Hey, I have made the requested code style formatting. |
|
@almilosz , Thanks !! |
|
Kindly requestin you to review the changes made. Thanks and regards, |
almilosz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
functionality and tests looks good. I would change the usage of std::stringstream to ov::SharedStreamBuffer and add std::variant for stream and tensor in ImportModelContext.
If CI's checks are green, I propose to merge this PR. Would you like to work on the improvements I mentioned in a new PR @Nishant-ZFYII ?
|
Good Day @almilosz , Quick question about CI. On my side, the GitHub Actions checks look green (summary shows success), but the required Is Jenkins something that needs to be manually triggered/approved (especially since this PR is from a fork), or is there a label/comment I should add to start it? Thanks — just want to make sure I’m not missing a required step on my side. |
|
@almilosz — thanks a lot for the review! Great to hear the functionality + tests look good. Yes, I’d be happy to work on the improvements you mentioned in a separate PR:
If you’re okay with it, I’ll wait for the remaining CI checks to go green and for this PR to merge first, and then I’ll open a follow-up PR with those refactors. Quick question so I do this the properly: is there an in-repo reference where |
|
I have to trigger |
|
build_jenkins |
|
Good Day @almilosz I’m seeing a few required checks failing on this PR, and I’m not sure if they’re caused by my changes or otherwise:
Can you please provide some guidance on how to solve this issue. |
|
Good Day @almilosz, Thanks . |
|
Hi @almilosz, I hope you're doing well. I wanted to follow up on my previous message. I'm seeing 3 failing checks in the CI:
I'm not sure if these failures are related to my changes or if they are pre-existing/infrastructure issues. I also see the branch is out-of-date with the base branch. Should I click "Update branch" to sync with master, or would you prefer I wait? Could you please provide some guidance on:
I'm new to contributing to this project and would really appreciate any help or direction you can provide. Thank you for your time! |
|
Hello, sometimes checks fail due to infrastructure errors. Right now please wait on my feedback. Have a nice day |
|
build_jenkins |
|
build_jenkins |
|
build_jenkins |
|
Congrats on merging your PR! |
Summary
Expose
Core::import_model(tensor, device, config)to the Node.js API so compiled models can be imported from anov.Tensor, matching the Python bindings (#32651).Fixes #32725
What/Why
OpenVINO C++ already supports importing a compiled model from a Tensor (#31327). This PR wires that overload into the JavaScript/Node.js bindings so users can round-trip a compiled model via
exportModelSync()→Tensor→importModelSync()(and the async equivalents).Changes
Tensoroverload forCoreWrap::import_model()(sync)Tensoroverload forCoreWrap::import_model_async()(async)Tensoras the first parameterTesting
validate_value,cast_to_tensor,to_anyMap)Example