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
G-API: Get input model layout from the IR if possible in OV 2.0 backend #24658
G-API: Get input model layout from the IR if possible in OV 2.0 backend #24658
Conversation
This PR fixes smart classroom G-API demo in OMZ: openvinotoolkit/open_model_zoo#3881 |
|
||
auto f = std::make_shared<ov::Model>(ov::ResultVector{result}, ov::ParameterVector{data1}, "function_name"); | ||
|
||
ov::serialize(f, model_path, weights_path, ov::pass::Serialize::Version::IR_V11); |
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.
Probably there's some general approach to producing temporary files in tests in OpenCV.. Like some specific folder or naming convention?
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.
btw maybe we in the future we need to extend our API to accept objects of ov::Model
directly instead of xml/bin
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.
btw maybe we in the future we need to extend our API to accept objects of
ov::Model
directly instead of xml/bin
In that case it won't be ov::Model
because headers will depend on OpenVINO
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.
Dude we did this with PlaidML already
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.
plaidml.hpp
looks clear from framework details
@TolyaTalamanov do you have any comments here? |
m_input_model_layout.emplace(input_name, default_layout); | ||
const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name)); | ||
if (!input_layout.empty()) { | ||
input_info.model().set_layout(input_layout); |
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.
input_info.model()
and m_model
should refer to the same object, I'm wondering why this line is even needed.
Looks even though model
has model
layout it must be specified to PrePostProcessor
as well.
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.
Worth a comment at least, since it's not clear why we need to specify model
layout for the preprocessor created from exactly the same model.
PrePostProcessor ppp(model);
audo layout = ov::layout::get::layout(model->input(input_name));
if (!layout.empty()) {
ppp.input(input_name).model().set_layout(layout);
}
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.
Ok, looks we figured out it locally. Need to force model layout only if model doesn't have one otherwise keep it as-is.
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.
It works without this line. OV works fine
ov::layout::set_layout(data1, ov::Layout("NHWC")); | ||
|
||
auto sin = std::make_shared<ov::opset8::Sin>(data1); | ||
sin->output(0).set_names({"sin_t"}); // tensor name |
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.
not necessarily needed to specify name there
|
||
auto f = std::make_shared<ov::Model>(ov::ResultVector{result}, ov::ParameterVector{data1}, "function_name"); | ||
|
||
ov::serialize(f, model_path, weights_path, ov::pass::Serialize::Version::IR_V11); |
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.
btw maybe we in the future we need to extend our API to accept objects of
ov::Model
directly instead of xml/bin
In that case it won't be ov::Model
because headers will depend on OpenVINO
model_path, | ||
weights_path, | ||
device_id); | ||
EXPECT_NO_THROW(comp.apply(cv::gin(in_mat1), cv::gout(gapi_mat), |
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.
Tbh I didn't get the purpose of this test, the model has NHWC
layout but you pass NCHW
mat, so it won't throw any exceptions because ov
backend doesn't validate either layouts match or not.
The test should be something like that:
- Model has
NHWC
layout - Provide 4D mat and specify only
tensor
layout (e.g cfgInputTensorLayout('NCHW')) - Check if there is
NHWC->NCHW
conversion is added to the model. (it won't be added without your changes, since model layout not specified therefore won't be propagated intoPrePostProcessor
)
m_input_model_layout.emplace(input_name, default_layout); | ||
const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name)); | ||
if (!input_layout.empty()) { | ||
input_info.model().set_layout(input_layout); |
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.
Worth a comment at least, since it's not clear why we need to specify model
layout for the preprocessor created from exactly the same model.
PrePostProcessor ppp(model);
audo layout = ov::layout::get::layout(model->input(input_name));
if (!layout.empty()) {
ppp.input(input_name).model().set_layout(layout);
}
m_input_model_layout.emplace(input_name, default_layout); | ||
const auto& input_layout = ::ov::layout::get_layout(m_model->input(input_name)); | ||
if (!input_layout.empty()) { | ||
input_info.model().set_layout(input_layout); |
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.
Looking at it on the second day, is this a valid change at all? Why we're setting whole model's layout based on some input layout?
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.
It's not for the whole
model, it's set to input_info
which refers to the particular input layer.
input_info = m_ppp.input(input_name);
...
input_info.model().set_layout(input_layout);
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.
This line will be removed, since OV does it in their PrePostProc
Please explain what is the issue and what is the change here. |
The issue is that user might not want to configure input model layout which is available in the network. So, they don't set |
18ed79e
to
f6240db
Compare
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.
LGTM 👍
@asmorkalov @mshabunin can we merge this please? |
@asmorkalov @mshabunin kind reminder |
@asmorkalov @mshabunin can we merge it this week pretty please? |
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.
Looks good to me.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.