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

fs: http: fix isfile method #116

Merged
merged 3 commits into from
Aug 4, 2022
Merged

fs: http: fix isfile method #116

merged 3 commits into from
Aug 4, 2022

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Aug 3, 2022

This seemingly minor change fixes pushing data to an HTTP remote storage with dvc v2.16.0. At the moment, dvc v2.16.0 cannot push any data at all to an HTTP remote storage!

I noticed that with dvc v2.15.0 pushing data to an HTTP remote storage works fine while it doesn't work - i.e. data just isn't transferred without an error - with v2.16.0. Between dvc v2.15.0 and v2.16.0, dvc-objects (a transitive dependency of dvc via dvc-data) was bumped from v0.1.5 to v0.1.6 by the bump of dvc-data from v0.1.5 to v0.1.6. After some digging, I arrived at dvc-objects and #114 which, among other changes, changed the implementation of the ObjectDB.exists(...) method:

 def exists(self, oid: str) -> bool:
-    return self.fs.exists(self.oid_to_path(oid))
+    return self.fs.isfile(self.oid_to_path(oid))

Now, FileSystem.exists(...) simply delegates the call to the underlying fsspec filesystem implementation, but HTTPFileSystem.isfile(...) overrides this delegation by a hardcoded return value True which makes the ObjectDB.exists(...) method always return True and prevents any data upload to an HTTP remote storage because every file is considered to exist already.

@efiop efiop requested a review from skshetry August 3, 2022 21:39
`isfile` is implemented already in the base class.
@codecov-commenter
Copy link

Codecov Report

Merging #116 (b314c2c) into main (ebee287) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   67.10%   67.11%   +0.01%     
==========================================
  Files          39       39              
  Lines        2371     2369       -2     
  Branches      275      275              
==========================================
- Hits         1591     1590       -1     
+ Misses        750      749       -1     
  Partials       30       30              
Impacted Files Coverage Δ
src/dvc_objects/fs/implementations/http.py 38.15% <ø> (-0.31%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Thank you @sisp 🙂

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.

3 participants