-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix remote file system (get/put) #1955
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,7 +248,10 @@ | |
self.strip_file_header(from_path), self.strip_file_header(to_path), dirs_exist_ok=True | ||
) | ||
print(f"Getting {from_path} to {to_path}") | ||
return file_system.get(from_path, to_path, recursive=recursive, **kwargs) | ||
dst = file_system.get(from_path, to_path, recursive=recursive, **kwargs) | ||
if isinstance(dst, (str, pathlib.Path)): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh really? didn't see that. link me? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and yes we should, but the action should still be to return the to_path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's part of the spec and I fear this is specific to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AsyncFileSystem.put or AsyncFileSystem.get always return a list. |
||
return dst | ||
return to_path | ||
except OSError as oe: | ||
logger.debug(f"Error in getting {from_path} to {to_path} rec {recursive} {oe}") | ||
file_system = self.get_filesystem(get_protocol(from_path), anonymous=True) | ||
|
@@ -271,7 +274,11 @@ | |
self.strip_file_header(from_path), self.strip_file_header(to_path), dirs_exist_ok=True | ||
) | ||
from_path, to_path = self.recursive_paths(from_path, to_path) | ||
return file_system.put(from_path, to_path, recursive=recursive, **kwargs) | ||
dst = file_system.put(from_path, to_path, recursive=recursive, **kwargs) | ||
if isinstance(dst, (str, pathlib.Path)): | ||
return dst | ||
else: | ||
return to_path | ||
|
||
def put_raw_data( | ||
self, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does dst stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a test like
flytekit/tests/flytekit/unit/core/test_type_engine.py
Line 203 in 38c7687
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dst
is short for destination, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, since fsspec uses dst as well