[Good First Issue][PyOV]: Support slice and negative indexes for PartialShape and Shape

[jiwaszki]

Context

This change will align the behavior of container-like OpenVINO structure to feel more pythonic.

What needs to be done?

• Edit existing or add overloads of __getitem__ for PartialShape and Shape classes to behave like Python lists.
• Add various testcases to verify changes.

Example code that should be passing:

from openvino import Shape, PartialShape

a = [1, 2, 3]

s = Shape(a)
assert a[-1] == s[-1]
assert a[0:2] == s[0:2]

ps = PartialShape(a)
assert a[-2] == ps[-2]
assert a[1:2] == ps[1:2]

Example Pull Requests

N/A

Resources

• Contribution guide - start here!
• Intel DevHub Discord channel - engage in discussions, ask questions and talk to OpenVINO developers
• Reference from Stack Overflow

Contact points

@jiwaszki @akuporos @p-wysocki

Ticket

75641

[sanbuphy]

i can try this

[p-wysocki]

Awesome, let us know if you have any questions either here or on Discord. :)

[ilya-lavrenov]

@jiwaszki should negative indexes be supported on C++ level as well ?

[jiwaszki]

@ilya-lavrenov a really good question. This makes a lot of sense from Python perspective but is it common practice in C++? It can be quite handy, so I am leaning into adding them to C++ as well. Although the effort of adding it to C++ should be separate (to not overdo this GFI) and later impls can be switched/influence each other.

Just a note: slices do not need to be supported -- if anything should mimic them in C++ that would be ranges.

Let's move the discussion outside this GFI.

@praasz FYI, to be in the loop as Core related topic.

[sanbuphy]

Oh, it seems the issue has been resolved. @jiwaszki the negative idx ,
but it's a good example to me

[ilya-lavrenov]

@sanbuphy it's resolved only on C++ side, while Python implementation is not finished

[jiwaszki]

I am reserving this task for @imperixxl on GSoC for Project 7.

@p-wysocki can you assign him? Seems like I can't do it by hand for some reason. (@sanbuphy gave me permission to unpin her from task, thank you!)

[imperixxl]

.take

[github-actions]

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

[imperixxl]

Hi, I wanted some help regarding the building of environment. I started with the normal windows build (did it twice) but it didn't work, so I removed everything and started fresh with Ubuntu 22. The issue is in the code and not the build, I am building everything correctly and the commands work as well but the Python environment is where I face issues; and the dependency issues of course. I've read stack overflow posts and old discussions in the repository but I couldn't fix it at all.
Examples of the few errors I am getting: import could not be resolved, attempted relative import with no known parent package, undefined variables (like ov, pytest etc) and name followed by '::' must be a class or namespace name. i have tried putting the compilers on path, checking the environment variables over and over again, but I cannot get rid of the errors (they just stack up every file i go to). i performed a clean install of the repo and build, tried windows, dockerfile, and linux installation but the errors don't go. For reference, the first time is see a dependency error arises in the second example (the first example works as there is no dependency)
PS this is happening in a pyenv directory with all necessary imports

As for the actual issue, I navigated through the openvino folders on github itself to locate openvino/src/bindings/python/src/pyopenvino/graph to find the class definitions of Shape/PartialShape; which further contain the getitem function that I have to edit.

Any help would be appreciated, thanks

[imperixxl]

Hi, I was wondering if someone is going to help or reply. Please unassign the task if you're not going to help. Thanks

[jiwaszki]

@imperixxl I can't reproduce this issue. For reference, I am working on ubuntu22.04, building on commit from Feb 29 (45ce9e7) with preferred way of creating wheels and installing them into environment. Here are results of following instructions:

Can you share any reports from "examples of the few errors"? I cannot deduce what are the issues and possible fixes from descriptions only.

If you have issues with building, please consider opening issue for OpenVINO. It may be that some edge-cases are broken or unsupported.

If the build stage takes too long, consider what can be removed for it, i.e. add to cmake build command ENABLE_TESTS=OFF that will turn off them. It's up to you to find minimal build scenario that works for you and your case, for everyone it is different. What is your command now?

[imperixxl]

@jiwaszki The build doesn't throw any errors, it gets completed without any errors (except some copyright warnings which I suppose can be ignored). For the build command, I am currently using cmake -DCMAKE_BUILD_TYPE=Release .. cmake --build . --parallel 4 followed by the commands for Python runtime API. Also, I am using Ubuntu 22.04 through WSL in win11.

