Skip to content
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

spec: wrap/override fsspec httpfs #45

Merged
merged 1 commit into from
Feb 8, 2023
Merged

spec: wrap/override fsspec httpfs #45

merged 1 commit into from
Feb 8, 2023

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Feb 7, 2023

@pmrowla pmrowla requested a review from skshetry February 7, 2023 06:26
@pmrowla pmrowla self-assigned this Feb 7, 2023
@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 7, 2023

It would probably be better for our filesystems to just wrap fsspec directly instead of using the fs.fs redirection that we have right now, but this fixes the issue until we can actually get around to refactoring dvc_objects.fs

dvc_http/spec.py Outdated Show resolved Hide resolved
dvc_http/__init__.py Outdated Show resolved Hide resolved
@skshetry
Copy link
Member

skshetry commented Feb 8, 2023

It would probably be better for our filesystems to just wrap fsspec directly instead of using the fs.fs redirection that we have right now, but this fixes the issue until we can actually get around to refactoring dvc_objects.fs

Ideally, we should just directly use fsspec-provided filesystems. There should be no wrappers.

Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

@pmrowla, I have approved the PR, I have a few minor suggestions inlined. But I think it'd be better to upstream this in fsspec's http implementation itself.

Also, I'll encourage you to open a discussion regarding batching issue in fsspec, as this dvc-http is not the only place this might break.
We also have similar code in dvc-azure, but I am not sure if that breaks anything or not.

https://github.com/iterative/dvc-azure/blob/78e3ff570d176bec954915847c403122a2b8cf95/dvc_azure/__init__.py#L178-L180

@pmrowla pmrowla merged commit 8582d20 into main Feb 8, 2023
@pmrowla pmrowla deleted the wrap-async-putfile branch February 8, 2023 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dvc push doesn't work with PUT method
2 participants