-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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]: Support aten::avg_poolNd and aten::max_poolNd with no batch #20927
Comments
|
Thank you for looking into this issue! Please let us know if you have any questions or require any help. |
Hello @ahmadchalhoub! Thank you for taking a look, please let us know if you have any questions. Just yesterday our CONTRIBUTING.md has been updated with a technical guide - I highly recommend checking it out. :) |
@p-wysocki Thanks for your input! I'll definitely take a look at the updates :). I already started working on the PR for this but it might take me a few days. |
Hello @ahmadchalhoub, do you need any help? Are you still working on that issue? I am happy to announce that we have created a channel dedicated to Good First Issues support on our Intel DevHub Discord server! Join it to receive support, engage in discussions, ask questions and talk to OpenVINO developers. |
Hi @p-wysocki! Very sorry for the delay; I got caught up with some things and will not be able to work on this issue. I will un-assign myself so someone else can pick it up :) |
No worries, you're welcome to pick up another issue anytime. :) |
@p-wysocki I am new to open-source, could you please provide some guidance relevant to above issue? |
@mmikolajcz could you help here? |
Hello @Sar2580P, you can start with our technical guide in CONTRIBUTING.md :) You're also welcome to join our Intel DevHub Discord server, where you can receive chat support from OpenVINO developers. |
That is exactly right, you need to reshape input 2 times, but the main problem is to calculate the shape to reshape to, when input shape is only know in runtime. |
I am very sorry, but do you know the deadline for this bug? |
.take |
Thank you for looking into this issue! Please let us know if you have any questions or require any help. |
Hi. I am new to Open source contributing but I am excited to start as I am too. I am going to participate in GSOC 24 too and choosing OpenVino organization as well. Currently I'm pursuing a Bachelor's degree in CSE AI and Machine Learning and have also some experience in Deep learning and neural networks as well. The problem here is that we need a batch size initialization and channel here refers to the image components of an image. E.g RGB, CMY, Grayscale etc. Here in CNN it typically means the depth dimension of the input tensor. For OpenVino's MaxPool and AvgPool operations it assumes batch and channels dimensions exist. But for images that don't have it in input need to be assigned batch as 1 and then after operation get back to its original size. In the above code given we can try making a block of code which can check whether the image has a batch or not and if not then we assign batch as 1. I am writing a code here since i dont know if there is a seperate section for providing codes as solutions In this repo, we can try the following code: (This is my first open source contribution please be kind :) ) OutputVector translate_avg_poolnd(const NodeContext& context) {
} Please feel free to guide me more and notify me if anything comes up. |
@krish1209 Please check contribution guide for tips how to contribute and create Pull Requests. The code you provided will not actually work as you expect, because Also please write a test for all these cases first and make sure your solution passes all tests. If you have any questions please feel free to ask. |
Okay, I'll spin my head around it. Meanwhile could you let me know like how are you thinking of approaching this ? |
Hello guys, I'm facing this issue and looking forward to fixing it. My MaxPool3D layer is causing the issue:
Are there any ways to escape this issue instead of waiting for new updates? |
@cannguyen275 you could modify the model code to unsqueeze before pool and squeeze after, so you will run pooling in |
.take |
Thank you for looking into this issue! Please let us know if you have any questions or require any help. |
@mlukasze Hello, I’m new to open source. Could you help me understand how avg_poolnd.cpp and max_poolnd.cpp handle inputs of varying dimensions? |
Hello @tianyiSKY1! Thank you for taking a look at the task. In the files you mentioned the inputs are retrieved using
As for inputs of varying dimensions you're most likely talking about They can be static (for example In the snippet in #20927 (comment) you can see how to access the input's Additionally, it's worth checking out @mvafin's very informative comment about handling different data layouts: #20927 (comment). Please let us know if you have any other questions. |
@p-wysocki Hi, I conducted a preliminary test and wrote some test code. However, every time I input a case for the CHWD dimension, the test fails with an 'is_conversion_successful' error. Is there a part of the code that autommatically detects the input shape, which might be preventing me from proceeding with further testing? class TestAvgPool3D(PytorchLayerTest):
def _prepare_input(self):
return (self.input_tensor, )
def create_model(self, kernel_size, stride, padding, ceil_mode=True, count_include_pad=True):
class aten_avg_pool3d(torch.nn.Module):
def __init__(self, kernel_size, stride, padding, ceil_mode, count_include_pad) -> None:
super().__init__()
self.kernel_size = kernel_size
print(kernel_size)
self.stride = stride
print(stride)
self.padding = padding
self.ceil_mode = ceil_mode
self.count_include_pad = count_include_pad
def forward(self, input_tensor):
return torch.nn.functional.avg_pool3d(input_tensor, self.kernel_size, self.stride, self.padding, self.ceil_mode,
self.count_include_pad)
ref_net = None
return aten_avg_pool3d(kernel_size, stride, padding, ceil_mode=True, count_include_pad=True), ref_net, "aten::avg_pool3d"
@pytest.mark.parametrize('input_shape', [[1, 3, 15, 15, 15], [3, 15, 15, 15]])
@pytest.mark.parametrize("params", d3_params)
@pytest.mark.parametrize("ceil_mode", [True, False])
@pytest.mark.parametrize("count_include_pad", [True, False])
@pytest.mark.nightly
@pytest.mark.precommit
@pytest.mark.precommit_torch_export
@pytest.mark.precommit_fx_backend
def test_avg_pool3d(self, params, ceil_mode, count_include_pad, ie_device, precision, ir_version, input_shape):
self.input_tensor = np.random.randn(*input_shape).astype(np.float32)
self._test(*self.create_model(**params, ceil_mode=ceil_mode, count_include_pad=count_include_pad),
ie_device, precision, ir_version, trace_model=True, dynamic_shapes=False) FAILED test_avg_pool.py::TestAvgPool3D::test_avg_pool3d[ ie_device:CPU - precision:FP16 - count_include_pad:False - ceil_mode:False - params:{'kernel_size': [3, 2, 1], 'stride': None, 'padding': [0, 0, 0]} - input_shape:[3, 15, 15, 15] ] - openvino._pyopenvino.OpConversionFailure: Check 'is_conversion_successful' failed at src/frontends/pytorch/src/frontend.cpp:166: |
Judging by the check it seems the operator for some reason hasn't been converted: openvino/src/frontends/pytorch/src/frontend.cpp Lines 172 to 173 in 69180e2
I'd use a debugger to see what's going on in the function during the test exactly, with the breakpoint at the beginning of the conversion function. Perhaps there's some assert the operator doesn't pass and the conversion function fails? |
hi @p-wysocki I found places that prevented me from converting models: openvino/src/core/shape_inference/include/pooling_shape_inference_util.hpp Lines 56 to 59 in 91b5744
I'm trying to force a model conversion by commenting out this code. I did a recompile after making the changes, but it still runs as the original code when I test it. I've tried deleting the \build file and recompiling, but it doesn't work.
|
@tianyiSKY1 You shouldn't modify shape inference code. The problem that needs to be solved is reshaping of CHW to 1CHW input and the reshape of the output of the operation back by removing 1. |
@mvafin I have tried reshaping the CHW to a 1CHW input and reshaping the output of the operation by removing the 1. But in the testing phase, the check for the input shape prevents the model conversion. OutputVector translate_avg_pool3d(const NodeContext& context) {
auto input = context.get_input(0);
auto input_shape = input.get_partial_shape();
auto kernel = context.const_input<Shape>(1);
// Check if the input has a batch dimension
bool has_batch_dim = input_shape.rank().is_static() && input_shape.rank().get_length() == 5;
if (!has_batch_dim) {
// If there is no batch dimension, add one by unsqueezing the input tensor
auto zero = v0::Constant::create(element::i32, Shape{}, {0});
auto unsqueezed_input = context.mark_node(std::make_shared<v0::Unsqueeze>(input, zero));
input = unsqueezed_input;
}
auto pooled_output = translate_avg_pool_base(context, input);
auto const_0 = context.mark_node(v0::Constant::create(element::i32, Shape{1}, {0}));
auto const_1 = context.mark_node(v0::Constant::create(element::i32, Shape{1}, {1}));
auto slice_end = context.mark_node(v0::Constant::create(element::i32, Shape{1}, {-4}));
if (!has_batch_dim) {
// If there was no batch dimension, remove the added dimension by slicing the output tensor
auto sliced_output = context.mark_node(std::make_shared<v8::Slice>(pooled_output[0], const_0, slice_end, const_1, const_0));
Output<Node> pooled_output = sliced_output;
}
return {pooled_output};
}; |
Did you add dynamic_shape=False to test class? Your solution uses get_partial_shape which is not-recommended, because it will not work for dynamic shapes. Also you use To make your solution working on dynamic shapes, you need to do something like this assuming 3d case: |
@mvafin Yes, I added dynamic_shape=False to test class. |
@mvafin Hi, I'm sorry to interrupt. Can you please guide me a little further? OutputVector translate_avg_pool3d(const NodeContext& context) {
auto input = context.get_input(0);
auto kernel = context.const_input<Shape>(1);
// Check if the input has a batch dimension
auto input_shape_of = context.mark_node(std::make_shared<v3::ShapeOf>(input));
auto has_batch_dim = context.mark_node(std::make_shared<v1::Equal>(
input_shape_of,
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {5}))
));
if (!has_batch_dim) {
// If there is no batch dimension, add one by reshaping the input tensor
auto reshape_pattern = context.mark_node(v0::Constant::create(element::i32, Shape{5}, {-1, 0, 0, 0, 0}));
input = context.mark_node(std::make_shared<v1::Reshape>(input, reshape_pattern, true));
}
auto pooled_output = translate_avg_pool_base(context, input);
if (!has_batch_dim) {
// If there was no batch dimension, remove the added dimension by slicing the output tensor
auto input_shape_of = context.mark_node(std::make_shared<v3::ShapeOf>(input));
auto pooled_output_shape_of = context.mark_node(std::make_shared<v3::ShapeOf>(pooled_output[0]));
auto slice_input_shape = context.mark_node(std::make_shared<v8::Slice>(input_shape_of,
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-3})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0}))));
auto slice_pooled_output_shape = context.mark_node(std::make_shared<v8::Slice>(pooled_output_shape_of,
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-3})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0}))));
auto concat_shape = context.mark_node(std::make_shared<v0::Concat>(OutputVector{slice_input_shape, slice_pooled_output_shape}, 0));
for (auto& node : pooled_output) {
node = context.mark_node(std::make_shared<v1::Reshape>(node, concat_shape, true));
}
}
return {pooled_output};
}; |
@tianyiSKY1 No worries, thank you for working on this issue. You seem to misunderstand how operations work. As for the code under the if it seems right in concept, but it needs to be tested. |
@mvafin Hi, I followed your instructions and tried again. It even directly expands all input tensor without judgment to directly expand the batch dimensions. But it still doesn't work. Did I fail to expand the dimension? OutputVector translate_avg_pool3d(const NodeContext& context) {
auto input = context.get_input(0);
auto kernel = context.const_input<Shape>(1);
// Check if the input has a batch dimension
auto input_shape = input.get_partial_shape();
if (input_shape.is_static()) {
bool has_batch_dim = input_shape.rank().get_length() == 5;
if (!has_batch_dim) {
// Reshape input to add a batch dimension
auto reshape_pattern = context.mark_node(v0::Constant::create(element::i64, Shape{5}, {-1, 0, 0, 0, 0}));
input = context.mark_node(std::make_shared<v1::Reshape>(input, reshape_pattern, true));
}
} else {
// If the input shape is dynamic, add a batch dimension by default
auto reshape_pattern = context.mark_node(v0::Constant::create(element::i64, Shape{5}, {-1, 0, 0, 0, 0}));
input = context.mark_node(std::make_shared<v1::Reshape>(input, reshape_pattern, true));
}
auto pooled_output = translate_avg_pool_base(context, input);
// If there was no batch dimension, remove the added dimension by slicing the output tensor
auto input_shape_of = context.mark_node(std::make_shared<v3::ShapeOf>(input));
auto pooled_output_shape_of = context.mark_node(std::make_shared<v3::ShapeOf>(pooled_output[0]));
auto slice_input_shape = context.mark_node(std::make_shared<v8::Slice>(input_shape_of,
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-3})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0}))));
auto slice_pooled_output_shape = context.mark_node(std::make_shared<v8::Slice>(pooled_output_shape_of,
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-3})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {-1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {1})),
context.mark_node(v0::Constant::create(element::i64, Shape{1}, {0}))));
auto concat_shape = context.mark_node(std::make_shared<v0::Concat>(OutputVector{slice_input_shape, slice_pooled_output_shape}, 0));
for (auto& node : pooled_output) {
node = context.mark_node(std::make_shared<v1::Reshape>(node, concat_shape, true));
}
return pooled_output;
};
|
@mvafin Hello, I was wondering if there is a way to view the variables in |
@mvafin gently ping |
@tianyiSKY1 From what I can see your code should work. You can use |
Hi, @mvafin I've solved the problem of the code not being used. After successfully running the code I realized that I can't expand the dimension of the input with auto reshape_pattern = context.mark_node(v0::Constant::create(element::i64, Shape{5}, {-1, 0, 0, 0, 0}));
input = context.mark_node(std::make_shared<v1::Reshape>(input, reshape_pattern, true)); If I expand the dimension in this way, will report an error E RuntimeError: Check 'i < input_shape.size()' failed at src/core/shape_inference/include/reshape_shape_inference.hpp:306:
E While validating node 'opset1::Reshape aten::avg_pool3d/Reshape (opset1::Parameter input_tensor[0]:f32[?,?,?,?], opset1::Constant aten::avg_pool3d/Constant[0]:i64[5]) -> (dynamic[?,?,?,?,?])' with friendly_name 'aten::avg_pool3d/Reshape':
E Shape inference input shapes {[?,?,?,?],[5]}
E '0' dimension is out of range I tried to successfully expand the dimensions with auto unsqueeze_axis = context.mark_node(v0::Constant::create(element::i64, Shape{}, {0}));
input = context.mark_node(std::make_shared<v0::Unsqueeze>(input, unsqueeze_axis)); However, due to the failure to obtain dynamic shapes, it would expand inputs that have batch dimensions as well. |
@tianyiSKY1 It is my bad, I suggested using Reshape with zeros, but this is will not work in this case. Using |
Context
OpenVINO component responsible for support of PyTorch models is called as PyTorch Frontend (PT FE). PT FE converts a model represented as TorchScript model to a model in OpenVINO opset.
What needs to be done?
Example Pull Requests
No response
Resources
Contact points
@openvinotoolkit/openvino-pytorch-frontend-maintainers
Ticket
No response
The text was updated successfully, but these errors were encountered: