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

Extra imports and an environment for opencv-python package #21209

Closed
wants to merge 4 commits into from

Conversation

asenyaev
Copy link
Contributor

@asenyaev asenyaev commented Dec 6, 2021

This PR contains an extra environment from the script in opencv-python repository and extra imports which are defined after the opencv-python package build.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@alalek
Copy link
Member

alalek commented Dec 6, 2021

IMO, we don't really need such downstream details in upstream repository.

@asenyaev
Copy link
Contributor Author

asenyaev commented Dec 6, 2021

@alalek, do you mean we don't need to have it in opencv repository and we should add it in opencv-python repository?

@alalek
Copy link
Member

alalek commented Dec 6, 2021

Yes, this specific code should be kept in opencv-python repository.

Comment on lines 206 to 215

# hotfix for pylint issue (https://github.com/opencv/opencv-python/issues/570)
from . import getStructuringElement, MORPH_ELLIPSE

# extra imports for a proper autocomplete work in IDE
from . import _registerMatType
from . import gapi
from . import mat_wrapper
from . import misc
from . import utils
Copy link
Contributor Author

@asenyaev asenyaev Dec 6, 2021

Choose a reason for hiding this comment

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

Okay. I would only suggest to keep it here, which fixes pylint issue (it even happens if to build opencv from the source) and allows to use modules (e.g. gapi, misc) using an autocomplete in IDE.

Copy link
Member

@alalek alalek Dec 6, 2021

Choose a reason for hiding this comment

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

Unfortunately pylint can't work with modern cv2 without manual own "brain" (like numpy) or plugin module (like django).
It doesn't see anything except import_module("cv2.cv2") line in opencv_python package. Neither sys.path changes, nor module symbols update.

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.

Hopefully #20370 can help to resolve some of these issues.

@@ -179,3 +179,13 @@ def load_first_config(fnames, required=True):


bootstrap()

# hotfix for pylint issue (https://github.com/opencv/opencv-python/issues/570)
from . import getStructuringElement, MORPH_ELLIPSE
Copy link
Member

Choose a reason for hiding this comment

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

We definitely should not have that.
Issue provides a minimal reproducer. Do you want to add here other 100500 symbols?

/cc @asmorkalov

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've seen a problem only with these two. We didn't have this issue when we had used an import of anything in a binary, e.g.

from .cv2 import *

I guess there was the import of the same modules if the issue has been disappeared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line.

from . import getStructuringElement, MORPH_ELLIPSE

# extra imports for a proper autocomplete work in IDE
from . import _registerMatType
Copy link
Member

Choose a reason for hiding this comment

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

Users should not use private internal symbols directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, it shouldn't be here. I've just faced an error, when mat_wrapper module couldn't be imported w/o it. It's been when we have imported *.so file as cv2 module in opencv-python package.

Removed it.

from . import gapi
from . import mat_wrapper
from . import misc
from . import utils
Copy link
Member

Choose a reason for hiding this comment

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

OpenCV has flexible module architecture (with custom set of modules), so such attempt of using hard-coded names just breaks the current design.
You can do that in opencv-python which uses fixed pre-defined build configuration, but it is not suitable for this upstream repository.

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 thought the idea was to keep one __init__.py file in opencv repository, and not to overwrite it in opencv-python repository. But okay, I'm closing this PR then.

@asenyaev asenyaev closed this Dec 7, 2021
@asenyaev asenyaev deleted the asen/extra_import_opencv_python branch December 7, 2021 12:29
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

2 participants