From d6148c96ddfa0e0da416d7d4d793ea581c83e072 Mon Sep 17 00:00:00 2001 From: RRosio Date: Fri, 31 Jan 2025 23:45:45 -0800 Subject: [PATCH 01/17] add test checks --- jupyter_fsspec/tests/test_api.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jupyter_fsspec/tests/test_api.py b/jupyter_fsspec/tests/test_api.py index a218438..2f59598 100644 --- a/jupyter_fsspec/tests/test_api.py +++ b/jupyter_fsspec/tests/test_api.py @@ -11,6 +11,11 @@ async def test_get_config(jp_fetch): json_body = response.body.decode("utf-8") body = json.loads(json_body) assert body["status"] == "success" + assert ( + body["description"] + == "Retrieved available filesystems from configuration file." + ) + assert body["content"] != [] async def test_get_files_memory(fs_manager_instance, jp_fetch): From 2664f69da29af9edb61d3aa93f66403a4e026eb6 Mon Sep 17 00:00:00 2001 From: RRosio Date: Sun, 2 Feb 2025 22:16:49 -0800 Subject: [PATCH 02/17] add robust error handling for config --- jupyter_fsspec/file_manager.py | 276 +++++++++++++++++++++------------ jupyter_fsspec/handlers.py | 21 ++- pyproject.toml | 1 + 3 files changed, 189 insertions(+), 109 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 9822def..e3a7518 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -1,13 +1,14 @@ from jupyter_core.paths import jupyter_config_dir -import fsspec +from pydantic import BaseModel +from typing import Optional, Dict, List from fsspec.utils import infer_storage_options from fsspec.registry import known_implementations +import fsspec import os import sys import yaml import hashlib import urllib.parse -import traceback import logging logging.basicConfig(level=logging.WARNING, stream=sys.stdout) @@ -15,15 +16,50 @@ logger.setLevel(logging.INFO) +class Source(BaseModel): + name: str + path: str + protocol: Optional[str] = None + args: Optional[List] = None + kwargs: Optional[Dict] = None + + +class Config(BaseModel): + sources: List[Source] + + +def handle_exception(default_return=None): + def decorator(func): + def closure(*args, **kwargs): + try: + return func(*args, **kwargs) + except Exception as e: + logger.error(f"Error : {func.__name__}: {e}.") + return default_return + + return closure + + return decorator + + +@handle_exception( + default_return={"operation_success": False, "error": "Error validating config"} +) +def validate_config(config_loaded): + Config.parse_obj(config_loaded) + return {"operation_success": True, "validated_config": config_loaded} + + class FileSystemManager: def __init__(self, config_file): self.filesystems = {} self.base_dir = jupyter_config_dir() - logger.debug(f"Using Jupyter config directory: {self.base_dir}") - os.makedirs(self.base_dir, exist_ok=True) + logger.info(f"Using Jupyter config directory: {self.base_dir}") self.config_path = os.path.join(self.base_dir, config_file) - self.config = self.load_config() + config = self.load_config() + self.config = config.get("config_content", {}) + self.async_implementations = self._asynchronous_implementations() self.initialize_filesystems() @@ -46,45 +82,54 @@ def _decode_key(self, encoded_key): def create_default(): return FileSystemManager(config_file="jupyter-fsspec.yaml") - # os.path.exists(config_path) + @handle_exception( + default_return={"operation_success": False, "error": "Error reading config."} + ) + def retrieve_config_content(self, config_path): + with open(config_path, "r") as file: + config_content = yaml.safe_load(file) + + if not config_content: + return {"operation_success": True, "config_content": {}} + + validation_result = validate_config(config_content) + if not validation_result.get("operation_success"): + raise ValueError(validation_result["error"]) + + config_validated = validation_result.get("validated_config", {}) + return {"operation_success": True, "config_content": config_validated} + + @handle_exception( + default_return={"operation_success": False, "error": "Error loading config."} + ) def load_config(self): config_path = self.config_path - result = {"sources": []} - - try: - if not os.path.exists(config_path): - logger.debug( - f"Config file not found at {config_path}. Creating default file." - ) - self.create_config_file() - - with open(config_path, "r") as file: - config_loaded = yaml.safe_load(file) - if config_loaded is not None and "sources" in config_loaded: - result = config_loaded - except FileNotFoundError: - logger.error( - f"Config file was not found and could not be created at {config_path}." + config_content = {"sources": []} + + if not os.path.exists(config_path): + logger.debug( + f"Config file not found at {config_path}. Creating default file." ) - except yaml.YAMLError as e: - logger.error(f"Error parsing configuration file: {e}") - # TODO: Check for permissions / handle case for OSError - # except OSError as oserr: - except Exception: - traceback.print_exc() - logger.error("Error occured when loading the config file.") - return result + file_creation_result = self.create_config_file() + + if not file_creation_result.get("operation_success"): + print(f"inner file_creation_result: {file_creation_result}") + return file_creation_result + + config_content = self.retrieve_config_content(config_path) + + return config_content def hash_config(self, config_content): yaml_str = yaml.dump(config_content) hash = hashlib.md5(yaml_str.encode("utf-8")).hexdigest() return hash - def create_config_file(self): - config_path = self.config_path - os.makedirs(os.path.dirname(config_path), exist_ok=True) - + @handle_exception( + default_return={"operation_success": False, "error": "Error writing config."} + ) + def write_default_config(self, path): placeholder_config = { "sources": [ {"name": "test", "path": "memory://mytests"}, @@ -94,19 +139,40 @@ def create_config_file(self): config_documentation = """# This file is in you JUPYTER_CONFIG_DIR.\n# Multiple filesystem sources can be configured\n# with a unique `name` field, the `path` which\n# can include the protocol, or can omit it and\n# provide it as a seperate field `protocol`. \n# You can also provide `args` such as `key` \n# and `kwargs` such as `client_kwargs` to\n# the filesystem. More details can be found at https://jupyter-fsspec.readthedocs.io/en/latest/#config-file.""" - try: - yaml_content = yaml.dump(placeholder_config, default_flow_style=False) - commented_yaml = "\n".join( - f"# {line}" for line in yaml_content.splitlines() - ) + yaml_content = yaml.dump(placeholder_config, default_flow_style=False) + commented_yaml = "\n".join(f"# {line}" for line in yaml_content.splitlines()) + + full_content = config_documentation + "\n\n" + commented_yaml + with open(path, "w") as config_file: + config_file.write(full_content) + + logger.info(f"Configuration file created at {path}") + return {"operation_success": True, "message": "Wrote default config."} + + @handle_exception( + default_return={ + "operation_success": False, + "error": "Error creating config file.", + } + ) + def create_config_file(self): + config_path = self.config_path + config_dir = os.path.dirname(config_path) - full_content = config_documentation + "\n\n" + commented_yaml - with open(config_path, "w") as config_file: - config_file.write(full_content) + logger.debug(f"Ensuring config directory exists: {config_dir}.") + os.makedirs(config_dir, exist_ok=True) - logger.debug(f"Configuration file created at {config_path}") - except Exception as e: - logger.error("Error creating configuration file: ", e) + if not os.access(config_dir, os.W_OK): + raise PermissionError(f"Config directory was not writable: {config_dir}") + + write_result = self.write_default_config(config_path) + if not write_result.get("operation_success"): + raise IOError(f"{write_result.get('error', 'Unkown error')}.") + + return { + "operation_success": True, + "message": f"Config file created at {config_path}", + } def _get_protocol_from_path(self, path): storage_options = infer_storage_options(path) @@ -126,73 +192,76 @@ def _asynchronous_implementations(self): return async_filesystems def _async_available(self, protocol): - if protocol in self.async_implementations: - return True - else: - return False + return protocol in self.async_implementations + @handle_exception( + default_return={ + "operation_success": False, + "error": "Error initializing filesystems.", + } + ) def initialize_filesystems(self): new_filesystems = {} + config = self.config + + if config == {}: + self.filesystems = new_filesystems + return # Init filesystem - try: - for fs_config in self.config["sources"]: - fs_name = fs_config.get("name", None) - fs_path = fs_config.get("path", None) - fs_protocol = fs_config.get("protocol", None) - args = fs_config.get("args", []) - kwargs = fs_config.get("kwargs", {}) - - if not fs_name: - logger.error("Skipping configuration: Missing 'name'") - continue - if fs_protocol is None: - if fs_path: - fs_protocol = self._get_protocol_from_path(fs_path) - else: - logger.error( - f"Skipping '{fs_name}': Missing 'protocol' and 'path' to infer it from" - ) - continue - - # TODO: support for case no path - if not fs_path: + for fs_config in config.get("sources", []): + fs_name = fs_config.get("name", None) + fs_path = fs_config.get("path", None) + fs_protocol = fs_config.get("protocol", None) + args = fs_config.get("args", []) + kwargs = fs_config.get("kwargs", {}) + + if fs_protocol is None: + if fs_path: + fs_protocol = self._get_protocol_from_path(fs_path) + else: logger.error( - f"Filesystem '{fs_name}' with protocol 'fs_protocol' requires 'path'" + f"Skipping '{fs_name}': Missing 'protocol' and 'path' to infer it from" ) + continue - key = self._encode_key(fs_config) - - # Add: _is_protocol_supported? Or rely on fsspec? - fs_async = self._async_available(fs_protocol) - fs = fsspec.filesystem( - fs_protocol, asynchronous=fs_async, *args, **kwargs - ) - - if fs_protocol == "memory": - if not fs.exists(fs_path): - fs.mkdir(fs_path) - - # Store the filesystem instance - new_filesystems[key] = { - "instance": fs, - "name": fs_name, - "protocol": fs_protocol, - "path": fs._strip_protocol(fs_path), - "canonical_path": fs.unstrip_protocol(fs_path), - } - logger.debug( - f"Initialized filesystem '{fs_name}' with protocol '{fs_protocol}'" - ) - - except Exception as e: - traceback.print_exc() - print(f"Error initializing filesystems: {e}") + # TODO: support for case no path + # if not fs_path: + # logger.error( + # f"Filesystem '{fs_name}' with protocol 'fs_protocol' requires 'path'" + # ) + + key = self._encode_key(fs_config) + + # Add: _is_protocol_supported? Or rely on fsspec? + fs_async = self._async_available(fs_protocol) + fs = fsspec.filesystem(fs_protocol, asynchronous=fs_async, *args, **kwargs) + + if fs_protocol == "memory" and not fs.exists(fs_path): + fs.mkdir(fs_path, exists_ok=True) + + # Store the filesystem instance + new_filesystems[key] = { + "instance": fs, + "name": fs_name, + "protocol": fs_protocol, + "path": fs._strip_protocol(fs_path), + "canonical_path": fs.unstrip_protocol(fs_path), + } + logger.debug( + f"Initialized filesystem '{fs_name}' with protocol '{fs_protocol}' at path '{fs_path}'" + ) self.filesystems = new_filesystems def check_reload_config(self): - new_content = self.load_config() + load_config = self.load_config() + + if not load_config.get("operation_success"): + print(f"load_config was not succes but is: {load_config}") + return load_config + + new_content = load_config.get("config_content", {}) hash_new_content = self.hash_config(new_content) current_config_hash = self.hash_config(self.config) @@ -200,12 +269,12 @@ def check_reload_config(self): self.config = new_content self.initialize_filesystems() - def get_all_filesystems(self): - self._initialize_filesystems() + return new_content def get_filesystem(self, key): return self.filesystems.get(key) + # TODO: Update to pull full dict with all filesystems def get_filesystem_by_protocol(self, fs_protocol): for encoded_key, fs_info in self.filesystems.items(): if fs_info.get("protocol") == fs_protocol: @@ -214,5 +283,6 @@ def get_filesystem_by_protocol(self, fs_protocol): def get_filesystem_protocol(self, key): filesystem_rep = self.filesystems.get(key) - print(f"filesystem_rep: {filesystem_rep}") + if not filesystem_rep: + return None return filesystem_rep["protocol"] + "://" diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 729d5d6..25b20c3 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -4,7 +4,6 @@ from .utils import parse_range import tornado import json -import traceback import logging logging.basicConfig(level=logging.INFO) @@ -61,7 +60,11 @@ def get(self): :rtype: dict """ try: - self.fs_manager.check_reload_config() + check = self.fs_manager.check_reload_config() + + if not check.get("operation_success") and not check.get("sources"): + raise Exception + file_systems = [] for fs in self.fs_manager.filesystems: @@ -83,15 +86,21 @@ def get(self): } ) self.finish() - except Exception as e: - traceback.print_exc() + except Exception: + err_mgs = "" + + if check.get("operation_success", True): + err_mgs = "FileNotFound" + else: + err_mgs = check["error"] + self.set_status(404) self.write( { "response": { "status": "failed", - "error": "FILE_NOT_FOUND", - "description": f"Error loading config: {str(e)}", + "error": err_mgs, + "description": "Error retrieving config.", } } ) diff --git a/pyproject.toml b/pyproject.toml index 9714e92..3f0ca7f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -27,6 +27,7 @@ classifiers = [ ] dependencies = [ "fsspec", + "pydantic>=2", "jupyter_server>=2.4.0,<3" ] dynamic = ["version", "description", "urls", "keywords"] From 10fa713e612edf81ea03254c5d3f7347efe2c043 Mon Sep 17 00:00:00 2001 From: RRosio Date: Mon, 3 Feb 2025 00:13:59 -0800 Subject: [PATCH 03/17] added more config tests, updated handler check --- .pytest.ini | 3 ++ conftest.py | 82 +++++++++++++++++++++++++++++++- jupyter_fsspec/file_manager.py | 2 +- jupyter_fsspec/handlers.py | 12 +++-- jupyter_fsspec/tests/test_api.py | 34 ++++++++++++- 5 files changed, 126 insertions(+), 7 deletions(-) create mode 100644 .pytest.ini diff --git a/.pytest.ini b/.pytest.ini new file mode 100644 index 0000000..5559dae --- /dev/null +++ b/.pytest.ini @@ -0,0 +1,3 @@ +[pytest] +markers = + no_setup_config_file_fs: mark test to not use setup_config_file_fs fixture diff --git a/conftest.py b/conftest.py index d13ecee..81b3c29 100644 --- a/conftest.py +++ b/conftest.py @@ -35,7 +35,87 @@ def setup_tmp_local(tmp_path: Path): yield [local_root, local_empty_root] -@pytest.fixture(scope="function", autouse=True) +@pytest.fixture(scope="function") +def no_config_permission(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + os.chmod(config_dir, 0o44) + + with patch( + "jupyter_fsspec.file_manager.jupyter_config_dir", return_value=str(config_dir) + ): + print(f"Patching jupyter_config_dir to: {config_dir}") + yield config_dir + + os.chmod(config_dir, 0o755) + config_dir.rmdir() + + +@pytest.fixture(scope="function") +def malformed_config(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + + yaml_content = f"""sources: + - name: "TestSourceAWS" + path: "s3://my-test-bucket/" + kwargs: + anon: false + key: "my-access-key" + secret: "my-secret-key" + client_kwargs: + endpoint_url: "{ENDPOINT_URI}" + """ + yaml_file = config_dir / "jupyter-fsspec.yaml" + yaml_file.write_text(yaml_content) + + with patch( + "jupyter_fsspec.file_manager.jupyter_config_dir", return_value=str(config_dir) + ): + print(f"Patching jupyter_config_dir to: {config_dir}") + + +@pytest.fixture(scope="function") +def bad_info_config(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + + yaml_content = f"""sources: + - nme: "TestSourceAWS" + path: s3://my-test-bucket/" + kwargs: + anon: false + key: "my-access-key" + secret: "my-secret-key" + client_kwargs: + endpoint_url: "{ENDPOINT_URI}" + """ + yaml_file = config_dir / "jupyter-fsspec.yaml" + yaml_file.write_text(yaml_content) + + with patch( + "jupyter_fsspec.file_manager.jupyter_config_dir", return_value=str(config_dir) + ): + print(f"Patching jupyter_config_dir to: {config_dir}") + + +@pytest.fixture(scope="function") +def empty_config(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + + yaml_content = """ """ + yaml_file = config_dir / "jupyter-fsspec.yaml" + yaml_file.write_text(yaml_content) + + with patch( + "jupyter_fsspec.file_manager.jupyter_config_dir", return_value=str(config_dir) + ): + print(f"Patching jupyter_config_dir to: {config_dir}") + + +# TODO: split? +@pytest.fixture(scope="function") def setup_config_file_fs(tmp_path: Path, setup_tmp_local): tmp_local = setup_tmp_local[0] items_tmp_local = list(tmp_local.iterdir()) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index e3a7518..d45ecc2 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -46,7 +46,7 @@ def closure(*args, **kwargs): default_return={"operation_success": False, "error": "Error validating config"} ) def validate_config(config_loaded): - Config.parse_obj(config_loaded) + Config.model_validate(config_loaded) return {"operation_success": True, "validated_config": config_loaded} diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 25b20c3..57567dc 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -60,9 +60,13 @@ def get(self): :rtype: dict """ try: - check = self.fs_manager.check_reload_config() + config = self.fs_manager.check_reload_config() - if not check.get("operation_success") and not check.get("sources"): + if ( + not config.get("operation_success") + and "sources" not in config + and config + ): raise Exception file_systems = [] @@ -89,10 +93,10 @@ def get(self): except Exception: err_mgs = "" - if check.get("operation_success", True): + if config.get("operation_success", True): err_mgs = "FileNotFound" else: - err_mgs = check["error"] + err_mgs = config["error"] self.set_status(404) self.write( diff --git a/jupyter_fsspec/tests/test_api.py b/jupyter_fsspec/tests/test_api.py index 2f59598..9e3ee12 100644 --- a/jupyter_fsspec/tests/test_api.py +++ b/jupyter_fsspec/tests/test_api.py @@ -4,7 +4,7 @@ # TODO: Testing: different file types, received expected errors -async def test_get_config(jp_fetch): +async def test_get_config(setup_config_file_fs, jp_fetch): response = await jp_fetch("jupyter_fsspec", "config", method="GET") assert response.code == 200 @@ -18,6 +18,38 @@ async def test_get_config(jp_fetch): assert body["content"] != [] +@pytest.mark.no_setup_config_file_fs +async def test_no_config(no_config_permission, jp_fetch): + with pytest.raises(HTTPClientError) as exc_info: + await jp_fetch("jupyter_fsspec", "config", method="GET") + assert exc_info.value.code == 404 + + +@pytest.mark.no_setup_config_file_fs +async def test_malformed_config(malformed_config, jp_fetch): + with pytest.raises(HTTPClientError) as exc_info: + await jp_fetch("jupyter_fsspec", "config", method="GET") + assert exc_info.value.code == 404 + + +@pytest.mark.no_setup_config_file_fs +async def test_bad_config_info(bad_info_config, jp_fetch): + with pytest.raises(HTTPClientError) as exc_info: + await jp_fetch("jupyter_fsspec", "config", method="GET") + assert exc_info.value.code == 404 + + +@pytest.mark.no_setup_config_file_fs +async def test_empty_config(empty_config, jp_fetch): + fetch_config = await jp_fetch("jupyter_fsspec", "config", method="GET") + assert fetch_config.code == 200 + + json_body = fetch_config.body.decode("utf-8") + body = json.loads(json_body) + assert body["status"] == "success" + assert body["content"] == [] + + async def test_get_files_memory(fs_manager_instance, jp_fetch): fs_manager = fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") From 83de5e1d1b8f9209a69e247d77ce75c3235f4780 Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 4 Feb 2025 22:21:32 -0800 Subject: [PATCH 04/17] make initial changes from review, remove code unnecessary and update server status codes --- jupyter_fsspec/file_manager.py | 9 ++------- jupyter_fsspec/handlers.py | 2 +- jupyter_fsspec/tests/test_api.py | 10 +++++----- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index d45ecc2..3548c36 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -202,14 +202,12 @@ def _async_available(self, protocol): ) def initialize_filesystems(self): new_filesystems = {} - config = self.config - if config == {}: - self.filesystems = new_filesystems + if self.config == {}: return # Init filesystem - for fs_config in config.get("sources", []): + for fs_config in self.config.get("sources", []): fs_name = fs_config.get("name", None) fs_path = fs_config.get("path", None) fs_protocol = fs_config.get("protocol", None) @@ -237,9 +235,6 @@ def initialize_filesystems(self): fs_async = self._async_available(fs_protocol) fs = fsspec.filesystem(fs_protocol, asynchronous=fs_async, *args, **kwargs) - if fs_protocol == "memory" and not fs.exists(fs_path): - fs.mkdir(fs_path, exists_ok=True) - # Store the filesystem instance new_filesystems[key] = { "instance": fs, diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 57567dc..ab77c32 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -98,7 +98,7 @@ def get(self): else: err_mgs = config["error"] - self.set_status(404) + self.set_status(500) self.write( { "response": { diff --git a/jupyter_fsspec/tests/test_api.py b/jupyter_fsspec/tests/test_api.py index 9e3ee12..1fe0c1c 100644 --- a/jupyter_fsspec/tests/test_api.py +++ b/jupyter_fsspec/tests/test_api.py @@ -15,28 +15,28 @@ async def test_get_config(setup_config_file_fs, jp_fetch): body["description"] == "Retrieved available filesystems from configuration file." ) - assert body["content"] != [] + assert len(body["content"]) == 4 @pytest.mark.no_setup_config_file_fs async def test_no_config(no_config_permission, jp_fetch): with pytest.raises(HTTPClientError) as exc_info: await jp_fetch("jupyter_fsspec", "config", method="GET") - assert exc_info.value.code == 404 + assert exc_info.value.code == 500 @pytest.mark.no_setup_config_file_fs async def test_malformed_config(malformed_config, jp_fetch): with pytest.raises(HTTPClientError) as exc_info: await jp_fetch("jupyter_fsspec", "config", method="GET") - assert exc_info.value.code == 404 + assert exc_info.value.code == 500 @pytest.mark.no_setup_config_file_fs async def test_bad_config_info(bad_info_config, jp_fetch): with pytest.raises(HTTPClientError) as exc_info: await jp_fetch("jupyter_fsspec", "config", method="GET") - assert exc_info.value.code == 404 + assert exc_info.value.code == 500 @pytest.mark.no_setup_config_file_fs @@ -47,7 +47,7 @@ async def test_empty_config(empty_config, jp_fetch): json_body = fetch_config.body.decode("utf-8") body = json.loads(json_body) assert body["status"] == "success" - assert body["content"] == [] + assert len(body["content"]) == 0 async def test_get_files_memory(fs_manager_instance, jp_fetch): From 6d4908a4692696c5a67a58c94e53f72a0e7af949 Mon Sep 17 00:00:00 2001 From: RRosio Date: Thu, 6 Feb 2025 13:06:05 -0800 Subject: [PATCH 05/17] updated handling exceptions --- jupyter_fsspec/exceptions.py | 4 + jupyter_fsspec/file_manager.py | 141 ++++++++++--------------------- jupyter_fsspec/handlers.py | 148 ++++++++++++++++++++++----------- 3 files changed, 149 insertions(+), 144 deletions(-) diff --git a/jupyter_fsspec/exceptions.py b/jupyter_fsspec/exceptions.py index 64ed5d0..8813f1d 100644 --- a/jupyter_fsspec/exceptions.py +++ b/jupyter_fsspec/exceptions.py @@ -3,3 +3,7 @@ class JupyterFsspecException(Exception): pass + + +class ConfigFileException(JupyterFsspecException): + pass diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 3548c36..0d72d1c 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -13,7 +13,7 @@ logging.basicConfig(level=logging.WARNING, stream=sys.stdout) logger = logging.getLogger(__name__) -logger.setLevel(logging.INFO) +logger.setLevel(logging.DEBUG) class Source(BaseModel): @@ -28,39 +28,14 @@ class Config(BaseModel): sources: List[Source] -def handle_exception(default_return=None): - def decorator(func): - def closure(*args, **kwargs): - try: - return func(*args, **kwargs) - except Exception as e: - logger.error(f"Error : {func.__name__}: {e}.") - return default_return - - return closure - - return decorator - - -@handle_exception( - default_return={"operation_success": False, "error": "Error validating config"} -) -def validate_config(config_loaded): - Config.model_validate(config_loaded) - return {"operation_success": True, "validated_config": config_loaded} - - class FileSystemManager: def __init__(self, config_file): self.filesystems = {} self.base_dir = jupyter_config_dir() logger.info(f"Using Jupyter config directory: {self.base_dir}") self.config_path = os.path.join(self.base_dir, config_file) - - config = self.load_config() - self.config = config.get("config_content", {}) - self.async_implementations = self._asynchronous_implementations() + self.config = self.load_config(handle_errors=True) self.initialize_filesystems() def _encode_key(self, fs_config): @@ -82,54 +57,48 @@ def _decode_key(self, encoded_key): def create_default(): return FileSystemManager(config_file="jupyter-fsspec.yaml") - @handle_exception( - default_return={"operation_success": False, "error": "Error reading config."} - ) - def retrieve_config_content(self, config_path): + @staticmethod + def validate_config(config_loaded): + Config.model_validate(config_loaded) + + def retrieve_config_content(self): + config_path = self.config_path + with open(config_path, "r") as file: config_content = yaml.safe_load(file) if not config_content: - return {"operation_success": True, "config_content": {}} + return {} - validation_result = validate_config(config_content) - if not validation_result.get("operation_success"): - raise ValueError(validation_result["error"]) + self.validate_config(config_content) - config_validated = validation_result.get("validated_config", {}) - return {"operation_success": True, "config_content": config_validated} + return config_content - @handle_exception( - default_return={"operation_success": False, "error": "Error loading config."} - ) - def load_config(self): + def load_config(self, handle_errors=False): config_path = self.config_path - config_content = {"sources": []} - - if not os.path.exists(config_path): - logger.debug( - f"Config file not found at {config_path}. Creating default file." - ) - - file_creation_result = self.create_config_file() - - if not file_creation_result.get("operation_success"): - print(f"inner file_creation_result: {file_creation_result}") - return file_creation_result - config_content = self.retrieve_config_content(config_path) + try: + if not os.path.exists(config_path): + logger.debug( + f"Config file not found at {config_path}. Creating default file." + ) + self.create_config_file() + config_content = self.retrieve_config_content() + return config_content + except Exception as e: + if handle_errors: + logger.error(f"Error loading configuration file: {e}") + return {} + raise - return config_content - - def hash_config(self, config_content): + @staticmethod + def hash_config(config_content): yaml_str = yaml.dump(config_content) hash = hashlib.md5(yaml_str.encode("utf-8")).hexdigest() return hash - @handle_exception( - default_return={"operation_success": False, "error": "Error writing config."} - ) - def write_default_config(self, path): + def write_default_config(self): + config_path = self.config_path placeholder_config = { "sources": [ {"name": "test", "path": "memory://mytests"}, @@ -143,18 +112,12 @@ def write_default_config(self, path): commented_yaml = "\n".join(f"# {line}" for line in yaml_content.splitlines()) full_content = config_documentation + "\n\n" + commented_yaml - with open(path, "w") as config_file: + with open(config_path, "w") as config_file: config_file.write(full_content) - logger.info(f"Configuration file created at {path}") - return {"operation_success": True, "message": "Wrote default config."} + logger.info(f"Configuration file created at {config_path}") + return - @handle_exception( - default_return={ - "operation_success": False, - "error": "Error creating config file.", - } - ) def create_config_file(self): config_path = self.config_path config_dir = os.path.dirname(config_path) @@ -165,21 +128,18 @@ def create_config_file(self): if not os.access(config_dir, os.W_OK): raise PermissionError(f"Config directory was not writable: {config_dir}") - write_result = self.write_default_config(config_path) - if not write_result.get("operation_success"): - raise IOError(f"{write_result.get('error', 'Unkown error')}.") + self.write_default_config() - return { - "operation_success": True, - "message": f"Config file created at {config_path}", - } + return - def _get_protocol_from_path(self, path): + @staticmethod + def _get_protocol_from_path(path): storage_options = infer_storage_options(path) protocol = storage_options.get("protocol", "file") return protocol - def _asynchronous_implementations(self): + @staticmethod + def _asynchronous_implementations(): async_filesystems = [] for protocol, impl in known_implementations.items(): @@ -194,16 +154,11 @@ def _asynchronous_implementations(self): def _async_available(self, protocol): return protocol in self.async_implementations - @handle_exception( - default_return={ - "operation_success": False, - "error": "Error initializing filesystems.", - } - ) def initialize_filesystems(self): new_filesystems = {} - if self.config == {}: + if not self.config: + self.config = {} return # Init filesystem @@ -250,21 +205,15 @@ def initialize_filesystems(self): self.filesystems = new_filesystems def check_reload_config(self): - load_config = self.load_config() - - if not load_config.get("operation_success"): - print(f"load_config was not succes but is: {load_config}") - return load_config - - new_content = load_config.get("config_content", {}) - hash_new_content = self.hash_config(new_content) + new_config_content = self.load_config() + hash_new_content = self.hash_config(new_config_content) current_config_hash = self.hash_config(self.config) if current_config_hash != hash_new_content: - self.config = new_content + self.config = new_config_content self.initialize_filesystems() - return new_content + return new_config_content def get_filesystem(self, key): return self.filesystems.get(key) diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index ab77c32..8e88cb3 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -2,6 +2,9 @@ from jupyter_server.base.handlers import APIHandler from jupyter_server.utils import url_path_join from .utils import parse_range +from .exceptions import ConfigFileException +from contextlib import contextmanager +import yaml import tornado import json import logging @@ -42,6 +45,75 @@ def validate_fs(self, request_type, key, item_path): return fs, item_path +@contextmanager +def handle_exception( + handler, default_response={"error": "Error handling request."}, status_code=500 +): + try: + yield + except yaml.YAMLError as e: + logger.error(f"YAMLError: {default_response['error']}") + logger.error(f"YAMLError: {str(e)}") + + if handler._finished: + return + + try: + handler.set_status(status_code) + handler.write({"status": "failed", "description": str(e), "content": []}) + except Exception as response_e: + logger.error(f"Error writing handler response: {response_e}") + + handler.finish() + raise ConfigFileException + except PermissionError as e: + logger.error(f"PermissionError: {default_response['error']}") + logger.error(f"PermissionError: {str(e)}") + + if handler._finished: + return + + try: + handler.set_status(status_code) + handler.write({"status": "failed", "description": str(e), "content": []}) + except Exception as response_e: + logger.error(f"Error writing handler response: {response_e}") + + handler.finish() + raise ConfigFileException + except FileNotFoundError as e: + logger.error(f"FileNotFoundError: {default_response['error']}") + logger.error(f"FileNotFoundError: {str(e)}") + + if handler._finished: + return + + try: + handler.set_status(status_code) + handler.write({"status": "failed", "description": str(e), "content": []}) + except Exception as response_e: + logger.error(f"Error writing handler response: {response_e}") + + handler.finish() + raise ConfigFileException + except Exception as e: + # TODO: remove default_response? + logger.error(f"UnkownError: {default_response['error']}") + logger.error(f"UnkownException: {str(e)}") + + if handler._finished: + return + + try: + handler.set_status(status_code) + handler.write({"status": "failed", "description": str(e), "content": []}) + except Exception as response_e: + logger.error(f"Error writing handler response: {response_e}") + + handler.finish() + raise ConfigFileException + + class FsspecConfigHandler(APIHandler): """ @@ -59,56 +131,36 @@ def get(self): :return: dict with filesystems key and list of filesystem information objects :rtype: dict """ - try: - config = self.fs_manager.check_reload_config() + file_systems = [] - if ( - not config.get("operation_success") - and "sources" not in config - and config + try: + with handle_exception( + self, default_response={"error": "Error retrieving config file."} ): - raise Exception - - file_systems = [] - - for fs in self.fs_manager.filesystems: - fs_info = self.fs_manager.filesystems[fs] - instance = { - "key": fs, - "name": fs_info["name"], - "protocol": fs_info["protocol"], - "path": fs_info["path"], - "canonical_path": fs_info["canonical_path"], - } - file_systems.append(instance) - self.set_status(200) - self.write( - { - "status": "success", - "description": "Retrieved available filesystems from configuration file.", - "content": file_systems, - } - ) - self.finish() - except Exception: - err_mgs = "" - - if config.get("operation_success", True): - err_mgs = "FileNotFound" - else: - err_mgs = config["error"] - - self.set_status(500) - self.write( - { - "response": { - "status": "failed", - "error": err_mgs, - "description": "Error retrieving config.", - } - } - ) - self.finish() + self.fs_manager.check_reload_config() + except ConfigFileException: + return + + for fs in self.fs_manager.filesystems: + fs_info = self.fs_manager.filesystems[fs] + instance = { + "key": fs, + "name": fs_info["name"], + "protocol": fs_info["protocol"], + "path": fs_info["path"], + "canonical_path": fs_info["canonical_path"], + } + file_systems.append(instance) + + self.set_status(200) + self.write( + { + "status": "success", + "description": "Retrieved available filesystems from configuration file.", + "content": file_systems, + } + ) + self.finish() # ==================================================================================== From 74b6de064ac0c769a32ffe2c459170172dd71dab Mon Sep 17 00:00:00 2001 From: RRosio Date: Thu, 6 Feb 2025 15:47:46 -0800 Subject: [PATCH 06/17] clean up exception handling --- jupyter_fsspec/handlers.py | 86 ++++++++++++++------------------------ 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 8e88cb3..03db81d 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -4,6 +4,7 @@ from .utils import parse_range from .exceptions import ConfigFileException from contextlib import contextmanager +from pydantic import ValidationError import yaml import tornado import json @@ -46,69 +47,48 @@ def validate_fs(self, request_type, key, item_path): @contextmanager -def handle_exception( - handler, default_response={"error": "Error handling request."}, status_code=500 -): +def handle_exception(handler, status_code=500): try: yield except yaml.YAMLError as e: - logger.error(f"YAMLError: {default_response['error']}") - logger.error(f"YAMLError: {str(e)}") + error_message = { + "error": "YAMLError", + "details": getattr(e, "problem", str(e)), + "line": getattr(e, "problem_mark", None).line + 1 + if getattr(e, "problem_mark", None) + else None, + } - if handler._finished: - return - - try: - handler.set_status(status_code) - handler.write({"status": "failed", "description": str(e), "content": []}) - except Exception as response_e: - logger.error(f"Error writing handler response: {response_e}") - - handler.finish() - raise ConfigFileException - except PermissionError as e: - logger.error(f"PermissionError: {default_response['error']}") - logger.error(f"PermissionError: {str(e)}") + logger.error(error_message) - if handler._finished: - return - - try: - handler.set_status(status_code) - handler.write({"status": "failed", "description": str(e), "content": []}) - except Exception as response_e: - logger.error(f"Error writing handler response: {response_e}") + handler.set_status(status_code) + handler.write({"status": "failed", "description": error_message, "content": []}) handler.finish() raise ConfigFileException - except FileNotFoundError as e: - logger.error(f"FileNotFoundError: {default_response['error']}") - logger.error(f"FileNotFoundError: {str(e)}") - - if handler._finished: - return - - try: - handler.set_status(status_code) - handler.write({"status": "failed", "description": str(e), "content": []}) - except Exception as response_e: - logger.error(f"Error writing handler response: {response_e}") + except ValidationError as e: + error_message = [] + for err in e.errors(): + error = f"{err['msg']}: {err['type']} {err['loc']}" + error_message.append(error) + logger.error(error_message) + + handler.set_status(status_code) + handler.write( + { + "status": "failed", + "description": {error: "ValidationError", "details": error_message}, + "content": [], + } + ) - handler.finish() raise ConfigFileException except Exception as e: - # TODO: remove default_response? - logger.error(f"UnkownError: {default_response['error']}") - logger.error(f"UnkownException: {str(e)}") + error_message = f"{type(e).__name__}: {str(e)}" + logger.error(error_message) - if handler._finished: - return - - try: - handler.set_status(status_code) - handler.write({"status": "failed", "description": str(e), "content": []}) - except Exception as response_e: - logger.error(f"Error writing handler response: {response_e}") + handler.set_status(status_code) + handler.write({"status": "failed", "description": error_message, "content": []}) handler.finish() raise ConfigFileException @@ -134,9 +114,7 @@ def get(self): file_systems = [] try: - with handle_exception( - self, default_response={"error": "Error retrieving config file."} - ): + with handle_exception(self): self.fs_manager.check_reload_config() except ConfigFileException: return From b3e0116d5f15863e794af5a47ed96565e6fd597a Mon Sep 17 00:00:00 2001 From: RRosio Date: Thu, 6 Feb 2025 21:53:09 -0800 Subject: [PATCH 07/17] updated and added filesystem manager tests --- .../tests/unit/test_filesystem_manager.py | 266 +++++++++++------- 1 file changed, 168 insertions(+), 98 deletions(-) diff --git a/jupyter_fsspec/tests/unit/test_filesystem_manager.py b/jupyter_fsspec/tests/unit/test_filesystem_manager.py index 0a40fe9..a9e9d6c 100644 --- a/jupyter_fsspec/tests/unit/test_filesystem_manager.py +++ b/jupyter_fsspec/tests/unit/test_filesystem_manager.py @@ -1,13 +1,11 @@ import pytest import yaml +from pydantic import ValidationError +from pathlib import Path from jupyter_fsspec.file_manager import FileSystemManager +from unittest.mock import patch -# Test FileSystemManager class and file operations - -# ============================================================ -# Test FileSystemManager config loading/read -# ============================================================ @pytest.fixture def config_file(tmp_path): config = {"sources": [{"name": "inmem", "path": "/mem_dir", "protocol": "memory"}]} @@ -17,37 +15,87 @@ def config_file(tmp_path): return config_path -# @pytest.fixture -# def mock_filesystem(config_file): -# fs_test_manager = FileSystemManager(config_file) -# file_systems = fs_test_manager.filesystems -# return file_systems +@pytest.fixture +def empty_config_file(tmp_path): + config = {} + config_path = tmp_path / "config.yaml" + with open(config_path, "w") as file: + yaml.dump(config, file) + return config_path + + +@pytest.fixture(scope="function") +def setup_config_dir(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + + yaml_file = config_dir / "jupyter-fsspec.yaml" + yaml_file.write_text("") + with patch( + "jupyter_fsspec.file_manager.jupyter_config_dir", return_value=str(config_dir) + ): + print(f"Patching jupyter_config_dir to: {config_dir}") + yield config_dir -# test that the file systems are created -def test_filesystem_init(config_file): - fs_test_manager = FileSystemManager(config_file) - assert fs_test_manager is not None - # assert 'memory' in fs_test_manager.filesystems +@pytest.fixture +def mock_config(): + mock_config = { + "sources": [ + {"protocol": "memory", "name": "test_memory_fs", "path": "/test_memory"} + ] + } + return mock_config + + +@pytest.fixture +def invalid_mock_config(): + mock_config = { + "sources": [ + {"protocol": "memory", "nme": "test_memory_fs", "pah": "/test_memory"} + ] + } + return mock_config + + +@pytest.fixture(scope="function") +def bad_yaml_config(tmp_path: Path): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + + yaml_content = """sources: + - nme: "TestSourceAWS" + path: s3://my-test-bucket/" + kwargs: + anon: false + key: "my-access-key" + secret: "my-secret-key" + """ + yaml_file = config_dir / "jupyter-fsspec.yaml" + yaml_file.write_text(yaml_content) + return yaml_file + + +# =============================== +# Test FileSystemManager +# =============================== +def test_filesystem_init(setup_config_dir, config_file): + fs_manager = FileSystemManager(config_file) + + assert fs_manager is not None + assert "inmem" in fs_manager.filesystems # Validate in-memory filesystem - in_memory_fs = fs_test_manager.get_filesystem( - fs_test_manager._encode_key( - {"name": "inmem", "path": "/mem_dir", "protocol": "memory"} - ) - ) + in_memory_fs = fs_manager.get_filesystem(fs_manager._encode_key({"name": "inmem"})) assert in_memory_fs is not None - # TODO: - # assert any('memory' in key for key in fs_test_manager.filesystems) - # assert in_memory_fs['protocol'] == 'memory' - # assert in_memory_fs['path'] == '/mem_dir' + assert in_memory_fs["protocol"] == "memory" + assert in_memory_fs["path"] == "/mem_dir" -# test key encoding/decoding -def test_key_decode_encode(config_file): - fs_test_manager = FileSystemManager(config_file) +def test_key_decode_encode(setup_config_dir, config_file): + fs_manager = FileSystemManager(config_file) fs_test_config = { "name": "mylocal", @@ -55,102 +103,124 @@ def test_key_decode_encode(config_file): "path": str(config_file.parent), } - # TODO: update _encoded_key - encoded_key = fs_test_manager._encode_key(fs_test_config) - decoded_name = fs_test_manager._decode_key(encoded_key) + encoded_key = fs_manager._encode_key(fs_test_config) + decoded_name = fs_manager._decode_key(encoded_key) assert decoded_name == fs_test_config["name"] -# ============================================================ -# Test FileSystemManager file operations -# ============================================================ -@pytest.fixture -def mock_config(): - mock_config = { - "sources": [ - {"protocol": "memory", "name": "test_memory_fs", "path": "/test_memory"} - ] - } - return mock_config +def test_load_config_empty(setup_config_dir, empty_config_file): + with patch.object(FileSystemManager, "__init__", lambda self: None): + fs_manager = FileSystemManager() + fs_manager.filesystems = {} + fs_manager.base_dir = setup_config_dir + fs_manager.config_path = empty_config_file + fs_manager.async_implementations = fs_manager._asynchronous_implementations() -@pytest.fixture -def fs_manager_instance(mock_config): - fs_test_manager = FileSystemManager.__new__(FileSystemManager) - fs_test_manager.config = mock_config - fs_test_manager.filesystems = {} - fs_test_manager._initialize_filesystems() - return fs_test_manager + loaded_config = fs_manager.load_config(handle_errors=True) + assert loaded_config == {} + fs_manager.config = loaded_config + fs_manager.initialize_filesystems() + assert fs_manager.filesystems == {} -@pytest.fixture -def populated_fs_manager(mock_config, fs_manager_instance): - key = fs_manager_instance._encode_key(mock_config["sources"][0]) - dir_path = "test_memory" - dest_path = "test_dir" - file_path = f"{dir_path}/{dest_path}/test_file_pop.txt" - file_content = b"This is a test for a populated filesystem!" +def test_load_populated_config(setup_config_dir, config_file): + with patch.object(FileSystemManager, "__init__", lambda self: None): + fs_manager = FileSystemManager() - fs_manager_instance.write(key, dir_path, dest_path) + fs_manager.filesystems = {} + fs_manager.base_dir = setup_config_dir + fs_manager.config_path = config_file + fs_manager.async_implementations = fs_manager._asynchronous_implementations() - fs_manager_instance.write(key, file_path, file_content) + loaded_config = fs_manager.load_config(handle_errors=True) + assert loaded_config == { + "sources": [{"name": "inmem", "path": "/mem_dir", "protocol": "memory"}] + } + fs_manager.config = loaded_config - second_file_path = f"{dir_path}/second_test_file_pop.txt" - second_file_content = b"Second test for a populated filesystem!" - fs_manager_instance.write(key, second_file_path, second_file_content) - return fs_manager_instance, key + fs_manager.initialize_filesystems() + assert len(fs_manager.filesystems) == 1 + mem_instance_info = fs_manager.filesystems["inmem"] + assert mem_instance_info["name"] == "inmem" + assert mem_instance_info["protocol"] == "memory" + assert mem_instance_info["path"] == "/mem_dir" + assert mem_instance_info["canonical_path"] == "memory:///mem_dir" + mem_fs_instance = mem_instance_info["instance"] + assert mem_fs_instance.ls("/") == [ + {"name": "/my_mem_dir", "size": 0, "type": "directory"} + ] -# TODO: update config path for tests -def xtest_file_read_write(mock_config, fs_manager_instance): - key = fs_manager_instance._encode_key(mock_config["sources"][0]) - # write - item_path = "/test_memory/my_file.txt" - content = b"Hello, this is a test!" +def test_check_reload_config(setup_config_dir, config_file): + with patch.object(FileSystemManager, "__init__", lambda self: None): + fs_manager = FileSystemManager() + fs_manager.config_path = config_file + fs_manager.config = {} + fs_manager.async_implementations = fs_manager._asynchronous_implementations() + + fs_manager.check_reload_config() + + assert len(fs_manager.filesystems) == 1 + mem_instance_info = fs_manager.filesystems["inmem"] + assert mem_instance_info["name"] == "inmem" + assert mem_instance_info["protocol"] == "memory" + assert mem_instance_info["path"] == "/mem_dir" + assert mem_instance_info["canonical_path"] == "memory:///mem_dir" + + mem_fs_instance = mem_instance_info["instance"] + assert mem_fs_instance.ls("/") == [ + {"name": "/my_mem_dir", "size": 0, "type": "directory"} + ] + - write_result = fs_manager_instance.write(key, item_path, content) - assert write_result["status_code"] == 200 +def test_error_validate_config(invalid_mock_config): + with pytest.raises(ValidationError) as exc: + FileSystemManager.validate_config(invalid_mock_config) + exc_msg = str(exc.value) + assert "2 validation errors for Config" in exc_msg + assert "sources.0.name" in exc_msg + assert "sources.0.path" in exc_msg - # read - read_result = fs_manager_instance.read(key, item_path) - assert read_result["status_code"] == 200 - assert read_result["response"]["content"] == content.decode("utf-8") +def test_error_initialize_filesystems(caplog): + with patch.object(FileSystemManager, "__init__", lambda self: None): + fs_manager = FileSystemManager() + fs_manager.config = {"sources": [{"name": "inmem"}]} + fs_manager.async_implementations = fs_manager._asynchronous_implementations() + fs_manager.initialize_filesystems() -def xtest_file_update_delete(populated_fs_manager): - # key = fs_manager_instance._encode_key(mock_config["sources"][0]) - pass + expected_msg = "Skipping 'inmem': Missing 'protocol' and 'path' to infer it from" + assert expected_msg in caplog.text -def xtest_directory_read_write(mock_config, fs_manager_instance): - key = fs_manager_instance._encode_key(mock_config["sources"][0]) +def test_empty_initialize_filesystems(caplog): + with patch.object(FileSystemManager, "__init__", lambda self: None): + fs_manager = FileSystemManager() + fs_manager.config = {} + fs_manager.async_implementations = fs_manager._asynchronous_implementations() + fs_manager.initialize_filesystems() - # write - item_path = "test_memory" - dest_path = "my_test_subdir" + assert "Initialized filesystem" not in caplog.text - write_result = fs_manager_instance.write(key, item_path, dest_path) - assert write_result["status_code"] == 200 - # read - read_result = fs_manager_instance.read(key, item_path) - content_list = read_result["response"]["content"] - assert read_result["status_code"] == 200 +def test_error_create_config_file(setup_config_dir, config_file): + with patch("os.access", return_value=False): + with pytest.raises(PermissionError) as exc: + fs_manager = FileSystemManager(config_file) + fs_manager.create_config_file() - dir_name_to_check = "/" + item_path + "/my_test_subdir" - subdir_exists = any( - item["name"] == dir_name_to_check and item["type"] == "directory" - for item in content_list - ) - assert subdir_exists + expected_exc_msg = f"Config directory was not writable: {setup_config_dir.parent}" + assert expected_exc_msg == str(exc.value) -def xtest_directory_update_delete(populated_fs_manager): - # key = fs_manager_instance._encode_key(mock_config["sources"][0]) - pass - # update +def test_error_retrieve_config_content(bad_yaml_config): + with pytest.raises(yaml.YAMLError) as exc: + fs_manager = FileSystemManager(bad_yaml_config) + fs_manager.retrieve_config_content() - # delete + partial_exc_msg = "expected , but found '?'" + assert partial_exc_msg in str(exc.value) From a1002aa6da3afcd45f41f445408ee43f91ea3753 Mon Sep 17 00:00:00 2001 From: RRosio Date: Thu, 6 Feb 2025 22:03:01 -0800 Subject: [PATCH 08/17] back to info log level --- jupyter_fsspec/file_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 0d72d1c..7bbb3db 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -13,7 +13,7 @@ logging.basicConfig(level=logging.WARNING, stream=sys.stdout) logger = logging.getLogger(__name__) -logger.setLevel(logging.DEBUG) +logger.setLevel(logging.INFO) class Source(BaseModel): From 029fad6a47ffa2e61e789a3bb211fb6cb31d574a Mon Sep 17 00:00:00 2001 From: Rosio Date: Fri, 14 Feb 2025 00:43:10 -0800 Subject: [PATCH 09/17] Apply suggestions from code review Co-authored-by: Martin Durant --- jupyter_fsspec/file_manager.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 7bbb3db..09e0a5f 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -116,7 +116,6 @@ def write_default_config(self): config_file.write(full_content) logger.info(f"Configuration file created at {config_path}") - return def create_config_file(self): config_path = self.config_path @@ -130,7 +129,6 @@ def create_config_file(self): self.write_default_config() - return @staticmethod def _get_protocol_from_path(path): From cab7a0ffdb04bb70bd9d1ee1c77d5f3cfdd15458 Mon Sep 17 00:00:00 2001 From: RRosio Date: Fri, 14 Feb 2025 01:12:46 -0800 Subject: [PATCH 10/17] create and write config file into one function --- jupyter_fsspec/file_manager.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 09e0a5f..56f6490 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -97,7 +97,16 @@ def hash_config(config_content): hash = hashlib.md5(yaml_str.encode("utf-8")).hexdigest() return hash - def write_default_config(self): + def create_config_file(self): + config_path = self.config_path + config_dir = os.path.dirname(config_path) + + logger.debug(f"Ensuring config directory exists: {config_dir}.") + os.makedirs(config_dir, exist_ok=True) + + if not os.access(config_dir, os.W_OK): + raise PermissionError(f"Config directory was not writable: {config_dir}") + config_path = self.config_path placeholder_config = { "sources": [ @@ -117,19 +126,6 @@ def write_default_config(self): logger.info(f"Configuration file created at {config_path}") - def create_config_file(self): - config_path = self.config_path - config_dir = os.path.dirname(config_path) - - logger.debug(f"Ensuring config directory exists: {config_dir}.") - os.makedirs(config_dir, exist_ok=True) - - if not os.access(config_dir, os.W_OK): - raise PermissionError(f"Config directory was not writable: {config_dir}") - - self.write_default_config() - - @staticmethod def _get_protocol_from_path(path): storage_options = infer_storage_options(path) From 28cf78857cd2036088bcef45ecc1cffe14f6cb0c Mon Sep 17 00:00:00 2001 From: RRosio Date: Fri, 14 Feb 2025 01:21:02 -0800 Subject: [PATCH 11/17] remove unused code --- jupyter_fsspec/file_manager.py | 9 --------- jupyter_fsspec/tests/unit/test_filesystem_manager.py | 11 ----------- 2 files changed, 20 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 56f6490..ec2cef5 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -151,10 +151,6 @@ def _async_available(self, protocol): def initialize_filesystems(self): new_filesystems = {} - if not self.config: - self.config = {} - return - # Init filesystem for fs_config in self.config.get("sources", []): fs_name = fs_config.get("name", None) @@ -166,11 +162,6 @@ def initialize_filesystems(self): if fs_protocol is None: if fs_path: fs_protocol = self._get_protocol_from_path(fs_path) - else: - logger.error( - f"Skipping '{fs_name}': Missing 'protocol' and 'path' to infer it from" - ) - continue # TODO: support for case no path # if not fs_path: diff --git a/jupyter_fsspec/tests/unit/test_filesystem_manager.py b/jupyter_fsspec/tests/unit/test_filesystem_manager.py index a9e9d6c..8735178 100644 --- a/jupyter_fsspec/tests/unit/test_filesystem_manager.py +++ b/jupyter_fsspec/tests/unit/test_filesystem_manager.py @@ -186,17 +186,6 @@ def test_error_validate_config(invalid_mock_config): assert "sources.0.path" in exc_msg -def test_error_initialize_filesystems(caplog): - with patch.object(FileSystemManager, "__init__", lambda self: None): - fs_manager = FileSystemManager() - fs_manager.config = {"sources": [{"name": "inmem"}]} - fs_manager.async_implementations = fs_manager._asynchronous_implementations() - fs_manager.initialize_filesystems() - - expected_msg = "Skipping 'inmem': Missing 'protocol' and 'path' to infer it from" - assert expected_msg in caplog.text - - def test_empty_initialize_filesystems(caplog): with patch.object(FileSystemManager, "__init__", lambda self: None): fs_manager = FileSystemManager() From e1328cd2ceab1ac7c4746a6d227c7dfe2fe6980e Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 00:00:00 -0800 Subject: [PATCH 12/17] revised async fs handling --- conftest.py | 33 +++++++------------ jupyter_fsspec/file_manager.py | 28 ++++------------ jupyter_fsspec/handlers.py | 19 +++++------ jupyter_fsspec/tests/test_api.py | 18 +++++----- .../tests/unit/test_filesystem_manager.py | 4 --- 5 files changed, 37 insertions(+), 65 deletions(-) diff --git a/conftest.py b/conftest.py index 81b3c29..1d405b1 100644 --- a/conftest.py +++ b/conftest.py @@ -156,36 +156,27 @@ def setup_config_file_fs(tmp_path: Path, setup_tmp_local): @pytest.fixture(scope="function") -def fs_manager_instance(setup_config_file_fs, s3_client): +async def fs_manager_instance(setup_config_file_fs, s3_client): fs_manager = setup_config_file_fs fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_fs = fs_info["info"]["instance"] mem_root_path = fs_info["info"]["path"] if mem_fs: - if mem_fs.exists(f"{mem_root_path}/test_dir"): - mem_fs.rm(f"{mem_root_path}/test_dir", recursive=True) - if mem_fs.exists(f"{mem_root_path}/second_dir"): - mem_fs.rm(f"{mem_root_path}/second_dir", recursive=True) - - mem_fs.touch(f"{mem_root_path}/file_in_root.txt") - with mem_fs.open(f"{mem_root_path}/file_in_root.txt", "wb") as f: - f.write("Root file content".encode()) - - mem_fs.mkdir(f"{mem_root_path}/test_dir", exist_ok=True) - mem_fs.mkdir(f"{mem_root_path}/second_dir", exist_ok=True) - # mem_fs.mkdir(f'{mem_root_path}/second_dir/subdir', exist_ok=True) - mem_fs.touch(f"{mem_root_path}/test_dir/file1.txt") - with mem_fs.open(f"{mem_root_path}/test_dir/file1.txt", "wb") as f: - f.write("Test content".encode()) - f.close() + if await mem_fs._exists(f"{mem_root_path}/test_dir"): + await mem_fs._rm(f"{mem_root_path}/test_dir", recursive=True) + if await mem_fs._exists(f"{mem_root_path}/second_dir"): + await mem_fs._rm(f"{mem_root_path}/second_dir", recursive=True) + + await mem_fs._pipe(f"{mem_root_path}/file_in_root.txt", b"Root file content") + + await mem_fs._mkdir(f"{mem_root_path}/test_dir", exist_ok=True) + await mem_fs._mkdir(f"{mem_root_path}/second_dir", exist_ok=True) + + await mem_fs._pipe(f"{mem_root_path}/test_dir/file1.txt", b"Test content") else: print("In memory filesystem NOT FOUND") - if mem_fs.exists(f"{mem_root_path}/test_dir/file1.txt"): - mem_fs.info(f"{mem_root_path}/test_dir/file1.txt") - else: - print("File does not exist!") return fs_manager diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index ec2cef5..6fa38ec 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -2,7 +2,7 @@ from pydantic import BaseModel from typing import Optional, Dict, List from fsspec.utils import infer_storage_options -from fsspec.registry import known_implementations +from fsspec.implementations.asyn_wrapper import AsyncFileSystemWrapper import fsspec import os import sys @@ -34,7 +34,6 @@ def __init__(self, config_file): self.base_dir = jupyter_config_dir() logger.info(f"Using Jupyter config directory: {self.base_dir}") self.config_path = os.path.join(self.base_dir, config_file) - self.async_implementations = self._asynchronous_implementations() self.config = self.load_config(handle_errors=True) self.initialize_filesystems() @@ -132,22 +131,6 @@ def _get_protocol_from_path(path): protocol = storage_options.get("protocol", "file") return protocol - @staticmethod - def _asynchronous_implementations(): - async_filesystems = [] - - for protocol, impl in known_implementations.items(): - try: - fs_class = fsspec.get_filesystem_class(protocol) - if fs_class.async_impl: - async_filesystems.append(protocol) - except Exception: - pass - return async_filesystems - - def _async_available(self, protocol): - return protocol in self.async_implementations - def initialize_filesystems(self): new_filesystems = {} @@ -171,9 +154,12 @@ def initialize_filesystems(self): key = self._encode_key(fs_config) - # Add: _is_protocol_supported? Or rely on fsspec? - fs_async = self._async_available(fs_protocol) - fs = fsspec.filesystem(fs_protocol, asynchronous=fs_async, *args, **kwargs) + fs_class = fsspec.get_filesystem_class(fs_protocol) + if fs_class.async_impl: + fs = fsspec.filesystem(fs_protocol, asynchronous=True, *args, **kwargs) + else: + sync_fs = fsspec.filesystem(fs_protocol, *args, **kwargs) + fs = AsyncFileSystemWrapper(sync_fs) # Store the filesystem instance new_filesystems[key] = { diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 03db81d..647e8ec 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -330,9 +330,10 @@ async def get(self): fs_instance = fs["instance"] response = {"content": None} + is_async = fs_instance.async_impl try: - if fs_instance.async_impl: + if is_async: isdir = await fs_instance._isdir(item_path) else: isdir = fs_instance.isdir(item_path) @@ -340,21 +341,19 @@ async def get(self): if type == "range": range_header = self.request.headers.get("Range") start, end = parse_range(range_header) - if fs_instance.async_impl: + if is_async: result = await fs_instance._cat_ranges( [item_path], [int(start)], [int(end)] ) - if isinstance(result, bytes): - result = result.decode("utf-8") - response["content"] = result else: - # TODO: result = fs_instance.cat_ranges( [item_path], [int(start)], [int(end)] ) - if isinstance(result[0], bytes): - result = result[0].decode("utf-8") - response["content"] = result + + for i in range(len(result)): + if isinstance(result[i], bytes): + result[i] = result[i].decode("utf-8") + response["content"] = result self.set_header("Content-Range", f"bytes {start}-{end}") elif isdir: if fs_instance.async_impl: @@ -479,7 +478,7 @@ async def put(self): try: if fs_instance.async_impl: - isfile = await fs_instance.isfile(item_path) + isfile = await fs_instance._isfile(item_path) else: isfile = fs_instance.isfile(item_path) diff --git a/jupyter_fsspec/tests/test_api.py b/jupyter_fsspec/tests/test_api.py index 1fe0c1c..b2f7b61 100644 --- a/jupyter_fsspec/tests/test_api.py +++ b/jupyter_fsspec/tests/test_api.py @@ -51,7 +51,7 @@ async def test_empty_config(empty_config, jp_fetch): async def test_get_files_memory(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -105,11 +105,11 @@ async def test_get_files_memory(fs_manager_instance, jp_fetch): range_json_file_body = range_file_res.body.decode("utf-8") range_file_body = json.loads(range_json_file_body) assert range_file_body["status"] == "success" - assert range_file_body["content"] == "Test con" + assert range_file_body["content"] == ["Test con"] async def test_post_files(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -156,7 +156,7 @@ async def test_post_files(fs_manager_instance, jp_fetch): async def test_delete_files(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -207,7 +207,7 @@ async def test_delete_files(fs_manager_instance, jp_fetch): async def test_put_files(fs_manager_instance, jp_fetch): # PUT replace entire resource - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -245,7 +245,7 @@ async def test_put_files(fs_manager_instance, jp_fetch): async def test_rename_files(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -305,7 +305,7 @@ async def test_rename_files(fs_manager_instance, jp_fetch): # PATCH partial update without modifying entire data async def xtest_patch_file(fs_manager_instance, jp_fetch): # file only - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] mem_fs = mem_fs_info["info"]["instance"] @@ -326,7 +326,7 @@ async def xtest_patch_file(fs_manager_instance, jp_fetch): # TODO: async def xtest_action_same_fs_files(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance # get_filesystem_by_protocol(filesystem_protocol_string) returns first instance of that filesystem protocol mem_fs_info = fs_manager.get_filesystem_by_protocol("memory") mem_key = mem_fs_info["key"] @@ -433,7 +433,7 @@ async def xtest_action_same_fs_files(fs_manager_instance, jp_fetch): # TODO: Test count files; Upload/download no more than expected async def test_upload_download(fs_manager_instance, jp_fetch): - fs_manager = fs_manager_instance + fs_manager = await fs_manager_instance remote_fs_info = fs_manager.get_filesystem_by_protocol("s3") remote_key = remote_fs_info["key"] remote_fs = remote_fs_info["info"]["instance"] diff --git a/jupyter_fsspec/tests/unit/test_filesystem_manager.py b/jupyter_fsspec/tests/unit/test_filesystem_manager.py index 8735178..d1d3db1 100644 --- a/jupyter_fsspec/tests/unit/test_filesystem_manager.py +++ b/jupyter_fsspec/tests/unit/test_filesystem_manager.py @@ -116,7 +116,6 @@ def test_load_config_empty(setup_config_dir, empty_config_file): fs_manager.filesystems = {} fs_manager.base_dir = setup_config_dir fs_manager.config_path = empty_config_file - fs_manager.async_implementations = fs_manager._asynchronous_implementations() loaded_config = fs_manager.load_config(handle_errors=True) assert loaded_config == {} @@ -133,7 +132,6 @@ def test_load_populated_config(setup_config_dir, config_file): fs_manager.filesystems = {} fs_manager.base_dir = setup_config_dir fs_manager.config_path = config_file - fs_manager.async_implementations = fs_manager._asynchronous_implementations() loaded_config = fs_manager.load_config(handle_errors=True) assert loaded_config == { @@ -160,7 +158,6 @@ def test_check_reload_config(setup_config_dir, config_file): fs_manager = FileSystemManager() fs_manager.config_path = config_file fs_manager.config = {} - fs_manager.async_implementations = fs_manager._asynchronous_implementations() fs_manager.check_reload_config() @@ -190,7 +187,6 @@ def test_empty_initialize_filesystems(caplog): with patch.object(FileSystemManager, "__init__", lambda self: None): fs_manager = FileSystemManager() fs_manager.config = {} - fs_manager.async_implementations = fs_manager._asynchronous_implementations() fs_manager.initialize_filesystems() assert "Initialized filesystem" not in caplog.text From 90a399ade9995e5b8b9303755011390bfd515887 Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 00:21:21 -0800 Subject: [PATCH 13/17] use pydantic model for source object access --- jupyter_fsspec/file_manager.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index 6fa38ec..f499e06 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -20,8 +20,8 @@ class Source(BaseModel): name: str path: str protocol: Optional[str] = None - args: Optional[List] = None - kwargs: Optional[Dict] = None + args: Optional[List] = [] + kwargs: Optional[Dict] = {} class Config(BaseModel): @@ -136,11 +136,12 @@ def initialize_filesystems(self): # Init filesystem for fs_config in self.config.get("sources", []): - fs_name = fs_config.get("name", None) - fs_path = fs_config.get("path", None) - fs_protocol = fs_config.get("protocol", None) - args = fs_config.get("args", []) - kwargs = fs_config.get("kwargs", {}) + config = Source(**fs_config) + fs_name = config.name + fs_path = config.path + fs_protocol = config.protocol + args = config.args + kwargs = config.kwargs if fs_protocol is None: if fs_path: From afb2b1d53f88994e007ef3eca54a1c0d757e1978 Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 00:29:46 -0800 Subject: [PATCH 14/17] allow key error to happen --- jupyter_fsspec/file_manager.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index f499e06..b44a38d 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -199,6 +199,4 @@ def get_filesystem_by_protocol(self, fs_protocol): def get_filesystem_protocol(self, key): filesystem_rep = self.filesystems.get(key) - if not filesystem_rep: - return None return filesystem_rep["protocol"] + "://" From 77cc97f514f57f764a6c73a7bd258042fc46d07d Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 11:33:13 -0800 Subject: [PATCH 15/17] update to general exception handling --- jupyter_fsspec/handlers.py | 44 +++++++------------------------------- 1 file changed, 8 insertions(+), 36 deletions(-) diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 647e8ec..129366d 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -47,44 +47,16 @@ def validate_fs(self, request_type, key, item_path): @contextmanager -def handle_exception(handler, status_code=500): +def handle_exception( + handler, + status_code=500, + exceptions=(yaml.YAMLError, ValidationError, FileNotFoundError), + default_msg="Error loading config file.", +): try: yield - except yaml.YAMLError as e: - error_message = { - "error": "YAMLError", - "details": getattr(e, "problem", str(e)), - "line": getattr(e, "problem_mark", None).line + 1 - if getattr(e, "problem_mark", None) - else None, - } - - logger.error(error_message) - - handler.set_status(status_code) - handler.write({"status": "failed", "description": error_message, "content": []}) - - handler.finish() - raise ConfigFileException - except ValidationError as e: - error_message = [] - for err in e.errors(): - error = f"{err['msg']}: {err['type']} {err['loc']}" - error_message.append(error) - logger.error(error_message) - - handler.set_status(status_code) - handler.write( - { - "status": "failed", - "description": {error: "ValidationError", "details": error_message}, - "content": [], - } - ) - - raise ConfigFileException - except Exception as e: - error_message = f"{type(e).__name__}: {str(e)}" + except exceptions as e: + error_message = f"{type(e).__name__}: {str(e)}" if str(e) else default_msg logger.error(error_message) handler.set_status(status_code) From 267d91a409beba6530972df6f58c655477d367e9 Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 12:47:30 -0800 Subject: [PATCH 16/17] handle permission errors --- jupyter_fsspec/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyter_fsspec/handlers.py b/jupyter_fsspec/handlers.py index 129366d..c12ed25 100644 --- a/jupyter_fsspec/handlers.py +++ b/jupyter_fsspec/handlers.py @@ -50,7 +50,7 @@ def validate_fs(self, request_type, key, item_path): def handle_exception( handler, status_code=500, - exceptions=(yaml.YAMLError, ValidationError, FileNotFoundError), + exceptions=(yaml.YAMLError, ValidationError, FileNotFoundError, PermissionError), default_msg="Error loading config file.", ): try: From 4c3cb7129dd80875b2415cfafb8af9d112aa1f2f Mon Sep 17 00:00:00 2001 From: RRosio Date: Tue, 18 Feb 2025 15:21:50 -0800 Subject: [PATCH 17/17] remove permission check and update test --- jupyter_fsspec/file_manager.py | 3 --- jupyter_fsspec/tests/unit/test_filesystem_manager.py | 5 ++++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/jupyter_fsspec/file_manager.py b/jupyter_fsspec/file_manager.py index b44a38d..204cc82 100644 --- a/jupyter_fsspec/file_manager.py +++ b/jupyter_fsspec/file_manager.py @@ -103,9 +103,6 @@ def create_config_file(self): logger.debug(f"Ensuring config directory exists: {config_dir}.") os.makedirs(config_dir, exist_ok=True) - if not os.access(config_dir, os.W_OK): - raise PermissionError(f"Config directory was not writable: {config_dir}") - config_path = self.config_path placeholder_config = { "sources": [ diff --git a/jupyter_fsspec/tests/unit/test_filesystem_manager.py b/jupyter_fsspec/tests/unit/test_filesystem_manager.py index d1d3db1..7d31289 100644 --- a/jupyter_fsspec/tests/unit/test_filesystem_manager.py +++ b/jupyter_fsspec/tests/unit/test_filesystem_manager.py @@ -1,5 +1,6 @@ import pytest import yaml +import os from pydantic import ValidationError from pathlib import Path from jupyter_fsspec.file_manager import FileSystemManager @@ -193,13 +194,15 @@ def test_empty_initialize_filesystems(caplog): def test_error_create_config_file(setup_config_dir, config_file): + os.chmod(config_file, 0o44) with patch("os.access", return_value=False): with pytest.raises(PermissionError) as exc: fs_manager = FileSystemManager(config_file) fs_manager.create_config_file() - expected_exc_msg = f"Config directory was not writable: {setup_config_dir.parent}" + expected_exc_msg = f"[Errno 13] Permission denied: '{config_file}'" assert expected_exc_msg == str(exc.value) + os.chmod(config_file, 0o755) def test_error_retrieve_config_content(bad_yaml_config):