Python submodules initialization fixes backport to 4.x#21489
Merged
opencv-pushbot merged 1 commit intoopencv:4.xfrom Jan 27, 2022
Merged
Conversation
VadimLevin
commented
Jan 20, 2022
modules/python/test/test_misc.py
Outdated
| msg="Module is not generated for nested namespace") | ||
| self.assertTrue(hasattr(cv.utils.nested, "testEchoBooleanFunction"), | ||
| msg="Function in nested module is not available") | ||
| self.assertEqual(sys.getrefcount(cv.utils.nested), 3, |
Contributor
Author
There was a problem hiding this comment.
In contrast with 3.4 branch - reference count should be 3, due to changes in how submodules are exported to package.
Contributor
Author
|
Investigation is required why Python 2 and Python 3 reports different reference counting for the nested submodule. |
Member
|
Update: Currently both Python2/3 using loader. |
Contributor
Author
I want to understand why it happens.
Apart from that, the logic for 2 is also valid: import cv2
import sys
print(sys.getrefcount(cv2.utils)) # Python 3 outputs: 3 | Python 2 outputs: 4
print(sys.getrefcount(cv2.utils) == sys.getrefcount(cv2._native.utils)) # Both versions output: True |
Member
|
@VadimLevin Could you please to finalize this PR? We need this port into 4.x branch for regular "Merge 3.4". Main difference in Python2/3 is supporting of "Python code" in bindings. Python 2:
Python 3:
I believe we could add "if" with Python version check for this check. |
4dece69 to
3364d53
Compare
Member
|
OSX crashes with Python2 during the module cleanup. Some debug logs (not really useful): Details |
Contributor
Author
I don't have an access to OSX machine to troubleshoot this crash. import cv2For example, on my Ubuntu 20.04 with Python 2.7.18 I've got the following filtered initialization/clean-up output |
alalek
reviewed
Jan 27, 2022
modules/python/src2/cv2.cpp
Outdated
| /// PyDict_SetItemString doesn't steal a reference so the reference counter | ||
| /// of the submodule should be decremented to bind submodule lifetime to the | ||
| /// parent module | ||
| Py_DECREF(submodule); |
Member
There was a problem hiding this comment.
There are OSX logs before and after removal of this line:
Member
There was a problem hiding this comment.
PyImport_AddModule() docs:
- Python3: https://docs.python.org/3/c-api/import.html
- Python2: https://docs.python.org/2/c-api/import.html
Return value: Borrowed reference
and description:
Return the module object corresponding to a module name. The name argument
may be of the formpackage.module. First check the modules dictionary if
there's one there, and if not, create a new one and insert it in the modules
dictionary.
Likely "the modules dictionary" is a global sys.modules. So reference counter should be 2, not 1:
- one in sys.modules
- one after
PyDict_SetItemString
Py_CLEAR(submodule); above should be dropped too.
Contributor
Author
There was a problem hiding this comment.
Oh, thank you. I see the logic behind PyImport_AddModule and then PyDict_SetItemString now.
Should I fix the patch to 3.4 first, and then update this PR?
Member
There was a problem hiding this comment.
These patches are independent (as we do manual port to 4.x for this patch). Update this and create separate PR to 3.4.
- Add special case handling when submodule has the same name as parent - `PyDict_SetItemString` doesn't steal reference, so reference count should be explicitly decremented to transfer object life-time ownership - Add sanity checks for module registration input - Add Python 2 and Python 3 reference counting handling
3364d53 to
d887306
Compare
6 tasks
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PyDict_SetItemStringdoesn't steal reference, so reference count should be explicitly decremented to transfer object life-time ownershipBackport: #21478
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.