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

Fail gracefully if cache directory is read only #430

Merged
merged 5 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions brainglobe_atlasapi/bg_atlas.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,14 @@ def check_latest_version(
True if it is, and None if we are offline.
"""

if self.remote_version is None: # in this case, we are offline
# Cache remote version to avoid multiple requests
remote_version = self.remote_version
# If we are offline, return None
if remote_version is None:
return

local = _version_str_from_tuple(self.local_version)
online = _version_str_from_tuple(self.remote_version)
online = _version_str_from_tuple(remote_version)

if local != online:
if print_warning:
Expand Down
19 changes: 11 additions & 8 deletions brainglobe_atlasapi/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@


def conf_from_url(url) -> configparser.ConfigParser:
"""Read conf file from an URL. And cache a copy in the brainglobe dir.
"""Read conf file from a URL. And cache a copy in the brainglobe dir.
Parameters
----------
url : str
Expand All @@ -305,14 +305,17 @@
text = requests.get(url).text
config_obj = configparser.ConfigParser()
config_obj.read_string(text)
cache_path = config.get_brainglobe_dir() / "last_versions.conf"
cache_path: Path = config.get_brainglobe_dir() / "last_versions.conf"

if not cache_path.parent.exists():
cache_path.parent.mkdir(parents=True, exist_ok=True)

# Cache the available atlases
with open(cache_path, "w") as f_out:
config_obj.write(f_out)
try:
if not cache_path.parent.exists():
cache_path.parent.mkdir(parents=True, exist_ok=True)

Check warning on line 312 in brainglobe_atlasapi/utils.py

View check run for this annotation

Codecov / codecov/patch

brainglobe_atlasapi/utils.py#L312

Added line #L312 was not covered by tests

# Cache the available atlases
with open(cache_path, "w") as f_out:
config_obj.write(f_out)
except OSError as e:
print(f"Could not update the latest atlas versions cache: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better if this used logging.warning or logging.info?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure, the rest of the file uses print statements to inform user of error cases so I didn't want to break from what's currently being used. Does brainglobe-atlasapi keep a log on disk somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like earlier in the utils.py file it sets the logging level for urllib3 but if print is used elsewhere, maybe let's stick to print here and open an issue to make logging consistent... (atlasgen uses loguru)


return config_obj

Expand Down
28 changes: 28 additions & 0 deletions tests/atlasapi/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import os
import sys
from unittest import mock

import pytest
Expand All @@ -7,6 +9,9 @@
from brainglobe_atlasapi import utils

test_url = "https://gin.g-node.org/BrainGlobe/atlases/raw/master/example_mouse_100um_v1.2.tar.gz"
conf_url = (
"https://gin.g-node.org/BrainGlobe/atlases/raw/master/last_versions.conf"
)


def test_http_check():
Expand Down Expand Up @@ -138,3 +143,26 @@ def test_conf_from_file_no_file(temp_path):
utils.conf_from_file(conf_path)

assert "Last versions cache file not found." == str(e)


@pytest.mark.skipif(sys.platform == "win32", reason="Does not run on Windows")
def test_conf_from_url_read_only(temp_path, mocker):
# Test with a valid URL and a non-existing parent folder
mocker.patch(
"brainglobe_atlasapi.utils.config.get_brainglobe_dir"
).return_value = temp_path
mock_print = mocker.patch("builtins.print")
# Save the current permissions
curr_mode = oct(os.stat(temp_path).st_mode)[-3:]

# Change the permissions to read-only
temp_path.chmod(0o444)
utils.conf_from_url(conf_url)

mock_print.assert_called_once_with(
f"Could not update the latest atlas versions cache: [Errno 13] "
f"Permission denied: '{temp_path / 'last_versions.conf'}'"
)

# Set the permissions back to the original
temp_path.chmod(int(curr_mode, 8))
Loading