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

Updated submodules to the release 4.5.4 and added python loader support #563

Merged
merged 3 commits into from Oct 20, 2021

Conversation

asenyaev
Copy link
Contributor

No description provided.

@asenyaev asenyaev self-assigned this Oct 15, 2021
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@@ -0,0 +1,23 @@
from .cv2 import *
from .data import *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we don't need these 2 lines

"data" submodule is imported automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace import cv2 to import cv2.cv2 for native_module in __init__.py, then we need first line, second can be removed.

Besides, I removed lines with a replacing, because rewrote config file, so, removed it.

try:
from .version import ci_build, headless

ci_and_not_headless = ci_build and not headless
Copy link
Member

@alalek alalek Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How I can verify that case that locally?

Update: Tried CI_BUILD=1, but it fails on missing files. So it is not easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to build with CI_BUILD=1 and also define ENABLE_HEADLESS=0, ENABLE_CONTRIB=0 and SDIST=0.

After the build you can reproduce these lines in the python interpreter. What I've done:

from cv2.version import ci_build, headless
ci_and_not_headless = ci_build and not headless
print (ci_and_not_headless)

In addition, to understand ci_and_not_headless working or not you can print QT_QPA_FONTDIR environment variable. Example:

import cv2, os
print(os.environ["QT_QPA_FONTDIR"])

setup.py Outdated
% cmake_install_dir, 'r') as opencv_init:
opencv_init_data = ""
for line in opencv_init:
opencv_init_replacement = line.replace('importlib.import_module("cv2")', 'importlib.import_module("cv2.cv2")')
Copy link
Member

@alalek alalek Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config-X.Y.py defines where we find native binary extension:

PYTHON_EXTENSIONS_PATHS = [
    os.path.join(LOADER_DIR, 'python-3.9')
] + PYTHON_EXTENSIONS_PATHS

Currently it is placed to

site-packages/cv2/cv2.cpython-39-x86_64-linux-gnu.so

which is not consistent (lines have no effect)

It would work automatically with import_module("cv2")

  1. if we move binary file to python-X.Y subdirectory (as it is located originally by CMake install)

    site-packages/cv2/python-3.9/cv2.cpython-39-x86_64-linux-gnu.so
    
  2. Or we replace these config lines to

    PYTHON_EXTENSIONS_PATHS = [
        LOADER_DIR
    ] + PYTHON_EXTENSIONS_PATHS
    

Note: We need to check/update RPATH to point properly to "Lib" directory with dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the logic when cv2.cpython-39-x86_64-linux-gnu.so locates under site-packages/cv2/python-3.9/cv2.cpython-39-x86_64-linux-gnu.so, and then libs cannot work properly, because there are the wrong path to libs in config.py.

Added replacing lines into __init__.py file, to define the proper path.

Comment on lines +125 to +136
"cv2.gapi": [
"python/cv2" + r"/gapi/.*\.py"
],
"cv2.mat_wrapper": [
"python/cv2" + r"/mat_wrapper/.*\.py"
],
"cv2.misc": [
"python/cv2" + r"/misc/.*\.py"
],
"cv2.utils": [
"python/cv2" + r"/utils/.*\.py"
],
Copy link
Member

