Skip to content

fs.upload: add support for optional callback #6546

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

Merged
merged 2 commits into from
Sep 14, 2021
Merged

Conversation

skshetry
Copy link
Collaborator

@skshetry skshetry commented Sep 7, 2021

Also removes _upload methods in other fses, and replaces them
with fsspec-compatible put_file which supports fsspec callback
even if they are not fsspec based filesystem.

@skshetry skshetry requested a review from a team as a code owner September 7, 2021 06:02
@skshetry skshetry requested a review from efiop September 7, 2021 06:02
@skshetry skshetry force-pushed the upload_callback branch 5 times, most recently from 8524c36 to 32c4cb3 Compare September 7, 2021 07:34
@skshetry skshetry requested a review from isidentical September 7, 2021 08:04
ExtraArgs=self.fs_args.get("s3_additional_kwargs"),
Config=self._transfer_config,
)
callback.set_size(os.path.getsize(from_file))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we need self.makedirs here in the put_file these days, cc @isidentical.

s3 = rem.fs
s3.fs.mkdir("another/")
s3 = rem.fs.s3
s3.create_bucket(Bucket="another")
Copy link
Collaborator Author

@skshetry skshetry Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The s3.fs.mkdir was failing locally for me, though I am not sure why. I replaced this with the one that shares the intention in a better way, which passes both locally and in CI.

@skshetry skshetry force-pushed the upload_callback branch 2 times, most recently from a6f21be to 28e8ba0 Compare September 13, 2021 07:38
def _upload(
self, from_file, to_info, name=None, no_progress_bar=False, **pbar_args
@staticmethod
def put_file_compat(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary, I'm planning to get rid of _upload/_download from CallbackMixin and then this CallbackMixin will just be a way to provide compat support for callback. Naming is bad, but will just be replace by put_file after I get rid of _download.

super().absolute_update(value)


def tdqm_or_callback_wrapped(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codebase is a bit weird, as we need to support both tqdm and FsspecCallback for now.

@@ -182,9 +181,29 @@ def set_size(self, size):
size = self.fs.info(self.path_info)["size"]
self.progress_bar.total = size
self.progress_bar.refresh()
super().set_size(size)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be useful in tests, as we can introspect size and value easily.

Also removes _upload methods in other fses, and replaces them
with fsspec-compatible put_file which supports fsspec callback
even if they are not fsspec based filesystem.
@skshetry skshetry merged commit 788b4d3 into master Sep 14, 2021
@skshetry skshetry deleted the upload_callback branch September 14, 2021 05:10
@efiop efiop added refactoring Factoring and re-factoring skip-changelog Skips changelog labels Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Factoring and re-factoring skip-changelog Skips changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants