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
Collaborator

@asenyaev asenyaev commented Oct 15, 2021

No description provided.

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

@alalek alalek left a comment

Thank you!

Loading

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

@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.

Perhaps we don't need these 2 lines

"data" submodule is imported automatically.

Loading

Copy link
Collaborator Author

@asenyaev asenyaev Oct 18, 2021

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.

Loading

try:
from .version import ci_build, headless

ci_and_not_headless = ci_build and not headless
Copy link

@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.

Loading

Copy link
Collaborator Author

@asenyaev asenyaev Oct 18, 2021

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"])

Loading

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

@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.

Loading

Copy link
Collaborator Author

@asenyaev asenyaev Oct 18, 2021

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.

Loading

"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

@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"?

Loading

Copy link

@alalek alalek Oct 16, 2021

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?)

Loading

Copy link
Collaborator Author

@asenyaev asenyaev Oct 18, 2021

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.

Loading

Copy link
Collaborator Author

@asenyaev asenyaev Oct 18, 2021

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?

Loading

Copy link
Collaborator

@sergregory sergregory Oct 20, 2021

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.

Loading

alalek
alalek approved these changes Oct 18, 2021
"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

@sergregory sergregory Oct 20, 2021

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.

Loading

@sergregory
Copy link
Collaborator

@sergregory sergregory commented Oct 20, 2021

@asenyaev please also merge #565 before the release

Loading

@asenyaev
Copy link
Collaborator Author

@asenyaev asenyaev commented Oct 20, 2021

@asenyaev please also merge #565 before the release

Sure, I'll do it.

Loading

@asenyaev asenyaev merged commit a0f47e9 into opencv:master Oct 20, 2021
130 checks passed
Loading
@alalek
Copy link

@alalek 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

Loading

@asenyaev
Copy link
Collaborator Author

@asenyaev 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.

Loading

cclauss added a commit to cclauss/opencv-python that referenced this issue 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
```
PYTHON_EXTENSIONS_PATHS = [
LOADER_DIR
] + PYTHON_EXTENSIONS_PATHS
Copy link
Contributor

@cclauss cclauss Nov 26, 2021

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
    ^

Loading

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

Successfully merging this pull request may close these issues.

None yet

5 participants