Add Python Bindings for getCacheDirectory function by sl-sergei · Pull Request #18931 · opencv/opencv

sl-sergei · 2020-11-26T12:33:38Z

This PR adds new Python binding for getCacheDirectory, necessary for the PR: #18591

force_builders=linux,docs,Custom
build_image:Custom=ubuntu-openvino-2020.4.0:16.04
build_image:Custom Win=openvino-2020.4.0
build_image:Custom Mac=openvino-2020.4.0

test_modules:Custom=python2,python3,java,samples
test_modules:Custom Win=python2,python3,java,samples
test_modules:Custom Mac=python2,python3,java,samples

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

sl-sergei · 2020-11-26T12:34:54Z

@alalek can you advise on better naming/location for cache_directory.hpp?

alalek · 2020-11-26T13:14:50Z

Why do we need to move declaration?

If it is really necessary, the lets put single CV_EXPORTS_W wrapper (without namespace hierarchy) into bindings_utils.hpp

sl-sergei · 2020-11-26T13:39:05Z

Why do we need to move declaration?

If it is really necessary, the lets put single CV_EXPORTS_W wrapper (without namespace hierarchy) into bindings_utils.hpp

I moved declaration because core/utils/filesystem.hpp is filtered out in python/bindings/CMakeLists.txt. If I move it without names, then it will be necessary to move definition into bindings_utils.cpp or change namespace logic and macros in filesystem.cpp

alalek · 2020-11-26T13:41:39Z

bindings_utils.cpp

Lets put wrapper implementation there.

alalek · 2020-11-26T16:15:25Z

modules/core/include/opencv2/core/utils/filesystem.hpp

- * @param configuration_name optional name of configuration parameter name which overrides default behavior.
- * @return Path to cache directory. Returns empty string if cache directories support is not available. Returns "disabled" if cache disabled by user.
- */
-CV_EXPORTS cv::String getCacheDirectory(const char* sub_directory_name, const char* configuration_name = NULL);


Please do not touch this at all.

Just add wrapper call for bindings only.

alalek

Looks much better.

alalek · 2020-11-26T16:46:40Z

modules/python/test/test_fs_cache_dir.py

+from tests_common import NewOpenCVTests
+
+class get_cache_dir_test(NewOpenCVTests):
+    def test_copytomask(self):


copytomask

?

please reuse test_misc.py

alalek · 2020-11-26T16:47:03Z

modules/python/test/test_fs_cache_dir.py

+        #New binding
+        path  = cv.utils.getCacheDirectory(subfolder_name)
+        self.assertTrue(os.path.exists(path))
+        self.assertTrue(os.path.isdir(path))


It make sense to dump path as failure message

sl-sergei · 2020-11-26T17:56:06Z

@alalek I updated the test, are there other drawbacks that I need to fix?

dkurt · 2020-11-27T18:32:10Z

related: #15317. So it seems to me there is simpler way to enable the function. Also I think that we need to preserve namespace for transferability: cv.utils.fs.getCacheDirectory.

alalek · 2020-11-27T18:42:17Z

BTW, There is an empty string result on Windows due to some reason. Perhaps we should improve this issue first.

sl-sergei · 2020-12-01T22:06:12Z

related: #15317. So it seems to me there is simpler way to enable the function. Also I think that we need to preserve namespace for transferability: cv.utils.fs.getCacheDirectory.

If we follow your way for enabling this function in Python, it will create bindings for other functions from "modules/.+/utils/.*" which I think should not be included in the Python package.

sl-sergei · 2020-12-01T22:07:17Z

BTW, There is an empty string result on Windows due to some reason. Perhaps we should improve this issue first.

@alalek I tried to reproduce this issue on local Windows machine, but I could not do that

dkurt · 2020-12-02T05:51:11Z

@sl-sergei, bindings are regulated by CV_EXPORTS_W macro. Other methods have just CV_EXPORTS.

sl-sergei · 2020-12-02T10:19:51Z

@dkurt This method creates bindings for enums in utils.logging, utils.trace and instr namespaces without any functions. I think they should not be included in Python package, so it is better to leave the filtering in CMake. @alalek Please, correct me if I'm wrong

dkurt · 2020-12-02T10:52:16Z

