From 3f86488a003c3ab0c45edd1263cf4094b55695db Mon Sep 17 00:00:00 2001 From: Tim Paine <3105306+timkpaine@users.noreply.github.com> Date: Sat, 18 Nov 2023 13:07:24 -0500 Subject: [PATCH 1/3] Re-add sync contents manager, fixes #179, fixes #181 --- jupyterfs/extension.py | 8 ++-- jupyterfs/metamanager.py | 60 +++++++++++++++++++++-------- jupyterfs/pathutils.py | 60 ++++++++++++++++++----------- jupyterfs/tests/test_metamanager.py | 14 ++++++- 4 files changed, 100 insertions(+), 42 deletions(-) diff --git a/jupyterfs/extension.py b/jupyterfs/extension.py index 0282928..7fd5f41 100644 --- a/jupyterfs/extension.py +++ b/jupyterfs/extension.py @@ -10,7 +10,7 @@ from jupyter_server.utils import url_path_join from ._version import __version__ # noqa: F401 -from .metamanager import MetaManager, MetaManagerHandler +from .metamanager import MetaManagerShared, MetaManager, MetaManagerHandler from .snippets import SnippetsHandler _mm_config_warning_msg = """Misconfiguration of MetaManager. Please add: @@ -37,11 +37,13 @@ def _load_jupyter_server_extension(serverapp): base_url = web_app.settings["base_url"] host_pattern = ".*$" - if not isinstance(serverapp.contents_manager, MetaManager): + if not isinstance(serverapp.contents_manager, MetaManagerShared): warnings.warn(_mm_config_warning_msg) return - if isinstance(serverapp.contents_manager_class, type) and not issubclass(serverapp.contents_manager_class, MetaManager): + if isinstance(serverapp.contents_manager_class, type) and not issubclass( + serverapp.contents_manager_class, MetaManagerShared + ): serverapp.contents_manager_class = MetaManager serverapp.log.info("Configuring jupyter-fs manager as the content manager class") diff --git a/jupyterfs/metamanager.py b/jupyterfs/metamanager.py index 37a30b1..8df85a3 100644 --- a/jupyterfs/metamanager.py +++ b/jupyterfs/metamanager.py @@ -15,7 +15,10 @@ from fs.errors import FSError from fs.opener.errors import OpenerError, ParseError from jupyter_server.base.handlers import APIHandler -from jupyter_server.services.contents.manager import AsyncContentsManager +from jupyter_server.services.contents.manager import ( + AsyncContentsManager, + ContentsManager, +) from .auth import substituteAsk, substituteEnv, substituteNone from .config import JupyterFs as JupyterFsConfig @@ -27,10 +30,10 @@ path_old_new, ) -__all__ = ["MetaManager", "MetaManagerHandler"] +__all__ = ["MetaManager", "SyncMetaManager", "MetaManagerHandler"] -class MetaManager(AsyncContentsManager): +class MetaManagerShared: copy_pat = re.compile(r"\-Copy\d*\.") @default("files_handler_params") @@ -149,25 +152,52 @@ def root_dir(self): file_exists = path_kwarg("file_exists", "", False) exists = path_first_arg("exists", False) - save = path_second_arg("save", "model", True) - rename = path_old_new("rename", False) + save = path_second_arg("save", "model", True, sync=False) + rename = path_old_new("rename", False, sync=False) + + get = path_first_arg("get", True, sync=False) + delete = path_first_arg("delete", False, sync=False) + + get_kernel_path = path_first_arg("get_kernel_path", False, sync=True) + + create_checkpoint = path_first_arg("create_checkpoint", False, sync=False) + list_checkpoints = path_first_arg("list_checkpoints", False, sync=False) + restore_checkpoint = path_second_arg( + "restore_checkpoint", "checkpoint_id", False, sync=False + ) + delete_checkpoint = path_second_arg( + "delete_checkpoint", "checkpoint_id", False, sync=False + ) + + +class SyncMetaManager(MetaManagerShared, ContentsManager): + ... + + +class MetaManager(MetaManagerShared, AsyncContentsManager): + async def copy(self, from_path, to_path=None): + return super(MetaManagerShared, self).copy(from_path=from_path, to_path=to_path) + + is_hidden = path_first_arg("is_hidden", False, sync=False) + dir_exists = path_first_arg("dir_exists", False, sync=False) + file_exists = path_kwarg("file_exists", "", False, sync=False) + exists = path_first_arg("exists", False, sync=False) + + save = path_second_arg("save", "model", True, sync=False) + rename = path_old_new("rename", False, sync=False) - get = path_first_arg("get", True) - delete = path_first_arg("delete", False) + get = path_first_arg("get", True, sync=False) + delete = path_first_arg("delete", False, sync=False) get_kernel_path = path_first_arg("get_kernel_path", False, sync=True) - create_checkpoint = path_first_arg("create_checkpoint", False) - list_checkpoints = path_first_arg("list_checkpoints", False) + create_checkpoint = path_first_arg("create_checkpoint", False, sync=False) + list_checkpoints = path_first_arg("list_checkpoints", False, sync=False) restore_checkpoint = path_second_arg( - "restore_checkpoint", - "checkpoint_id", - False, + "restore_checkpoint", "checkpoint_id", False, sync=False ) delete_checkpoint = path_second_arg( - "delete_checkpoint", - "checkpoint_id", - False, + "delete_checkpoint", "checkpoint_id", False, sync=False ) diff --git a/jupyterfs/pathutils.py b/jupyterfs/pathutils.py index ad3187d..45a539b 100644 --- a/jupyterfs/pathutils.py +++ b/jupyterfs/pathutils.py @@ -89,61 +89,69 @@ def path_first_arg(method_name, returns_model, sync=False): """Decorator for methods that accept path as a first argument, e.g. manager.get(path, ...)""" - if sync: - - def _wrapper(self, *args, **kwargs): - path, args = _get_arg("path", args, kwargs) - _, mgr, mgr_path = _resolve_path(path, self._managers) - result = getattr(mgr, method_name)(mgr_path, *args, **kwargs) - return result + def _wrapper(self, *args, **kwargs): + path, args = _get_arg("path", args, kwargs) + _, mgr, mgr_path = _resolve_path(path, self._managers) + result = getattr(mgr, method_name)(mgr_path, *args, **kwargs) + return result - else: + if sync: + return _wrapper - async def _wrapper(self, *args, **kwargs): - path, args = _get_arg("path", args, kwargs) - _, mgr, mgr_path = _resolve_path(path, self._managers) - result = getattr(mgr, method_name)(mgr_path, *args, **kwargs) - return result + async def _wrapper2(self, *args, **kwargs): + return _wrapper(self, *args, **kwargs) - return _wrapper + return _wrapper2 -def path_second_arg(method_name, first_argname, returns_model): +def path_second_arg(method_name, first_argname, returns_model, sync=False): """Decorator for methods that accept path as a second argument. e.g. manager.save(model, path, ...)""" - async def _wrapper(self, *args, **kwargs): + def _wrapper(self, *args, **kwargs): other, args = _get_arg(first_argname, args, kwargs) path, args = _get_arg("path", args, kwargs) _, mgr, mgr_path = _resolve_path(path, self._managers) result = getattr(mgr, method_name)(other, mgr_path, *args, **kwargs) return result - return _wrapper + if sync: + return _wrapper + + async def _wrapper2(self, *args, **kwargs): + return _wrapper(self, *args, **kwargs) + + return _wrapper2 -def path_kwarg(method_name, path_default, returns_model): +def path_kwarg(method_name, path_default, returns_model, sync=False): """Parameterized decorator for methods that accept path as a second argument. e.g. manager.file_exists(path='') """ - async def _wrapper(self, path=path_default, **kwargs): + def _wrapper(self, path=path_default, **kwargs): _, mgr, mgr_path = _resolve_path(path, self._managers) result = getattr(mgr, method_name)(path=mgr_path, **kwargs) return result - return _wrapper + if sync: + return _wrapper + + async def _wrapper2(self, *args, **kwargs): + return _wrapper(self, *args, **kwargs) + return _wrapper2 -def path_old_new(method_name, returns_model): + +def path_old_new(method_name, returns_model, sync=False): """Decorator for methods accepting old_path and new_path. e.g. manager.rename(old_path, new_path) """ - async def _wrapper(self, old_path, new_path, *args, **kwargs): + def _wrapper(self, old_path, new_path, *args, **kwargs): old_prefix, old_mgr, old_mgr_path = _resolve_path(old_path, self._managers) new_prefix, new_mgr, new_mgr_path = _resolve_path(new_path, self._managers) if old_mgr is not new_mgr: @@ -159,7 +167,13 @@ async def _wrapper(self, old_path, new_path, *args, **kwargs): result = getattr(new_mgr, method_name)(old_mgr_path, new_mgr_path, *args, **kwargs) return result - return _wrapper + if sync: + return _wrapper + + async def _wrapper2(self, *args, **kwargs): + return _wrapper(self, *args, **kwargs) + + return _wrapper2 # handlers for drive specifications in path strings, as in "fooDrive:bar/baz.buzz" diff --git a/jupyterfs/tests/test_metamanager.py b/jupyterfs/tests/test_metamanager.py index 8d5f89c..68c7a68 100644 --- a/jupyterfs/tests/test_metamanager.py +++ b/jupyterfs/tests/test_metamanager.py @@ -20,6 +20,14 @@ "JupyterFs": {}, } +sync_base_config = { + "ServerApp": { + "jpserver_extensions": {"jupyterfs.extension": True}, + "contents_manager_class": "jupyterfs.metamanager.SyncMetaManager", + }, + "JupyterFs": {}, +} + deny_client_config = { "JupyterFs": { "allow_user_resources": False, @@ -40,7 +48,7 @@ def our_config(): @pytest.fixture -def jp_server_config(tmp_path, tmp_osfs_resource, our_config): +def jp_server_config(base_config, tmp_path, tmp_osfs_resource, our_config): c = Config(base_config) c.JupyterFs.setdefault("resources", []) if tmp_osfs_resource: @@ -54,6 +62,7 @@ def jp_server_config(tmp_path, tmp_osfs_resource, our_config): return c +@pytest.mark.parametrize("base_config", [base_config, sync_base_config]) @pytest.mark.parametrize("our_config", [deny_client_config]) async def test_client_creation_disallowed(tmp_path, jp_fetch, jp_server_config): cc = ContentsClient(jp_fetch) @@ -61,6 +70,7 @@ async def test_client_creation_disallowed(tmp_path, jp_fetch, jp_server_config): assert resources == [] +@pytest.mark.parametrize("base_config", [base_config, sync_base_config]) @pytest.mark.parametrize("our_config", [deny_client_config]) @pytest.mark.parametrize("tmp_osfs_resource", [True]) async def test_client_creation_disallowed_retains_server_config(tmp_path, jp_fetch, jp_server_config): @@ -70,6 +80,7 @@ async def test_client_creation_disallowed_retains_server_config(tmp_path, jp_fet assert names == {"test-server-config"} +@pytest.mark.parametrize("base_config", [base_config, sync_base_config]) @pytest.mark.parametrize( "our_config", [ @@ -118,6 +129,7 @@ async def test_resource_validators(tmp_path, jp_fetch, jp_server_config): assert names == {"valid-1", "valid-2"} +@pytest.mark.parametrize("base_config", [base_config, sync_base_config]) @pytest.mark.parametrize( "our_config", [ From af914219d255127b85923cd4e6ae448cbf90bd07 Mon Sep 17 00:00:00 2001 From: Tim Paine <3105306+timkpaine@users.noreply.github.com> Date: Thu, 23 May 2024 12:14:00 +0200 Subject: [PATCH 2/3] Move sync contents manager methods to be sync --- jupyterfs/metamanager.py | 27 +++++++++++++-------------- jupyterfs/tests/test_metamanager.py | 8 ++++++++ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/jupyterfs/metamanager.py b/jupyterfs/metamanager.py index 8df85a3..0861092 100644 --- a/jupyterfs/metamanager.py +++ b/jupyterfs/metamanager.py @@ -147,31 +147,30 @@ def root_manager(self): def root_dir(self): return self.root_manager.root_dir - is_hidden = path_first_arg("is_hidden", False) - dir_exists = path_first_arg("dir_exists", False) - file_exists = path_kwarg("file_exists", "", False) - exists = path_first_arg("exists", False) + is_hidden = path_first_arg("is_hidden", False, sync=True) + dir_exists = path_first_arg("dir_exists", False, sync=True) + file_exists = path_kwarg("file_exists", "", False, sync=True) + exists = path_first_arg("exists", False, sync=False) - save = path_second_arg("save", "model", True, sync=False) - rename = path_old_new("rename", False, sync=False) + save = path_second_arg("save", "model", True, sync=True) + rename = path_old_new("rename", False, sync=True) - get = path_first_arg("get", True, sync=False) - delete = path_first_arg("delete", False, sync=False) + get = path_first_arg("get", True, sync=True) + delete = path_first_arg("delete", False, sync=True) get_kernel_path = path_first_arg("get_kernel_path", False, sync=True) - create_checkpoint = path_first_arg("create_checkpoint", False, sync=False) - list_checkpoints = path_first_arg("list_checkpoints", False, sync=False) + create_checkpoint = path_first_arg("create_checkpoint", False, sync=True) + list_checkpoints = path_first_arg("list_checkpoints", False, sync=True) restore_checkpoint = path_second_arg( - "restore_checkpoint", "checkpoint_id", False, sync=False + "restore_checkpoint", "checkpoint_id", False, sync=True ) delete_checkpoint = path_second_arg( - "delete_checkpoint", "checkpoint_id", False, sync=False + "delete_checkpoint", "checkpoint_id", False, sync=True ) -class SyncMetaManager(MetaManagerShared, ContentsManager): - ... +class SyncMetaManager(MetaManagerShared, ContentsManager): ... class MetaManager(MetaManagerShared, AsyncContentsManager): diff --git a/jupyterfs/tests/test_metamanager.py b/jupyterfs/tests/test_metamanager.py index 68c7a68..fc51e22 100644 --- a/jupyterfs/tests/test_metamanager.py +++ b/jupyterfs/tests/test_metamanager.py @@ -155,3 +155,11 @@ async def test_resource_validators_no_auth(tmp_path, jp_fetch, jp_server_config) ) names = set(map(lambda r: r["name"], resources)) assert names == {"valid-1", "valid-2"} + + +@pytest.mark.parametrize("base_config", [base_config, sync_base_config]) +@pytest.mark.parametrize("our_config", [{}]) +async def test_basic_sanity_check(tmp_path, jp_fetch, jp_server_config): + cc = ContentsClient(jp_fetch) + resources = await cc.get("/") + assert resources["type"] == "directory" From fc4d3d4d371b1c8c5d463df2db531ceee8b9bd7b Mon Sep 17 00:00:00 2001 From: Tim Paine <3105306+timkpaine@users.noreply.github.com> Date: Thu, 23 May 2024 12:18:13 +0200 Subject: [PATCH 3/3] fix lint --- jupyterfs/extension.py | 4 +--- jupyterfs/metamanager.py | 16 ++++------------ 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/jupyterfs/extension.py b/jupyterfs/extension.py index 7fd5f41..5e5b584 100644 --- a/jupyterfs/extension.py +++ b/jupyterfs/extension.py @@ -41,9 +41,7 @@ def _load_jupyter_server_extension(serverapp): warnings.warn(_mm_config_warning_msg) return - if isinstance(serverapp.contents_manager_class, type) and not issubclass( - serverapp.contents_manager_class, MetaManagerShared - ): + if isinstance(serverapp.contents_manager_class, type) and not issubclass(serverapp.contents_manager_class, MetaManagerShared): serverapp.contents_manager_class = MetaManager serverapp.log.info("Configuring jupyter-fs manager as the content manager class") diff --git a/jupyterfs/metamanager.py b/jupyterfs/metamanager.py index 0861092..303c6d7 100644 --- a/jupyterfs/metamanager.py +++ b/jupyterfs/metamanager.py @@ -162,12 +162,8 @@ def root_dir(self): create_checkpoint = path_first_arg("create_checkpoint", False, sync=True) list_checkpoints = path_first_arg("list_checkpoints", False, sync=True) - restore_checkpoint = path_second_arg( - "restore_checkpoint", "checkpoint_id", False, sync=True - ) - delete_checkpoint = path_second_arg( - "delete_checkpoint", "checkpoint_id", False, sync=True - ) + restore_checkpoint = path_second_arg("restore_checkpoint", "checkpoint_id", False, sync=True) + delete_checkpoint = path_second_arg("delete_checkpoint", "checkpoint_id", False, sync=True) class SyncMetaManager(MetaManagerShared, ContentsManager): ... @@ -192,12 +188,8 @@ async def copy(self, from_path, to_path=None): create_checkpoint = path_first_arg("create_checkpoint", False, sync=False) list_checkpoints = path_first_arg("list_checkpoints", False, sync=False) - restore_checkpoint = path_second_arg( - "restore_checkpoint", "checkpoint_id", False, sync=False - ) - delete_checkpoint = path_second_arg( - "delete_checkpoint", "checkpoint_id", False, sync=False - ) + restore_checkpoint = path_second_arg("restore_checkpoint", "checkpoint_id", False, sync=False) + delete_checkpoint = path_second_arg("delete_checkpoint", "checkpoint_id", False, sync=False) class MetaManagerHandler(APIHandler):