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

[Bug][python binding] _set_node_friendly_name() got multiple values for argument 'node' when node kwarg is used on node creation #15999

Closed
3 tasks done
daniil-lyakhov opened this issue Feb 28, 2023 · 5 comments · Fixed by #20695
Assignees
Labels
bug Something isn't working category: Python API OpenVINO Python bindings

Comments

@daniil-lyakhov
Copy link
Contributor

System information (version)
  • OpenVINO => openvino-dev==2022.3.0
  • Operating System / Platform => Ubuntu20
  • Problem classification: ov.Node creation
Detailed description

openvino/runtime/utils/decorators.py: nameable_op wrapper raise a runtime error if 'node' is passed to wrapped function as kwarg

src: https://github.com/openvinotoolkit/openvino/blob/master/src/bindings/python/src/openvino/runtime/utils/decorators.py#L18-L27

Steps to reproduce
from openvino.runtime import opset9 as opset

const = opset.constant([-1], np.int64)
min_op = opset.reduce_min(node=const, reduction_axes=0)

Following error is raised:

File "/home/dlyakhov/env/nncf_pt/lib/python3.8/site-packages/openvino/runtime/utils/decorators.py", line 24, in wrapper
    node = _set_node_friendly_name(node, **kwargs)
TypeError: _set_node_friendly_name() got multiple values for argument 'node'

Following code works as expected:

from openvino.runtime import opset9 as opset

const = opset.constant([-1], np.int64)
min_op = opset.reduce_min(const, reduction_axes=0)
Issue submission checklist
  • I report the issue, it's not a question

  • I checked the problem with documentation, FAQ, open issues, Stack Overflow, etc and have not found solution

  • There is reproducer code and related data files: images, videos, models, etc.

@akuporos
Copy link
Contributor

akuporos commented Feb 28, 2023

Hello @daniil-lyakhov,

Could you please tell me why do you want to pass node as keyword argument?

Actually, you may see that the function's signature is _set_node_friendly_name(node, **kwargs). So node is a positional argument, which is required here and cannot be passed as a keyword argument.

Best regards,
Anastasia

Ref: 105065

@daniil-lyakhov
Copy link
Contributor Author

daniil-lyakhov commented Mar 1, 2023

Hi @akuporos, than you for a quick response!
l've passed node argument as keywork for the sake of code readability. Yes, I understand the problem with _set_node_friendly_name(node, **kwargs), but when I see a python function, usually I expect that I can use keyword argument for any argument, and this is not the case for named nodes creation. Neither docs nor code stops me from putting node attribute as a keyword, and the error log looks odd at first sight. This logic looks invalid for me.

Could we remove this implicit limitation? Could _set_node_friendly_name(node, **kwargs) receive (*args, **kwargs) instead?

@p-wysocki
Copy link
Contributor

Could we remove this implicit limitation? Could _set_node_friendly_name(node, **kwargs) receive (*args, **kwargs) instead?

I think that's a very valid request, since _set_node_friendly_name(node, **kwargs) is an internal function we could just change it.

The second option is (Anastasia's finding!) using slash in function arguments, however it has been introduced in Python 3.8 and we still support Python 3.7. We could wait for dropping Python 3.7 and solve the issue in an elegant, Pythonic way.

Personally I think we should go for the second option. What do you think @akuporos @jiwaszki?

@ilya-lavrenov ilya-lavrenov added the category: Python API OpenVINO Python bindings label Mar 2, 2023
@jiwaszki
Copy link

jiwaszki commented Mar 6, 2023

@p-wysocki +1 for the second option. Ticket is already in the backlog for the future release (waiting for the drop of 3.7).
Won't fix it in this release.
I will keep it open for the future reference.

@akladiev
Copy link
Collaborator

This issue will be closed in 2 weeks in case of no activity.

@akladiev akladiev added the Stale label May 13, 2023
@akuporos akuporos linked a pull request Oct 25, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working category: Python API OpenVINO Python bindings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants