Raise overload resolution exception in Python bindings if overload resolution fails#19312
Conversation
519e661 to
2190d33
Compare
alalek
left a comment
There was a problem hiding this comment.
Does it make sense to switch conversions errors to C++ exceptions (instead of creation of Python exceptions)?
It is long term task, because we have to update all conversion functions to this logic and C++ exceptions have higher costs for this task - it looks like we need something that behaves in the same way as purposed |
2190d33 to
69b57eb
Compare
modules/python/src2/cv2.cpp
Outdated
| const std::size_t header_size = 27; | ||
| const std::string bullet = "\n - "; | ||
|
|
||
| // Estimate required buffer size - save dynamic memory allocations = faster |
There was a problem hiding this comment.
Raise
This code works in case of errors only, so it is not a part of code flow critical path (no need to optimize this aggressively).
There was a problem hiding this comment.
True, but it is ~x8 times faster than std::ostringstream variant... 🤓
modules/python/src2/gen2.py
Outdated
| ' conversionErrors.reserve({});\n'.format(len(all_code_variants)) | ||
| code += ' \n'.join(gen_template_overloaded_function_call.substitute(variant=v) | ||
| for v in all_code_variants) | ||
| code += ' if (!conversionErrors.empty())\n' \ |
There was a problem hiding this comment.
Could this condition be false? (conversionErrors.empty() is true)
There was a problem hiding this comment.
In normal scenario: no - every overload resolution should succeed or fail setting an error, but there might be ill-formed pyopencv_to functions e.g. pyopencv_to_generic_vec - which has code path returning false, but without setting an error moreover there are some generated conversion (still need to be checked).
I think we can leave this check until stated problem is resolved (all conversions are tested - there not so many left, because we need to implement correct parsing objects those conform to sequence protocol e.g. tuples, lists, 1d numpy arrays or user classes (will be supported automatically)). What do you think?
2c0e7c1 to
1ab1c08
Compare
modules/python/src2/cv2.cpp
Outdated
| { | ||
| const std::vector<std::string>& conversionErrors = conversionErrorsTLS.getRef(); | ||
| const std::size_t conversionErrorsCount = conversionErrors.size(); | ||
| if (conversionErrorsCount > 0) { |
There was a problem hiding this comment.
Perhaps separate function can be extracted to be reused later + better readability:
string concatStrings(const list<string> & items, const string & prefix = string(), const string & item_prefix = srting());There was a problem hiding this comment.
I don't think that we can leave this 1 time used code here, if it occurs that somebody want's to concatenate strings in this module too - definitely function should be extracted.
1ab1c08 to
a0bdb78
Compare
Python doesn't support function overloads, but OpenCV is C++ library and massively utilize them. If user has wrong input for overloaded function he/she receives only the latest conversion error message and sometimes it is not clear what is going on.
If every argument parsing function reports good diagnostic message why the argument can't be parsed, user will have sufficient information to fix the problem - overload resolution exception contains information about each conversion error.
Resolves #16432, #15465
Example error message output:
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.