[PyOV] eq for parameter and result #26613

akuporos · 2024-09-16T11:22:20Z

Details:

add overloads for __eq__ and __hash__

Tickets:

CVS-131039

mitruska

Unfortunately, the changes are not fully backward compatible, because some of the basic python operators including "==" are overriten for Node by ov ops here:

openvino/src/bindings/python/src/openvino/runtime/__init__.py

Lines 69 to 85 in 68f7d33

    
           # Extend Node class to support binary operators 
        
           Node.__add__ = opset13.add 
        
           Node.__sub__ = opset13.subtract 
        
           Node.__mul__ = opset13.multiply 
        
           Node.__div__ = opset13.divide 
        
           Node.__truediv__ = opset13.divide 
        
           Node.__radd__ = lambda left, right: opset13.add(right, left) 
        
           Node.__rsub__ = lambda left, right: opset13.subtract(right, left) 
        
           Node.__rmul__ = lambda left, right: opset13.multiply(right, left) 
        
           Node.__rdiv__ = lambda left, right: opset13.divide(right, left) 
        
           Node.__rtruediv__ = lambda left, right: opset13.divide(right, left) 
        
           Node.__eq__ = opset13.equal 
        
           Node.__ne__ = opset13.not_equal 
        
           Node.__lt__ = opset13.less 
        
           Node.__le__ = opset13.less_equal 
        
           Node.__gt__ = opset13.greater 
        
           Node.__ge__ = opset13.greater_equal

This is why the test for operator.eq case is failing:

openvino/src/bindings/python/tests/test_graph/test_ops_binary.py

Line 152 in 3349870

def test_binary_operators_with_scalar(operator, expected_type):

The motivation to align .index() behavior is reasonable, but the impact and consistency should be verified.

Also there are existing methods that can be used to retrieve the index model.get_parameter_index(parameter) or model.get_result_index(parameter).

mitruska · 2024-09-17T09:00:10Z

src/bindings/python/tests/test_graph/test_basic.py

@@ -61,6 +61,7 @@ def test_graph_api():
    assert list(model.get_output_shape(0)) == [2, 2]
    assert (model.get_parameters()[1].get_partial_shape()) == PartialShape([3, 4, 5])
    assert len(model.get_parameters()) == 3
+    assert model.get_parameters().index(parameter_c) == 2


There is also model.get_parameter_index(parameter) available in Python and C++ OV API.

openvino/src/bindings/python/src/pyopenvino/graph/model.cpp

Lines 1094 to 1099 in 68f7d33

