Skip to content
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

Extend Python API with a new model.reshape() overload #18347

Open
p-wysocki opened this issue Jul 3, 2023 · 44 comments · May be fixed by #25133
Open

Extend Python API with a new model.reshape() overload #18347

p-wysocki opened this issue Jul 3, 2023 · 44 comments · May be fixed by #25133
Assignees
Labels
category: Python API OpenVINO Python bindings good first issue Good for newcomers no_stale Do not mark as stale

Comments

@p-wysocki
Copy link
Contributor

p-wysocki commented Jul 3, 2023

Context

Currently there are multiple ways to reshape model inputs in Python API. They are defined as bindings to C++ code.

A new way to reshape has been suggested - implicit input reshaping with a list of input shapes. An example of new, desired overload/Python API function:

  1. model has three inputs: A, B and C
  2. model is being reshaped with a list of shapes:
    model.reshape([[2,2], [1, 3, 224, 244], [10]])
  3. the given shapes are assigned to respective inputs:
    A.shape = [2, 2]
    B.shape = [1, 3, 244, 244]
    C.shape = [10]

Python API model binding needs to be extended with a new overload which will add this functionality.

What needs to be done?

  1. Add a new overload for model.reshape in model C++ bindings
  2. Add tests for the new functionality in the correct test file - test_model.py
  3. Submit a PR
  4. Wait for review

Resources

Contact points

@p-wysocki
@jiwaszki
@akuporos

Don't hesitate to reach out, we're here to help!

Ticket: 110444

@p-wysocki p-wysocki added good first issue Good for newcomers no_stale Do not mark as stale labels Jul 3, 2023
@ilya-lavrenov
Copy link
Contributor

Do we need any placeholder if I want to change only specific shapes?
E.g. model has 3 inputs and old last one needs to be changed.

@p-wysocki
Copy link
Contributor Author

I guess in that case the user should use dict overload, where a specific input can be reshaped. Either that or we do the placeholder you suggested, but I think that whatever the placeholder would be, it wouldn't be clear. We can't do -1 since it means a dynamic shape, None I think would look a bit weird, maybe some string? I guess it's a thing to discuss.

cc @jiwaszki @akuporos

@dev-seek
Copy link

dev-seek commented Jul 4, 2023

hey @p-wysocki @ilya-lavrenov I want to work on this can you please assign it to me

@dev-seek
Copy link

dev-seek commented Jul 4, 2023

thanks a lot for assigning me , i will surely come up with doubts or updates if any

@p-wysocki
Copy link
Contributor Author

@dev-seek Thanks for taking a look, we're here to answer any questions. :)

@jiwaszki
Copy link

I guess in that case the user should use dict overload, where a specific input can be reshaped. Either that or we do the placeholder you suggested, but I think that whatever the placeholder would be, it wouldn't be clear. We can't do -1 since it means a dynamic shape, None I think would look a bit weird, maybe some string? I guess it's a thing to discuss.

cc @jiwaszki @akuporos

As for placeholder values, there is a problem with adding them. What should be a concept instead of integers? Enums like ov.PartialShape.KEEP and consequence of it ov.Dimension.KEEP? Or just a string value KEEP?

I think that placeholder values are not in the scope of this task and @p-wysocki solution with dict is suitable for now (e.x. {1: [1, 21, 37]}).

In general case user could perform something like model.reshape([model.inputs[0].shape, [1, 22, 48], model.inputs[0].partial_shape) where shape/partial_shape can be used interchangeably depending on dynamism of given model inputs. It's worth adding a test-case(s) for such usage.

@dev-seek
Copy link

hey @p-wysocki @jiwaszki Just to clearify can i build the whole on Debian as here -> https://github.com/openvinotoolkit/openvino/blob/master/docs/dev/build_linux.md its written for Ubuntu can i do the same for Debian as Ubuntu is based on Debian

@p-wysocki
Copy link
Contributor Author

Hi @dev-seek, as far as I know this instruction works for Debian. If you encounter any issues please let us know.

@Jay-sanjay
Copy link

hey is the issue resolve I want to contribute . I am new to open-source contribution

@dev-seek
Copy link

Hey @Jay-sanjay i am already contributing to it

@p-wysocki
Copy link
Contributor Author

I'm returning the task to being open due to current assignee's inactivity.

@dev-seek
Copy link

Sry @p-wysocki i am not inactive in middle i am suffering from a bad health issue related to heart...
But believe me i am on after a week...
If u can plz remain it as it is..

@dev-seek
Copy link

i really want this to be done if u can, please reassign it to me @p-wysocki

@p-wysocki
Copy link
Contributor Author

Sure, get well soon. :)

