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

Add filesystem abstraction using fsspec #8379

Merged
merged 11 commits into from
Nov 22, 2023
Merged

Conversation

asherbondy
Copy link
Contributor

Changes made:

  • Add fs_utils.py for API that offers 1:1 replacement for current filesystem usage.
  • Update usages of open, glob, makedirs, normpath to use new API.
  • Update test/datasets/* to use in-memory filesystem, simplifying test and testing fsspec API.

@rusty1s rusty1s changed the title Add filesystem abstraction using fsspec (#8336). Add filesystem abstraction using fsspec Nov 15, 2023
@asherbondy
Copy link
Contributor Author

Hey folks, Just in case it is not clear from the PR title. I would like to make sure the proposed API is agreed upon based on the presented cases before attempting to carry the fs abstraction throughout the rest of the code base.

Changes made:

* Add fs_utils.py for API that offers 1:1 replacement for current
  filesystem usage.
* Update usages of open, glob, makedirs, normpath to use new API.
* Update test/dataset/* to use in-memory filesystem, simplifying test
  and testing fsspec API.
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (4f5100f) 88.32% compared to head (a023f20) 87.66%.

❗ Current head a023f20 differs from pull request most recent head a42cf72. Consider uploading reports for the commit a42cf72 to get more accurate results

Files Patch % Lines
torch_geometric/io/fs.py 86.48% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8379      +/-   ##
==========================================
- Coverage   88.32%   87.66%   -0.66%     
==========================================
  Files         475      476       +1     
  Lines       28942    29011      +69     
==========================================
- Hits        25562    25433     -129     
- Misses       3380     3578     +198     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

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

Thanks. I think this is a great start. I moved the files to torch_geometric.io and suggest usage of

from torch_geometric.io import fs

I think this is a bit cleaner.

The cp function for my taste seems a bit overloaded. Is it common practice to fuse downloading and extraction here? I think this is a bit error-prone. Made some updates but broke memory:// support in conftest, since fs.ls doesn't return the protocol :(

@rusty1s rusty1s enabled auto-merge (squash) November 22, 2023 11:11
@rusty1s rusty1s merged commit 4d3b9a4 into pyg-team:master Nov 22, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants