[Python API] Added implicit input reshaping API and extended reshape() Unit Tests.#32648
[Python API] Added implicit input reshaping API and extended reshape() Unit Tests.#32648Jarvis2001 wants to merge 27 commits intoopenvinotoolkit:masterfrom
reshape() Unit Tests.#32648Conversation
almilosz
left a comment
There was a problem hiding this comment.
Hello @Jarvis2001,
I approved tests to run and left first round of review comments. Please fix them and pay attention to existing overloads of Model.reshape. Maybe you just have to change helper Common::partial_shape_from_list?
Have a nice day,
Alicja
| if (py::isinstance<py::dict>(shapes)) { | ||
| py::dict shapes_dict = shapes.cast<py::dict>(); | ||
| for (auto& item : shapes_dict) { | ||
| new_shapes.emplace(output_from_handle(self, item.first), partial_shape_from_handle(item.second)); | ||
| } |
There was a problem hiding this comment.
You repeat here what this reshape overload does
| param = ops.parameter([1, 3, 224, 224], np.float32, name="input") | ||
| relu = ops.relu(param) | ||
| model = Model([relu], [param], "TinyModel") | ||
| model.reshape({0: [0, 6]}) |
There was a problem hiding this comment.
Tests for reshape with dictionaries and input indexes/port/names exist, e.g test exists.
The task is to add a new overload that accepts a list of input shapes and assigns them to corresponding inputs.
|
|
||
|
|
||
| def test_dynamic_dimension_input_shape(): | ||
| """Test reshaping a model with mixed dynamic and static dimensions. |
There was a problem hiding this comment.
I can't find .reshape method in this test
| param1 = ops.parameter(PartialShape([-1, -1]), dtype=np.float32, name="input1") | ||
| param2 = ops.parameter(PartialShape([-1, -1]), dtype=np.float32, name="input2") | ||
| output = ops.add(ops.add(param0, param1), param2) | ||
| model = Model(output, [param0, param1, param2], "PartialReshapeModel") |
There was a problem hiding this comment.
There are helpers to create models e.g. model = generate_add_model(). They should be enough to test new functionality, if they are not add a new helper, do not repeat code for model creation
| # Dict form; works | ||
| model.reshape({0: [2, 2], 1: [2, 2], 2: [2, 2]}) | ||
| inputs = model.inputs | ||
| assert inputs[0].partial_shape == PartialShape([2, 2]) | ||
| assert inputs[1].partial_shape == PartialShape([2, 2]) | ||
| assert inputs[2].partial_shape == PartialShape([2, 2]) |
There was a problem hiding this comment.
Please add tests only for new functionality
| assert inputs[1].partial_shape == PartialShape([2, 2]) | ||
| assert inputs[2].partial_shape == PartialShape([2, 2]) | ||
| # Single-input model | ||
| param_s = ops.parameter([4, 4], dtype=np.float32, name="S") |
There was a problem hiding this comment.
Please split this test. You can have test for model with one input, one for multiple input etc.
|
PS: I can see python api linter checks failed. Please fix them |
almilosz
left a comment
There was a problem hiding this comment.
Hello,
there is still issue with linting.
Please check that right now we have reshape(py::list) i reshape(list, optional variable_shapes)
|
This PR will be closed in a week because of 2 weeks of no activity. |
|
As @almilosz said previously, please fix the failing CI checks, which are mostly linters. In case of further inactivity this PR will be closed. |
There was a problem hiding this comment.
Pull request overview
Adds a new Python Model.reshape() overload that supports reshaping all inputs via a single “list of shapes” argument, and expands the unit test suite to cover more reshape patterns and validation scenarios.
Changes:
- Extends the Python binding for
Model.reshape()to accept a list of per-input shapes and map them to inputs by order. - Adds new test helper model generators for single-/multi-input reshape scenarios.
- Adds unit tests covering list-of-shapes reshape, dynamic dimensions, and several error cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/bindings/python/src/pyopenvino/graph/model.cpp |
Implements list-of-shapes parsing and dispatches reshape to the map-based overload. |
src/bindings/python/tests/utils/helpers.py |
Adds helper generators for single-/multi-/two-input models used by new reshape tests. |
src/bindings/python/tests/test_runtime/test_model.py |
Adds new reshape unit tests and refactors some existing formatting/imports. |
| bool has_list = false; | ||
| bool has_int = false; | ||
|
|
||
| for (size_t i = 0; i < partial_shape.size(); ++i) { | ||
| py::handle elem = partial_shape[i]; | ||
| if (py::isinstance<py::list>(elem)) { | ||
| has_list = true; | ||
| } else if (py::isinstance<py::int_>(elem)) { | ||
| has_int = true; | ||
| } else if (!py::isinstance<py::tuple>(elem)) { | ||
| throw std::runtime_error("Invalid shape format"); | ||
| } | ||
| } | ||
|
|
||
| if (has_list) { | ||
| is_multi_input_format = true; | ||
| } else { | ||
| is_multi_input_format = !has_int; | ||
| } |
There was a problem hiding this comment.
The multi-vs-flat detection is incorrect and causes regressions for valid flat shapes that include list-based dimension intervals (e.g. [1, 3, [224, 256], [224, 256]]). Any list element currently forces is_multi_input_format = true, which would then throw on partial_shape.size() != inputs.size(). Suggest basing multi-input detection on partial_shape.size() == inputs.size() and all_of(top-level elems are list/tuple/PartialShape-like shape containers), otherwise treat it as the flat-shape form.
| bool has_list = false; | |
| bool has_int = false; | |
| for (size_t i = 0; i < partial_shape.size(); ++i) { | |
| py::handle elem = partial_shape[i]; | |
| if (py::isinstance<py::list>(elem)) { | |
| has_list = true; | |
| } else if (py::isinstance<py::int_>(elem)) { | |
| has_int = true; | |
| } else if (!py::isinstance<py::tuple>(elem)) { | |
| throw std::runtime_error("Invalid shape format"); | |
| } | |
| } | |
| if (has_list) { | |
| is_multi_input_format = true; | |
| } else { | |
| is_multi_input_format = !has_int; | |
| } | |
| bool all_containers = true; | |
| bool has_int = false; | |
| for (size_t i = 0; i < partial_shape.size(); ++i) { | |
| py::handle elem = partial_shape[i]; | |
| if (py::isinstance<py::int_>(elem)) { | |
| has_int = true; | |
| all_containers = false; | |
| } else if (py::isinstance<py::list>(elem) || py::isinstance<py::tuple>(elem)) { | |
| // list/tuple are treated as shape containers at the top level | |
| } else { | |
| throw std::runtime_error("Invalid shape format"); | |
| } | |
| } | |
| // Multi-input format is used only when the number of provided shapes | |
| // matches the number of model inputs and all top-level elements are | |
| // shape containers (lists/tuples). Otherwise treat as a flat shape. | |
| is_multi_input_format = (partial_shape.size() == inputs.size()) && all_containers; |
| auto parse_dimension = [](py::handle dim_obj) -> ov::Dimension { | ||
| if (py::isinstance<py::list>(dim_obj)) { | ||
| throw std::runtime_error("Unexpected nested list in dimension specification."); | ||
| } | ||
| if (py::isinstance<py::tuple>(dim_obj)) { | ||
| py::tuple t = dim_obj.cast<py::tuple>(); | ||
| if (t.size() != 2) { | ||
| throw std::runtime_error( | ||
| "Two elements are expected in tuple(lower, upper) for dynamic dimension, but " + | ||
| std::to_string(t.size()) + " elements were given."); | ||
| } | ||
| if (!py::isinstance<py::int_>(t[0]) || !py::isinstance<py::int_>(t[1])) { | ||
| throw std::runtime_error("Tuple elements must be integers."); | ||
| } | ||
| int lower = t[0].cast<int>(); | ||
| int upper = t[1].cast<int>(); | ||
| return ov::Dimension(lower, upper); | ||
| } | ||
| if (py::isinstance<py::int_>(dim_obj)) { | ||
| return ov::Dimension(dim_obj.cast<int>()); | ||
| } | ||
| throw std::runtime_error("Invalid dimension type. Must be int or (lower, upper) tuple."); | ||
| }; |
There was a problem hiding this comment.
The multi-input parsing path only supports int and (lower, upper) tuples, and explicitly rejects list-based intervals. This is inconsistent with the existing reshape dimension syntax described in the docstring (and typically supported elsewhere via partial_shape_from_handle(...)). To avoid behavior divergence between flat vs multi-input forms, reuse the existing parsing helpers per shape (e.g., parse each inner shape with the same logic used by get_variables_shapes), instead of introducing a reduced/parallel parser.
| (2) `[min, max]`, dynamic dimension where `min` specifies lower bound and `max` specifies upper bound; | ||
| the range includes both `min` and `max`; using `-1` for `min` or `max` means no known bound (3) `(min, | ||
| max)`, the same as above (4) `-1` is a dynamic dimension without known bounds (4) | ||
| `openvino.Dimension` (5) `str` using next syntax: | ||
| '?' - to define fully dynamic dimension | ||
| '1' - to define dimension which length is 1 | ||
| '1..10' - to define bounded dimension | ||
| '..10' or '1..' to define dimension with only lower or only upper limit | ||
|
|
There was a problem hiding this comment.
The updated docstring claims [min, max], openvino.Dimension, and str forms are supported for dimensions, but the newly added multi-input parsing rejects list-based dims and any non-int/tuple dim types. Either expand the implementation to match the documented accepted forms (preferred), or update the docstring to reflect the narrower support.
| (2) `[min, max]`, dynamic dimension where `min` specifies lower bound and `max` specifies upper bound; | |
| the range includes both `min` and `max`; using `-1` for `min` or `max` means no known bound (3) `(min, | |
| max)`, the same as above (4) `-1` is a dynamic dimension without known bounds (4) | |
| `openvino.Dimension` (5) `str` using next syntax: | |
| '?' - to define fully dynamic dimension | |
| '1' - to define dimension which length is 1 | |
| '1..10' - to define bounded dimension | |
| '..10' or '1..' to define dimension with only lower or only upper limit | |
| (2) `(min, max)`, dynamic dimension where `min` specifies lower bound and `max` specifies upper bound; | |
| the range includes both `min` and `max`; using `-1` for `min` or `max` means no known bound | |
| (3) `-1` is a dynamic dimension without known bounds |
| def model(): | ||
| return generate_multi_input_model() | ||
|
|
||
|
|
||
| def test_reshape_list_of_shapes(model): | ||
| """Multi-input: list-of-lists reshapes all inputs in order.""" | ||
| input_shapes = [[2, 2], [1, 3, 224, 244], [10]] | ||
|
|
||
| model.reshape(input_shapes) | ||
|
|
||
| assert model.input("A").shape == (2, 2) | ||
| assert model.input("B").shape == (1, 3, 224, 244) | ||
| assert model.input("C").shape == (10,) | ||
|
|
||
|
|
||
| def test_reshape_list_of_shapes_wrong_size(model): | ||
| """Providing wrong number of shapes should raise an error.""" | ||
| input_shapes = [[2, 2], [1, 3, 224, 244]] # Missing third input | ||
| with pytest.raises(RuntimeError, match="Number of shapes does not match"): | ||
| model.reshape(input_shapes) |
There was a problem hiding this comment.
The fixture is named model, which is very generic and can easily collide with other tests/helpers in this module (or unintentionally inject into any test that happens to accept a model parameter). Consider renaming it to something specific like multi_input_model, and updating test_reshape_list_of_shapes / test_reshape_list_of_shapes_wrong_size accordingly.
| def model(): | |
| return generate_multi_input_model() | |
| def test_reshape_list_of_shapes(model): | |
| """Multi-input: list-of-lists reshapes all inputs in order.""" | |
| input_shapes = [[2, 2], [1, 3, 224, 244], [10]] | |
| model.reshape(input_shapes) | |
| assert model.input("A").shape == (2, 2) | |
| assert model.input("B").shape == (1, 3, 224, 244) | |
| assert model.input("C").shape == (10,) | |
| def test_reshape_list_of_shapes_wrong_size(model): | |
| """Providing wrong number of shapes should raise an error.""" | |
| input_shapes = [[2, 2], [1, 3, 224, 244]] # Missing third input | |
| with pytest.raises(RuntimeError, match="Number of shapes does not match"): | |
| model.reshape(input_shapes) | |
| def multi_input_model(): | |
| return generate_multi_input_model() | |
| def test_reshape_list_of_shapes(multi_input_model): | |
| """Multi-input: list-of-lists reshapes all inputs in order.""" | |
| input_shapes = [[2, 2], [1, 3, 224, 244], [10]] | |
| multi_input_model.reshape(input_shapes) | |
| assert multi_input_model.input("A").shape == (2, 2) | |
| assert multi_input_model.input("B").shape == (1, 3, 224, 244) | |
| assert multi_input_model.input("C").shape == (10,) | |
| def test_reshape_list_of_shapes_wrong_size(multi_input_model): | |
| """Providing wrong number of shapes should raise an error.""" | |
| input_shapes = [[2, 2], [1, 3, 224, 244]] # Missing third input | |
| with pytest.raises(RuntimeError, match="Number of shapes does not match"): | |
| multi_input_model.reshape(input_shapes) |
| def generate_multi_input_model(): | ||
|
|
There was a problem hiding this comment.
generate_multi_input_model introduces a new shared helper but lacks a docstring and type hints (unlike the newly added generate_single_input_model / generate_two_input_model). Also there’s an extra blank line after the def that doesn’t match the surrounding style. Consider adding a short docstring/return annotation and removing the extra blank line for consistency.
| def generate_multi_input_model(): | |
| def generate_multi_input_model() -> openvino.Model: | |
| """Generate a model with three inputs and three corresponding addition outputs.""" |
| assert "Incorrect key type <class 'openvino._pyopenvino.op.Parameter'> to reshape a model, " "expected keys as openvino.Output, int or str." in str( | ||
| e.value) |
There was a problem hiding this comment.
This assertion is now hard to read due to implicit string literal concatenation and an awkward line break. Consider refactoring to a single explicit message string (or using parentheses with a single string) and putting in str(e.value) on the same logical line to keep the test readable and easier to maintain.
- Resolved previous Python linter errors - Addressed new flake8 warnings (E501: line too long, E121/E126: indentation) - Fixed copyright header issue in CI - Verified build and tests locally
| When list or tuple are used to describe dimensions, each dimension can be written in form: | ||
|
|
||
| (1) non-negative `int` which means static value for the dimension | ||
| (2) `[min, max]`, dynamic dimension where `min` specifies lower bound and `max` specifies upper bound; | ||
| the range includes both `min` and `max`; using `-1` for `min` or `max` means no known bound (3) `(min, | ||
| max)`, the same as above (4) `-1` is a dynamic dimension without known bounds (4) | ||
| `openvino.Dimension` (5) `str` using next syntax: | ||
| '?' - to define fully dynamic dimension | ||
| '1' - to define dimension which length is 1 | ||
| '1..10' - to define bounded dimension | ||
| '..10' or '1..' to define dimension with only lower or only upper limit | ||
| (1) non-negative `int` which means static value for the dimension | ||
| (2) `(min, max)`, dynamic dimension where `min` specifies lower bound and `max` specifies upper bound; | ||
| the range includes both `min` and `max`; using `-1` for `min` or `max` means no known bound | ||
| (3) `-1` is a dynamic dimension without known bounds | ||
|
|
There was a problem hiding this comment.
[MEDIUM] The updated reshape docstring for the list overload no longer documents several dimension encodings that are still accepted by Common::partial_shape_from_list (e.g., [min, max] list intervals, openvino.Dimension, and str dimensions). Please align the docstring with the actual accepted formats to avoid misleading API users.
| assert shape[3].get_min_length() == 112 | ||
| assert shape[3].get_max_length() == 448 |
There was a problem hiding this comment.
[LOW] test_reshape_single_input_all_tuple_dims has duplicated assertions for shape[3].get_min_length() / get_max_length(); the last two lines repeat the previous two. Please remove the duplicates to keep the test concise.
| assert shape[3].get_min_length() == 112 | |
| assert shape[3].get_max_length() == 448 |
| from openvino import Core, Dimension, Layout, Model, Output, OVAny, \ | ||
| PartialShape, Shape, Tensor, Type, get_batch, save_model, serialize, set_batch |
There was a problem hiding this comment.
[LOW] The from openvino import ... statement now uses a backslash line continuation. In this test suite, multi-line imports are typically parenthesized (and this file previously used that style), which is less error-prone and easier to reformat. Please switch back to the parenthesized import style for consistency.
| from openvino import Core, Dimension, Layout, Model, Output, OVAny, \ | |
| PartialShape, Shape, Tensor, Type, get_batch, save_model, serialize, set_batch | |
| from openvino import ( | |
| Core, | |
| Dimension, | |
| Layout, | |
| Model, | |
| Output, | |
| OVAny, | |
| PartialShape, | |
| Shape, | |
| Tensor, | |
| Type, | |
| get_batch, | |
| save_model, | |
| serialize, | |
| set_batch, | |
| ) |
| bool has_int = false; | ||
|
|
||
| for (size_t i = 0; i < partial_shape.size(); ++i) { | ||
| py::handle elem = partial_shape[i]; | ||
| if (py::isinstance<py::int_>(elem)) { | ||
| has_int = true; |
There was a problem hiding this comment.
[BLOCKER] has_int is assigned but never used in the new reshape(py::list, ...) overload. OpenVINO builds commonly treat warnings as errors, so this can break compilation. Please remove the unused variable (and any related dead logic) or use it meaningfully.
| bool has_int = false; | |
| for (size_t i = 0; i < partial_shape.size(); ++i) { | |
| py::handle elem = partial_shape[i]; | |
| if (py::isinstance<py::int_>(elem)) { | |
| has_int = true; | |
| for (size_t i = 0; i < partial_shape.size(); ++i) { | |
| py::handle elem = partial_shape[i]; | |
| if (py::isinstance<py::int_>(elem)) { |
a1299e3 to
c4b1202
Compare
|
please update submodules to revert unintentionally catched thirdparty file |
This reverts commit 446f1da.
ee0dded to
11dd04c
Compare
Changes
New Overload: Adds a new overload to [Model.reshape] that accepts a list of shapes (e.g., [model.reshape([[2,2], [1,3,224,244], [10]]). Each shape in the list is assigned to the corresponding input in order.
Simplified API: This enables a more concise and intuitive way to reshape all model inputs, improving usability for common scenarios.
Tests: Adds unit tests to ensure the new API works as expected, including error handling for mismatched input counts.
Description
This PR also expands the Python API unit test suite for
Model.reshape()to cover additional usage patterns, edge cases, and validation scenarios. It builds on the existing reshape tests by:PartialShapewith tensor name coverage.All new tests follow the same upstream style:
Testing
All new tests pass locally against the latest OpenVINO build.
A clean version of: #31871
Closes: #18347