@dev-seek
Copy link

Thanks a lot @p-wysocki
Bed rest of 1 week is remaining, not suggested to work via doc...
After a week i will be back and start working on it for sure

@p-wysocki
Copy link
Contributor Author

I'm returning the task to being open due to current assignee's inactivity.

@siddhant-0707
Copy link
Contributor

siddhant-0707 commented Sep 15, 2023

Hey @p-wysocki doesn't this already solve this issue?

@p-wysocki
Copy link
Contributor Author

@slyalin could you please take a look? IIRC this feature was at your request, do you think there's a benefit on expanding our API to also accepting lists, when we have an overload which @siddhant-0707 linked?

@p-wysocki
Copy link
Contributor Author

@slyalin

@slyalin
Copy link
Contributor

slyalin commented Nov 20, 2023

@p-wysocki, the mentioned by @siddhant-0707 method accepts py::dict, this ticket is about a list. I don't see how they are connected. The problem when we want to skip part of the arguments in reshape and still want to use a method that accepts a list of shapes should be left as-is -- without any resolution, because in this case, indeed, mentioned by @siddhant-0707 method with py::dict should be used.

So we still don't have a method that just accepts the list of shapes. In the example from the description, instead of model.reshape([[2,2], [1, 3, 224, 244], [10]]) we still need to write model.reshape({0: [2,2], 1: [1, 3, 224, 244], 2: [10]}) which is rather annoying and is not aligned with capabilities provided for a compiled model used as a callable -- there I can use a list of inputs.

The real problem is how to align a newly proposed reshape method with other existing reshape methods: how to avoid ambiguities.

@mcshawn10
Copy link

want to make sure I understand this, basically to resolve this issue, go into model.cpp and add a new model.def("reshape", [](){ <code>} that will support the new desired functionality? if this seems to be an appropriate amount of understanding then I would like to pick up this issue

@mcshawn10
Copy link

@slyalin @p-wysocki

@mcshawn10
Copy link

Hi @p-wysocki, could you transfer assignment of this issue, thank you!

@mlukasze mlukasze assigned mcshawn10 and unassigned p-wysocki Dec 1, 2023
@mlukasze
Copy link
Contributor

mlukasze commented Dec 1, 2023

Hi @p-wysocki, could you transfer assignment of this issue, thank you!

you got it!
Sorry for inconvenience, we have to seriously talk with our bot ;)
have fun!

@p-wysocki
Copy link
Contributor Author

Hello @mcshawn10, is there anything we can help you with? Do you have any questions?

@mcshawn10
Copy link

@p-wysocki hi it's been a slow start, no questions yet, thank you!

@p-wysocki
Copy link
Contributor Author

Just yesterday our CONTRIBUTING.md has been updated with a technical guide - I highly recommend checking it out. :)

@daksh-code
Copy link

.take

Copy link
Contributor

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.

@mcshawn10 mcshawn10 removed their assignment Dec 17, 2023
@mcshawn10
Copy link

@daksh-code @p-wysocki removing my assignment since I have been working on other things and some else wants it, have fun!

@daksh-code
Copy link

.take

Copy link
Contributor

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

@p-wysocki
Copy link
Contributor Author

Hello @daksh-code, do you need any help?

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.

@siddhant-0707
Copy link
Contributor

.take

Copy link
Contributor

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

@siddhant-0707
Copy link
Contributor

PR #22171

@amkarn258
Copy link
Contributor

Hi @akuporos ,

Sure I can work on this. Can you please assign this to me?

@amkarn258
Copy link
Contributor

Hi,

It seems clear from the description that input is list of shapes, but want to confirm it again. It's list of shapes right? not list of lists?

@amkarn258
Copy link
Contributor

Hi @akuporos ,

Have added a PR for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Python API OpenVINO Python bindings good first issue Good for newcomers no_stale Do not mark as stale
Projects
Status: In Review