odb: use regular upload() when the source filesystem is local #6365
Conversation
| self.fs.upload(from_info, to_info) | ||
| elif isinstance(self.fs, LocalFileSystem): | ||
| from_fs.download_file(from_info, to_info) |
There was a problem hiding this comment.
Btw, should both of these use _file methods?
There was a problem hiding this comment.
What do you mean by that?
There was a problem hiding this comment.
@isidentical I see that we use fs.upload but fs.download_file. Wondering if they both should be upload/download or upload_file/download_file (likely the latter if we take fsspec's get/put_file future in the context, but maybe I'm missing something important here)
There was a problem hiding this comment.
We don't have upload_file(), so the upload() is put_file. And download_file() is the get_file(). The reason that I didn't use download() is that, it has an extra check against whether the source path is a directory, which is an waste of API call for this case where we know we are adding a single entity (file).
There was a problem hiding this comment.
Makes sense, thank you! I guess we'll switch to get_file/put_file when reducing fsspec_wrapper in the future.
|
LGTM |
In the old remote logic, for transfering to remotes we used
upload()instead ofupload_fobj()which on some backends defaulted toupload_fobjand on some others used the native transfer libraries logic. For some backends like oss / ssh etc. this is faster than theupload_fobj, so this patch adds an extra check to the ODB to match against whether the source file system is local or not. If it is the local filesystem, we can safely use_upload()if not we can still fallback to_upload_fobj().