• The first time I encounter a problem is when I run tests for the Python API. I've installed the requirements file and added the path to the user environment variables as well.
  When I run the first test (pytest tests/) it throws 3 errors (that have been thrown every single time I've built the environment).
  
  You can have a look at the whole test here; but the problem primarily lies in the module opset14; I did look the folder up in my code editor and found the folder (ops14) itself. I cannot ignore these test errors as the code I need to change in my gfi is using "cache" from openvino.properties and probably opset14 somewhere too, and I can see the errors there as well.
  Also, sometimes when I'm running the tests (in this case tox , it says that:
  

• If I ignore these, and move on to the examples, I successfully run the first example, but get an attribute error in the second.
  
  I double checked the directories, the files and their contents; but I really don't know what is happening.

• As for the actual code example: here I am editing shape.cpp; and the first time I run the file (without making any changes); I get this:
  
  I tried including the path of the actual directory in which shape.hpp is located to the C++ configurations, but it didn't work. I assume its an issue with file dependencies; as I mentioned earlier, python throws the "No such file or directory" even when the file is present at the specified location.
  If I somehow get rid of all the errors, I start getting class errors such as name followed by '::' must be a class or namespace name and variable errors.

• I also wanted to ask what workflow runs are, I get emails regarding the same (usually cancelled or failed)

[jiwaszki]

@imperixxl can you share the result of running python -c "import openvino as ov; print(ov); print(ov.__version__)" and pip show openvino and what is your PATH/PYTHONPATH ?

[imperixxl]

@jiwaszki text for your reference

[jiwaszki]

@imperixxl I think you have mismatching versions of OpenVINO itself between repository and what's in your PATH/PYTHONPATH/pip. You try to run tests for opset14 which is not present in 2023.3 release of OV (ref: https://github.com/openvinotoolkit/openvino/tree/2023.3.0/src/bindings/python/src/openvino/runtime). Please clean it up and try again. (PS I really recommend using wheels produced by build, it makes managing easier)

[imperixxl]

Thank you for your time @jiwaszki
Should I start fresh by deleting and cloning the repository again? I did update my package to 2024.0.0 but the test command is now throwing 6 errors(different from the last ones). Moreover, I would like to follow the wheels build if that will make things easier; also please mention how to go about the wheel build if possible.

[jiwaszki]

@imperixxl if you get the latest main branch there will be some changes between it and predownloaded package 2024.0 (you can check it with git status or even better git log while in openvino folder). To build wheels please refer to i.e. linux build instructions -- it should cover this path as well.

[jiwaszki]

Due to inactivity, I am self-assigning it. Awaiting another contributor @LucaTamSapienza to be assigned here.

[LucaTamSapienza]

.take

[github-actions]

Thanks for being interested in this issue. It looks like this ticket is already assigned to a contributor. Please communicate with the assigned contributor to confirm the status of the issue.

[LucaTamSapienza]

Hi @jiwaszki ,
In the Issue specification, you provided an example of what I should obtain after correctly extending Shape and Partial_Shape. However, when I try some tests for slicing, I get an assert error due to Python not knowing how to compare Shape/Partial_Shape and list objects. By casting the result to a list, I obtain correct results instead. Here's a code example for clarification:

from openvino import Shape, PartialShape

a = [1, 2, 3]

s = Shape(a)
assert a[-1] == s[-1]
assert a[0:2] == list(s[0:2]) #casting

ps = PartialShape(a)
assert a[-2] == ps[-2]
assert a[1:2] == list(ps[1:2]) #casting

My question is whether my interpretation is correct or if I might want to obtain a correct output when comparing Shape/PartialShape and list objects.

[jiwaszki]

Hi @jiwaszki , In the Issue specification, you provided an example of what I should obtain after correctly extending Shape and Partial_Shape. However, when I try some tests for slicing, I get an assert error due to Python not knowing how to compare Shape/Partial_Shape and list objects. By casting the result to a list, I obtain correct results instead. Here's a code example for clarification:

from openvino import Shape, PartialShape

a = [1, 2, 3]

s = Shape(a)
assert a[-1] == s[-1]
assert a[0:2] == list(s[0:2]) #casting

ps = PartialShape(a)
assert a[-2] == ps[-2]
assert a[1:2] == list(ps[1:2]) #casting

My question is whether my interpretation is correct or if I might want to obtain a correct output when comparing Shape/PartialShape and list objects.

@LucaTamSapienza Good observation but do you know the reason for it? What is the type returned from slicing operation? Why casting should appear?

[LucaTamSapienza]

@jiwaszki
After performing the slicing operation for Shape/PS objects, they are returning a Shape/PS object. Instead The slicing of a list in Python returns a list object, so when I try to compare using '==', I'll get false even if the values are the same due to this mismatch between list and Shape/PS. To have a correct comparison without needing to cast, I should return a list object after performing the slicing for Shape/PS, but in that case, I would lose the properties of the latter.
I'll create a PR to show you the changes I've made.

[jiwaszki]

@LucaTamSapienza please link PR here.

So how would you approach this issue? I think that #22316 targets the issue mentioned by you.

[LucaTamSapienza]

So how would you approach this issue?

Normally I would change the eq function to handle different objects for Shape and PS.

#22316

Since someone is already working on something similar in this PR, I would leave the test with the explicit casting, and after this PR is merged, I would delete it.

@LucaTamSapienza please link PR here.

Sure @jiwaszki , sorry for the late reply.
Here is my PR #23725