model.def(

"get_parameter_index",

(int64_t(ov::Model::*)(const std::shared_ptr<ov::op::v0::Parameter>&) const) & ov::Model::get_parameter_index,

py::arg("parameter"),

R"(

Return the index position of `parameter`

mitruska · 2024-09-17T09:00:58Z

src/bindings/python/tests/test_runtime/test_model.py

@@ -748,6 +748,7 @@ def test_model_add_remove_result_parameter_sink():

    results = model.get_results()
    assert len(results) == 2
+    assert results.index(result2) == 1


There is also model.get_result_index(parameter) available in Python and C++ OV API.

openvino/src/bindings/python/src/pyopenvino/graph/model.cpp

Lines 848 to 852 in 68f7d33

model.def("get_result_index",

(int64_t(ov::Model::*)(const ov::Output<ov::Node>&) const) & ov::Model::get_result_index,

py::arg("value"),

R"(

Return index of result.

mitruska · 2024-09-17T09:05:34Z

src/bindings/python/src/pyopenvino/graph/ops/parameter.cpp

+    parameter.def(
+        "__eq__",
+        [](const ov::op::v0::Parameter& a, const ov::op::v0::Parameter& b) {
+            return a.get_instance_id() == b.get_instance_id();
+        },
+        py::is_operator());


It changes behaviour of ==, but != is still overwritten by ov.op.not_equal, which can lead to inconsistency:

openvino/src/bindings/python/src/openvino/runtime/__init__.py

Lines 80 to 81 in 68f7d33

Node.__eq__ = opset13.equal

Node.__ne__ = opset13.not_equal

akuporos · 2024-09-24T12:37:48Z

I close it for right now.
Will restore when the right time come

### Details: - #26613 caused backward incompatible changes, so decided to deprecate comparison overloads for comparison operations as the usage of them confuse users - suppress deprecation warnings in tests - added bindings for `get_instance_id()` to Node class ### Tickets: - result of discussion [CVS-131039](https://jira.devtools.intel.com/browse/CVS-131039)

[PyOV] __eq__ for parameter and result

1dff7b8

akuporos requested a review from a team as a code owner September 16, 2024 11:22

github-actions bot added the category: Python API OpenVINO Python bindings label Sep 16, 2024

akuporos requested a review from praasz September 16, 2024 11:25

akuporos added this to the 2024.5 milestone Sep 16, 2024

akuporos requested review from p-wysocki and almilosz September 16, 2024 11:28

codestyle

a5c71f7

p-wysocki approved these changes Sep 16, 2024

View reviewed changes

akuporos requested a review from mitruska September 16, 2024 17:47

mitruska reviewed Sep 17, 2024

View reviewed changes

akuporos removed this from the 2024.5 milestone Sep 18, 2024

akuporos added the WIP work in progress label Sep 18, 2024

akuporos mentioned this pull request Sep 24, 2024

[PyOV] Deprecate comparison for operations #26761

Merged

akuporos closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PyOV] eq for parameter and result #26613

[PyOV] eq for parameter and result #26613

Uh oh!

akuporos commented Sep 16, 2024

Uh oh!

mitruska left a comment

Uh oh!

mitruska Sep 17, 2024

Uh oh!

mitruska Sep 17, 2024

Uh oh!

mitruska Sep 17, 2024

Uh oh!

akuporos commented Sep 24, 2024

Uh oh!

Uh oh!

	# Extend Node class to support binary operators
	Node.__add__ = opset13.add
	Node.__sub__ = opset13.subtract
	Node.__mul__ = opset13.multiply
	Node.__div__ = opset13.divide
	Node.__truediv__ = opset13.divide
	Node.__radd__ = lambda left, right: opset13.add(right, left)
	Node.__rsub__ = lambda left, right: opset13.subtract(right, left)
	Node.__rmul__ = lambda left, right: opset13.multiply(right, left)
	Node.__rdiv__ = lambda left, right: opset13.divide(right, left)
	Node.__rtruediv__ = lambda left, right: opset13.divide(right, left)
	Node.__eq__ = opset13.equal
	Node.__ne__ = opset13.not_equal
	Node.__lt__ = opset13.less
	Node.__le__ = opset13.less_equal
	Node.__gt__ = opset13.greater
	Node.__ge__ = opset13.greater_equal

	model.def(
	"get_parameter_index",
	(int64_t(ov::Model::*)(const std::shared_ptr<ov::op::v0::Parameter>&) const) & ov::Model::get_parameter_index,
	py::arg("parameter"),
	R"(
	Return the index position of `parameter`

	model.def("get_result_index",
	(int64_t(ov::Model::*)(const ov::Output<ov::Node>&) const) & ov::Model::get_result_index,
	py::arg("value"),
	R"(
	Return index of result.

[PyOV] __eq__ for parameter and result #26613

[PyOV] __eq__ for parameter and result #26613

Uh oh!

Conversation

akuporos commented Sep 16, 2024

Details:

Tickets:

Uh oh!

mitruska left a comment

Choose a reason for hiding this comment

Uh oh!

mitruska Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

mitruska Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

mitruska Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

akuporos commented Sep 24, 2024

Uh oh!

Uh oh!

[PyOV] eq for parameter and result #26613

[PyOV] eq for parameter and result #26613