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.

Sign up for GitHub

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

Jump to bottom

Raise exception instead of warning when cache isn't writable #150

Closed
leouieda opened this issue Mar 12, 2020 · 1 comment · Fixed by #171
Closed

Raise exception instead of warning when cache isn't writable #150

leouieda opened this issue Mar 12, 2020 · 1 comment · Fixed by #171
Labels
enhancement Idea or request for a new feature

Comments

@leouieda
Copy link
Member

Description of the desired feature

As pointed out by @danshapero in #149, when the cache folder isn't writable we warn the user through an error message but don't re-raise the PermissionError that is generated. This might be OK for interactive environments like Jupyter but it would cause an error for a script and the actual warning would be lost in the traceback.

A more sensible thing to do would be to add the message we create to the exception and re-raise it instead of logging.

The code for this is in pooch/core.py in the make_local_storage function (after #149 is merged).
@leouieda leouieda added enhancement Idea or request for a new feature good first issue Good for newcomers (doesn’t require deep knowledge of the project) labels Mar 12, 2020
@leouieda leouieda mentioned this issue Mar 12, 2020
Merged
6 tasks
@leouieda leouieda removed the good first issue Good for newcomers (doesn’t require deep knowledge of the project) label May 9, 2020
@leouieda
Copy link
Member Author

leouieda commented May 9, 2020

Actually, this is probably not a good idea. Since the cache directory is created at import time, this would cause packages to crash immediately even if the user doesn't care about sample data (see scikit-image/scikit-image#4664).

@leouieda leouieda mentioned this issue May 9, 2020
Merged
6 tasks
leouieda added a commit that referenced this issue May 9, 2020
@leouieda
Bug fix: creating the cache folder in parallel (#171)
dd56cd0 
Add `exist_ok` to `os.makedirs` in case the cache folder is being created 
in parallel (multiple jobs trying to call `makedirs` at the same time). 
Includes a test case with threads and processes to make sure it works.

Fixes #170 and fixes #150
This was referenced May 11, 2020
Closed
Merged
leouieda added a commit that referenced this issue May 14, 2020
@leouieda
Delay cache creation until first download (#173)
7726a7a 
As seen in recent issues in scikit-image, creating the cache folder in
`pooch.create` can cause problems since this function is called at import time
by our users. This means that importing the package in parallel can cause race
conditions and crashes. To prevent that from happening, delay the creation of
the cache folder until `Pooch.fetch` or `retrieve` are called. Creating the
folder name was factored out of `make_local_storage` into the new function
`cache_location`. Both `fetch` and `retrieve` then use `make_local_storage` to
create the folder while `create` only runs `cache_location`. Since we can't
check if the cache is writable when calling `create`, then it makes more sense
to raise an exception with the message about using the environment variable
instead of a log message (as #150 wanted).

Fixes #172
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Idea or request for a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Bug fix: creating the cache folder in parallel fatiando/pooch
1 participant
@leouieda