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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

use fsspec instead of gfile for all IO #3320

Merged
merged 3 commits into from
Sep 3, 2020
Merged

Conversation

f4hy
Copy link
Contributor

@f4hy f4hy commented Sep 2, 2020

What does this PR do?

This better supports remote (and local) file operations with a dedicated package.

Fixes #3242

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 馃檭

@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #3320 into master will increase coverage by 0%.
The diff coverage is 95%.

@@          Coverage Diff           @@
##           master   #3320   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          90      90           
  Lines        8158    8138   -20     
======================================
- Hits         7362    7349   -13     
+ Misses        796     789    -7     

@Borda Borda added feature Is an improvement or enhancement priority: 0 High priority task v1.0 allowed labels Sep 2, 2020
@Borda Borda added this to the 0.9.x milestone Sep 2, 2020
@f4hy f4hy removed the request for review from a team September 2, 2020 20:03
@mergify mergify bot requested a review from a team September 2, 2020 20:03
This better supports remote (and local) file operations with a dedicated package
@f4hy f4hy changed the title use fsspec instead of gfile for all IO [Ready for review] use fsspec instead of gfile for all IO Sep 3, 2020
Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

LGTM, Just one minor thing

pytorch_lightning/utilities/cloud_io.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team September 3, 2020 07:00
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -7,3 +7,4 @@ future>=0.17.1 # required for builtins in setup.py
# pyyaml>=3.13
PyYAML>=5.1 # OmegaConf requirement >=5.1
tqdm>=4.41.0
fsspec>=0.8.0
Copy link
Member

Choose a reason for hiding this comment

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

is this a minimal or this was just you are using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ya, that was just the version I tested on. I think this will work for my purposes as early as 0.6. We can relax this.

Copy link
Member

Choose a reason for hiding this comment

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

I will check it in #3132

@Borda Borda added the ready PRs ready to be merged label Sep 3, 2020
@mergify mergify bot requested a review from a team September 3, 2020 09:16
@Borda Borda changed the title [Ready for review] use fsspec instead of gfile for all IO use fsspec instead of gfile for all IO Sep 3, 2020
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

Nice work 馃憤

@Borda Borda merged commit 2d8c1b7 into Lightning-AI:master Sep 3, 2020
Borda added a commit that referenced this pull request Sep 18, 2020
* Fix ModelCheckpoint's name formatting

* Fix failing tests

* Add dot to CHECKPOINT_SUFFIX

* Set variables to their default values at the end of tests

* Fix logic for filepath='' and filename=None. Add test

* Fix Windows tests

* Fix typo. Remove leading line break and zeroes

* Remove CHECKPOINT_SUFFIX

* Fix typos. Use appropriate f-string format

* Apply suggestions from code review

* Fix broken tests after #3320

* Finish changes suggested by Borda

* Use explicit test var names

* Apply suggestions

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* Apply suggestions

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* Update CHANGELOG

* Apply suggestions from code review

* for

* prepend whitespace in warn msg

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Adrian W盲lchli <aedu.waelchli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

find alternative to gfile
5 participants