@alalek alalek Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we copy all artifacts of "cmake install" automatically "as is"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is packages = ["cv2", "cv2.data"] line in this file. Perhaps it should be updated somehow (can we remove cv2.data?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we copy all artifacts of "cmake install" as is, then there will be several folders and files which we do not want to have in a package. We control what should be in a package using rearrange_cmake_output_data in setup.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about cv2.data we have to keep it if we want to use haarcascades, because there are no __init__.py file in "cmake install" and we defining it here. However, we can write into a file after building. What do you think about anything of it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, we could specify haarcascades as package data, not a separate package. But it requires checking the code and import and so on, I'd delay it to the future.

Comment on lines +125 to +136
"cv2.gapi": [
"python/cv2" + r"/gapi/.*\.py"
],
"cv2.mat_wrapper": [
"python/cv2" + r"/mat_wrapper/.*\.py"
],
"cv2.misc": [
"python/cv2" + r"/misc/.*\.py"
],
"cv2.utils": [
"python/cv2" + r"/utils/.*\.py"
],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, we could specify haarcascades as package data, not a separate package. But it requires checking the code and import and so on, I'd delay it to the future.

@sergregory
Copy link
Collaborator

@asenyaev please also merge #565 before the release

@asenyaev
Copy link
Contributor Author

@asenyaev please also merge #565 before the release

Sure, I'll do it.

@alalek
Copy link
Member

alalek commented Oct 21, 2021

opencv

https://github.com/opencv/opencv/commits/39c3334147ec02761b117f180c9c4518be18d1fa

Submodule should point to 4.5.4 tag, not on the "-dev" merge commit.

cv.__version__ => 4.5.4-dev

@asenyaev
Copy link
Contributor Author

asenyaev commented Oct 21, 2021

opencv

https://github.com/opencv/opencv/commits/39c3334147ec02761b117f180c9c4518be18d1fa

Submodule should point to 4.5.4 tag, not on the "-dev" merge commit.

cv.__version__ => 4.5.4-dev

As far as I remember, we always attached the latest commit for submodules. But in this case, when we have a wrong opencv version, we have to define a tag.

cclauss added a commit to cclauss/opencv-python that referenced this pull request Nov 25, 2021
Related to changes made in opencv#563

$ `flake8 . --count --builtins=ml_ops --select=E9,F63,F7,F82,Y --show-source --statistics`
```
./opencv-python/scripts/__init__.py:2:5: F821 undefined name 'LOADER_DIR'
    LOADER_DIR
    ^
./opencv-python/scripts/__init__.py:3:5: F821 undefined name 'PYTHON_EXTENSIONS_PATHS'
] + PYTHON_EXTENSIONS_PATHS
    ^
./opencv-python/scripts/__init__.py:15:4: F821 undefined name 'sys'
if sys.platform.startswith("linux") and ci_and_not_headless:
   ^
./opencv-python/scripts/__init__.py:16:5: F821 undefined name 'os'
    os.environ["QT_QPA_PLATFORM_PLUGIN_PATH"] = os.path.join(
    ^
./opencv-python/scripts/__init__.py:16:49: F821 undefined name 'os'
    os.environ["QT_QPA_PLATFORM_PLUGIN_PATH"] = os.path.join(
                                                ^
./opencv-python/scripts/__init__.py:17:9: F821 undefined name 'os'
        os.path.dirname(os.path.abspath(__file__)), "qt", "plugins"
        ^
./opencv-python/scripts/__init__.py:17:25: F821 undefined name 'os'
        os.path.dirname(os.path.abspath(__file__)), "qt", "plugins"
                        ^
./opencv-python/scripts/__init__.py:21:4: F821 undefined name 'sys'
if sys.platform.startswith("linux") and ci_and_not_headless:
   ^
./opencv-python/scripts/__init__.py:22:5: F821 undefined name 'os'
    os.environ["QT_QPA_FONTDIR"] = os.path.join(
    ^
./opencv-python/scripts/__init__.py:22:36: F821 undefined name 'os'
    os.environ["QT_QPA_FONTDIR"] = os.path.join(
                                   ^
./opencv-python/scripts/__init__.py:23:9: F821 undefined name 'os'
        os.path.dirname(os.path.abspath(__file__)), "qt", "fonts"
        ^
./opencv-python/scripts/__init__.py:23:25: F821 undefined name 'os'
        os.path.dirname(os.path.abspath(__file__)), "qt", "fonts"
                        ^
12    F821 undefined name 'LOADER_DIR'
12
```
Comment on lines +1 to +3
PYTHON_EXTENSIONS_PATHS = [
LOADER_DIR
] + PYTHON_EXTENSIONS_PATHS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are undefined names in Python code.
$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

./scripts/__init__.py:2:5: F821 undefined name 'LOADER_DIR'
    LOADER_DIR
    ^
./scripts/__init__.py:3:5: F821 undefined name 'PYTHON_EXTENSIONS_PATHS'
] + PYTHON_EXTENSIONS_PATHS
    ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants