Skip to content
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

[Good First Issue]: Enable CompiledModel set/get property() #24374

Closed
almilosz opened this issue May 6, 2024 · 16 comments · Fixed by #25808
Closed

[Good First Issue]: Enable CompiledModel set/get property() #24374

almilosz opened this issue May 6, 2024 · 16 comments · Fixed by #25808
Assignees
Labels
category: JS API OpenVino JS API Bindings good first issue Good for newcomers no_stale Do not mark as stale
Milestone

Comments

@almilosz
Copy link
Contributor

almilosz commented May 6, 2024

Context

Node.js API is the newest binding of OpenVINO.
The task is to expose 2 methods: CompiledModel::set_property() and CompiledModel::get_property().
C++ docs here

What needs to be done?

  • add methods on C++ side (src/bindings/js/node/src/compiled_model.cpp)
  • update TypeScript definition (src/bindings/js/node/lib/addon.ts)
  • create unit test for added functionality using Node.js Test Runner
    Tip: Before creating a new function to convert an argument, take a look at helper methods to see if it doesn't exist already

Example Pull Requests

Resources

Contact points

@almilosz @vishniakov-nikolai

Ticket

134825

@almilosz almilosz added good first issue Good for newcomers category: JS API OpenVino JS API Bindings no_stale Do not mark as stale labels May 6, 2024
@github-project-automation github-project-automation bot moved this to Contributors Needed in Good first issues May 6, 2024
@Aryan8912
Copy link
Contributor

.take

Copy link
Contributor

github-actions bot commented May 7, 2024

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@mlukasze mlukasze moved this from Contributors Needed to Assigned in Good first issues May 7, 2024
@p-wysocki
Copy link
Contributor

Hello @Aryan8912, there are currently three issues with open PRs assigned to you. Please finish them before picking more tasks.

@p-wysocki p-wysocki moved this from Assigned to Contributors Needed in Good first issues May 7, 2024
@Plomo-02
Copy link

hello, Is it possible being assigned to this issue?

@Plomo-02
Copy link

.take

Copy link
Contributor

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@p-wysocki p-wysocki moved this from Contributors Needed to Assigned in Good first issues May 27, 2024
@mlukasze
Copy link
Contributor

hey @Plomo-02 do you need any help?
will you have a time to continue work on this task?

@mlukasze mlukasze moved this from Assigned to Contributors Needed in Good first issues Jul 25, 2024
@hub-bla
Copy link
Contributor

hub-bla commented Jul 26, 2024

.take

Copy link
Contributor

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@hub-bla
Copy link
Contributor

hub-bla commented Jul 27, 2024

Hi @almilosz @vishniakov-nikolai, I have encountered an issue while testing the implementation of CompiledModel::set_property()

  Exception from src/inference/src/cpp/compiled_model.cpp:136:
  Exception from src/plugins/intel_cpu/src/compiled_model.h:39:
  Not Implemented:
  It's not possible to set property of an already compiled model. Set property to Core::compile_model during compilation

It seems like there is a template:

// ! [compiled_model:set_property]
void ov::template_plugin::CompiledModel::set_property(const ov::AnyMap& properties) {
m_cfg = Configuration{properties, m_cfg};
}
// ! [compiled_model:set_property]

But only one plugin implements that:

void CompiledModel::set_property(const ov::AnyMap& properties) {
for (const auto& property : properties) {
if (property.first == ov::auto_batch_timeout.name()) {
m_time_out = property.second.as<std::uint32_t>();
m_config[ov::auto_batch_timeout.name()] = property.second.as<std::uint32_t>();
} else {
OPENVINO_THROW("AutoBatching Compiled Model dosen't support property",
property.first,
". The only property that can be changed on the fly is the ",
ov::auto_batch_timeout.name());
}
}
}

Other have:

void set_property(const ov::AnyMap& properties) override {
OPENVINO_THROW_NOT_IMPLEMENTED("It's not possible to set property of an already compiled model. "
"Set property to Core::compile_model during compilation");
};

My implementation so far:

Napi::Value CompiledModelWrap::set_property(const Napi::CallbackInfo &info) {
    Napi::Env env = info.Env();
    try{
        if (info.Length() != 1 || !info[0].IsObject()) {
            OPENVINO_THROW("Expected a single object argument for setting properties");
        }
        const auto properties = to_anyMap(env, info[0]);
        _compiled_model.set_property(properties);
    }catch (const std::exception& e) {
            reportError(env, e.what());
    }
    return env.Undefined();
}

My question

How should I test it?

@mlukasze mlukasze moved this from Contributors Needed to Assigned in Good first issues Jul 29, 2024
@nashez
Copy link
Contributor

nashez commented Jul 29, 2024

@hub-bla @mlukasze Is this still under development?
If not can I take this up?

@hub-bla
Copy link
Contributor

