[WIP] Add sync handler base #73
Conversation
…ckend sync handler tests
jupyter_fsspec/handlers.py
Outdated
| self.fs_manager = fs_manager | ||
|
|
||
| async def get(self): | ||
| # remote to local (fetching latest changes) |
There was a problem hiding this comment.
If it's to be explicitly "remote to local", then fs.get() may be simpler, unless you really use features from rsync (e.g., testing the file sizes of files, deleting ones that disappeared on remote).
jupyter_fsspec/handlers.py
Outdated
|
|
||
| try: | ||
| # rsync | ||
| # (remote_source_path, local_destination_path) |
There was a problem hiding this comment.
So you don't trigger this yet?
There was a problem hiding this comment.
We can probably figure out here is this is going to be async; OR, we can use the wrapper to make everything async (run on a thread) to make sure we don't block.
jupyter_fsspec/handlers.py
Outdated
| # ==================================================================================== | ||
| # Handle Syncing Local and Remote filesystems | ||
| # ==================================================================================== | ||
| class FilesystemSyncHandler(BaseFileSystemHandler): |
There was a problem hiding this comment.
I would suggest that the FIlesystemManager should have the core functionality, and that the handler only take care of passing on the appropriate parameters and set the output status.
jupyter_fsspec/handlers.py
Outdated
| def initialize(self, fs_manager): | ||
| self.fs_manager = fs_manager | ||
|
|
||
| async def get(self): |
There was a problem hiding this comment.
get implies no change of state. I would call both methods "post", but have them on different endpoints or with different operation parameters as with the other filesystem commands.
jupyter_fsspec/handlers.py
Outdated
| async def get(self): | ||
| # remote to local (fetching latest changes) | ||
| request_data = json.loads(self.request.body.decode("utf-8")) | ||
| local_destination_path = request_data.get("local_path") |
There was a problem hiding this comment.
Were we ever thinking of using pydantic? Unwrapping all the keys and validating seems a little tedious.
The endpoint doc must document verbosely what the expected input is like (e.g., this might eventually appear in swagger generated docs).
jupyter_fsspec/handlers.py
Outdated
| @@ -556,13 +627,15 @@ def setup_handlers(web_app): | |||
| route_fs_file_transfer = url_path_join( | |||
| base_url, "jupyter_fsspec", "files", "transfer" | |||
| ) | |||
| route_fs_sync = url_path_join(base_url, "jupyter_fsspec", "sync") | |||
There was a problem hiding this comment.
I don't really see why we need a separate URL here is we don't generally for other operations.
jupyter_fsspec/tests/test_api.py
Outdated
| fs_manager = 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"] |
There was a problem hiding this comment.
It's weird to me, that the call to get_filesystem succeeds without an exception but returns nothing.
Also, didn't we key these things by configured name rather than protocol?
| @@ -366,6 +366,52 @@ export class FsspecModel { | |||
| } | |||
| } | |||
|
|
|||
| async sync_push( | |||
There was a problem hiding this comment.
We may wish to eventually provide a feedback mechanism (see e.g., https://filesystem-spec.readthedocs.io/en/latest/features.html#callbacks ).
…r requests, add to tests up/downloading from root filesystems
|
Thank you Martin for your feedback! I have worked on some updates:
Still TBD:
|
|
I think that this PR is in a good state to merge in and further iterate on. I will go ahead and merge these changes and continue work as a part of the follow-up tickets that have been linked in my comment above! |
Frontend server call functions and base for backend sync handler and tests