We can add only filesystem.hpp in whitelist but not entire "core/utils"

sl-sergei · 2020-12-02T11:24:22Z

@dkurt How do you plan to do that? I don't know, maybe there is an easy way to do that, but I have already tried to change filtering logic/regex, but found that CMake has very limited options for regular expressions. The other option is to add filesystem.hpp directly to the CMake, but this would look really ugly...

alalek

More serious problem here is non-ASCII paths (especially on Windows) and cv::String result.

On Windows it is expected to have path like C:\Documents and Settings\username\Local Settings\AppData\Local\opencv\<major_version>.x<version_status>\.
username is a source of Unicode symbols.

P.S. Even TEMP / TMP env paths are point into User's profile.

Need to design more robust solution.

Ref: #18305

alalek · 2020-12-03T01:08:01Z

modules/core/include/opencv2/core/bindings_utils.hpp

 }

+namespace fs {
+    CV_EXPORTS_W cv::String getCacheDirectory(const char *sub_directory_name);


Aliases should work:

CV_EXPORTS_AS(getCacheDirectory) cv::String getCacheDirectory_bindings(const char *sub_directory_name, ...);

Also repair configuration parameter. It is necessary to keep OpenCV configurable.

IMHO, It make sense to hide some details and provide more specialized wrapper.

In core/utility.hpp add:

CV_EXPORTS_W cv::String getCacheDirectoryForDownloads(void);

with implementation (to avoid details in .py files):

{ return cv::utils::fs::getCacheDirectory("downloads", "OPENCV_DOWNLOADS_CACHE_DIR"); }

In core/utility.hpp add:

I don't think it is a good idea to put function declaration and implementation there, because it would require including core/filesystem.hpp there. So I left it inside binding_utils, which is in my opinion a more suitable place for a binding wrapper.

asmorkalov · 2020-12-09T08:12:10Z

@sl-sergei please squash commits

asmorkalov · 2020-12-10T12:33:33Z

@alalek Please take a look. Sergey added UTF8 handling too.

asmorkalov · 2021-03-03T09:04:46Z

Replaced by #19668

sl-sergei requested a review from alalek November 26, 2020 12:33

alalek reviewed Nov 26, 2020

View reviewed changes

alalek reviewed Dec 3, 2020

View reviewed changes

alalek mentioned this pull request Dec 3, 2020

Scripts for downloading models in DNN samples #18591

Merged

sl-sergei mentioned this pull request Dec 4, 2020

Possible non-ASCII paths returned from utils::fs::getCacheDirectory #19011

Open

4 tasks

Add Python Bindings for getCacheDirectory function

cbdd5ae

sl-sergei force-pushed the filesystem_py branch from 1c50c38 to cbdd5ae Compare December 10, 2020 12:02

Add fix for possible problems with non-ASCII symbols on Windows

f88d39b

asmorkalov mentioned this pull request Mar 3, 2021

getCacheDirectory binding for Python #19668

Merged

6 tasks

asmorkalov closed this Mar 3, 2021

Uh oh!

Conversation

sl-sergei commented Nov 26, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl-sergei commented Nov 26, 2020

Uh oh!

alalek commented Nov 26, 2020

Uh oh!

sl-sergei commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Nov 26, 2020

Uh oh!

alalek Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

alalek Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

alalek Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

sl-sergei commented Nov 26, 2020

Uh oh!

dkurt commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Nov 27, 2020

Uh oh!

sl-sergei commented Dec 1, 2020

Uh oh!

sl-sergei commented Dec 1, 2020

Uh oh!

dkurt commented Dec 2, 2020

Uh oh!

sl-sergei commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkurt commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl-sergei commented Dec 2, 2020

Uh oh!

alalek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

sl-sergei Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Dec 9, 2020

Uh oh!

asmorkalov commented Dec 10, 2020

Uh oh!

asmorkalov commented Mar 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sl-sergei commented Nov 26, 2020 •

edited by alalek

Loading

sl-sergei commented Nov 26, 2020 •

edited

Loading

dkurt commented Nov 27, 2020 •

edited

Loading

sl-sergei commented Dec 2, 2020 •

edited

Loading

dkurt commented Dec 2, 2020 •

edited

Loading

alalek left a comment •

edited

Loading