hub-bla commented Jul 29, 2024

Hi @nashez, the issue is still under development. I have everything implemented but this test for set_property(). I'm waiting for a response to my issue.

@almilosz
Copy link
Contributor Author

Hi @hub-bla!
Thanks for taking this issue.
CompiledModel::set_property() can be tested in Python like this:

timeout = 10
cm = core.compile_model(model, "BATCH:CPU")
cm.set_property(props.auto_batch_timeout(timeout))
assert timeout == cm.get_property('AUTO_BATCH_TIMEOUT')

To do that in Node.js API you will have to expose ov::auto_batch_timeout additionally, which may require more work.

Alternatively, you can wait as we look into the reasons why the option below is not working:

cm.set_property({'AUTO_BATCH_TIMEOUT': 1})

Let me know what you decide to do. If you have any questions, don't hesitate to ask them. You can reach me on Discord _almilosz and ask directly.

Greetings from Poznań,
Alicja

@hub-bla
Copy link
Contributor

hub-bla commented Jul 30, 2024

@almilosz I found that if you change 1 in cm.set_property({'AUTO_BATCH_TIMEOUT': 1}) to be a string it doesn't throw error so I guess there is a problem with converting this variable under the hood.

Is it enough reason for not exposing auto_batch_timeout in this issue?
Should I also create separate file for testing compiled_model?

Thank you for your hint!

Poznań? Never heard of it ;)
Must be like a GitHub project - constantly under 'maintenance' with occasional surprise updates.

@almilosz
Copy link
Contributor Author

Yes, please create a test like this in a new file compiled_model.test.js file
Exposing properties will be a separate issue :)

@hub-bla
Copy link
Contributor

hub-bla commented Jul 30, 2024

I looked into the problem with conversion and I think I found the reason why.

template <>
ov::Any js_to_cpp<ov::Any>(const Napi::Env& env, const Napi::Value& value) {
if (value.IsString()) {
return ov::Any(value.ToString().Utf8Value());
} else if (value.IsBigInt()) {
Napi::BigInt big_value = value.As<Napi::BigInt>();
bool is_lossless;
int64_t big_num = big_value.Int64Value(&is_lossless);
if (!is_lossless) {
OPENVINO_THROW("Result of BigInt conversion to int64_t results in a loss of precision");
}
return ov::Any(big_num);
} else if (value.IsNumber()) {
Napi::Number num = value.ToNumber();
if (is_napi_value_int(env, value)) {
return ov::Any(num.Int32Value());
} else {
return ov::Any(num.DoubleValue());
}
} else if (value.IsBoolean()) {
return ov::Any(value.ToBoolean());
} else {
OPENVINO_THROW("Cannot convert to ov::Any");
}
}

js_to_cpp which is used in to_anyMap returns int32_t
But in auto-batch CompiledModel::set_property we can see it tries to convert it as uint32_t
void CompiledModel::set_property(const ov::AnyMap& properties) {
for (const auto& property : properties) {
if (property.first == ov::auto_batch_timeout.name()) {
m_time_out = property.second.as<std::uint32_t>();
m_config[ov::auto_batch_timeout.name()] = property.second.as<std::uint32_t>();
} else {
OPENVINO_THROW("AutoBatching Compiled Model dosen't support property",
property.first,
". The only property that can be changed on the fly is the ",
ov::auto_batch_timeout.name());
}
}
}

and .as<std::uint32_t>() is the operation that fails
as() {
impl_check();
if (_impl->is(typeid(decay_t<T>))) {
return *static_cast<decay_t<T>*>(_impl->addressof());
} else if (_impl->is(typeid(std::string))) {
_temp = std::make_shared<Impl<decay_t<T>>>();
_impl->read_to(*_temp);
return *static_cast<decay_t<T>*>(_temp->addressof());
}
for (const auto& type_index : _impl->base_type_info()) {
if (util::equal(type_index, typeid(decay_t<T>))) {
return *static_cast<decay_t<T>*>(_impl->addressof());
}
}
OPENVINO_THROW("Bad cast from: ", _impl->type_info().name(), " to: ", typeid(T).name());
}

@almilosz almilosz linked a pull request Aug 8, 2024 that will close this issue
github-merge-queue bot pushed a commit that referenced this issue Aug 14, 2024
### Details:
- add 2 methods: `CompiledModel::set_property()` and
`CompiledModel::get_property()`
 - create TypeScript definition for new created methods
- create unit tests for new functionalities (due to the issue that was
mentioned
[here](#24374 (comment)),
it's more of a mock now and should be altered as soon as the issue is
fixed)

### Tickets:
 - 134825
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues Aug 14, 2024
@mlukasze mlukasze added this to the 2024.4 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: JS API OpenVino JS API Bindings good first issue Good for newcomers no_stale Do not mark as stale
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants