Allow loading checkpoints from urls #1667

yukw777 · 2020-04-29T18:13:59Z

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 to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1532. I didn't touch hpc loading.

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 · 2020-04-29T18:29:58Z

Codecov Report

Merging #1667 into master will increase coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff           @@
##           master   #1667   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files          75      76    +1     
  Lines        4788    4797    +9     
======================================
+ Hits         4134    4143    +9     
  Misses        654     654

Borda · 2020-05-26T22:30:57Z

@yukw777 how is it going, it this ready for a review? 🦝

yukw777 · 2020-05-27T03:31:39Z

@Borda I wasn’t sure how to write proper tests for this. Can you advise?

Borda · 2020-05-27T08:32:54Z

@Borda I wasn’t sure how to write proper tests for this. Can you advise?

can we somehow load from localhost? otherwise, we shall upload a model to S3 or other storage... but then this test won't work without an internet connection...

yukw777 · 2020-05-27T14:21:29Z

@Borda let me see if there’s a way to run a simple file serving server locally for pytest...

jeremyjordan · 2020-05-27T18:32:35Z

if there’s not, you can always mock out the call to the internet and then have a separate test which marks the test as an integration test (pytest.mark.integration) which should only be run on the CI server

yukw777 · 2020-05-30T21:40:23Z

Alright added some tests! I ended up creating a new pytest fixture that runs a simple server that serves tmpdir.

mergify · 2020-05-31T12:49:18Z

This pull request is now in conflict... :(

pep8speaks · 2020-05-31T17:16:27Z

Hello @yukw777! Thanks for updating this PR.

In the file tests/trainer/test_trainer.py:

Line 56:120: E501 line too long (134 > 119 characters)
Line 93:120: E501 line too long (134 > 119 characters)

Comment last updated at 2020-06-11 19:52:35 UTC

yukw777 · 2020-05-31T19:41:47Z

CircleCI jobs timed out. They should pass if rerun.

awaelchli · 2020-06-07T01:37:23Z

if you rebase, the failing test should pass

mergify · 2020-06-08T11:22:30Z

This pull request is now in conflict... :(

CHANGELOG.md

Borda · 2020-06-08T13:43:21Z

pytorch_lightning/utilities/io.py

+from urllib.parse import urlparse
+
+
+def load(path_or_url: str, map_location=None):


what is type of map_location

we can't really specify this b/c it can be a lot of different things: #1505

Welcome any suggestions.

tests/trainer/test_trainer.py

mergify · 2020-06-09T20:54:54Z

This pull request is now in conflict... :(

Borda

LGTM 🦝

CHANGELOG.md

Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com>

williamFalcon · 2020-06-09T23:16:11Z

this is super cool!
can we document this? add it to the checkpoint section and update the docstrings on the function

Laksh1997 · 2020-06-11T19:42:43Z

Very keen for this feature!

pytorch_lightning/core/saving.py

tests/trainer/test_trainer.py

pytorch_lightning/core/saving.py

Laksh1997 · 2020-06-11T21:38:11Z

Awesome stuff, do we know when the next tagged release will be? (which will include this feature)

Also, I assume this feature supports saving to S3 as well?

Awesome stuff!!

Borda · 2020-06-11T22:02:54Z

Awesome stuff, do we know when the next tagged release will be? (which will include this feature)

most likely by end of this week...
@williamFalcon adding to v0.8.x

* allow loading checkpoints from urls * tmpdir_server fixture * test cases for loading checkpoints from url * dir => root_dir * default map_location to None * test case for resume_from_checkpoint * changelog * doc update * monkeypatch TORCH_HOME to avoid caching * Use a threading server with random ports so that it is easier to clean up * test fixes * pep8 fix * ThreadingHTTPServer support in 3.6 * pep8 fix * fix changelog * separate tests for urls * typo Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> (cherry picked from commit 06cd849)

* allow loading checkpoints from urls * tmpdir_server fixture * test cases for loading checkpoints from url * dir => root_dir * default map_location to None * test case for resume_from_checkpoint * changelog * doc update * monkeypatch TORCH_HOME to avoid caching * Use a threading server with random ports so that it is easier to clean up * test fixes * pep8 fix * ThreadingHTTPServer support in 3.6 * pep8 fix * fix changelog * separate tests for urls * typo Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

danielgafni · 2020-11-28T14:10:07Z

Hello, guys! What's the state of this feature? I'm interested in using s3 with ModelCheckpoint directly. Could I help with the development maybe?

Borda · 2020-11-29T20:13:53Z

Hello, guys! What's the state of this feature? I'm interested in using s3 with ModelCheckpoint directly. Could I help with the development maybe?

Hi, it is in there already, mind try latest version?

danielgafni · 2020-12-05T15:17:29Z

@Borda Hi! Thanks, saving checkpoints to s3 is indeed working! However, logging with TensorboardLogger isn't. Should I make a separate issue for that?

P.S. I suspect it might be a tensorboard issue. It doesn't work with s3 without installing boto3, while TensorBoardLogger is not working without installing a conflicting version of s3fs.

boto3 1.16.30 requires botocore<1.20.0,>=1.19.30, but you'll have botocore 1.17.44 which is incompatible.

danielgafni · 2020-12-10T20:34:13Z

I just tried the new version and the issue I talked about is gone! Very cool!

mergify bot requested a review from a team April 29, 2020 18:23

williamFalcon changed the title ~~allow loading checkpoints from urls~~ [WIP] allow loading checkpoints from urls Apr 30, 2020

Borda added the feature Is an improvement or enhancement label May 26, 2020

Borda added this to the 0.8.0 milestone May 26, 2020

yukw777 marked this pull request as ready for review May 30, 2020 21:39

yukw777 changed the title ~~[WIP] allow loading checkpoints from urls~~ Allow loading checkpoints from urls May 30, 2020

Borda requested changes Jun 8, 2020

View reviewed changes

mergify bot requested a review from a team June 8, 2020 13:46

yukw777 requested review from Borda and removed request for a team June 8, 2020 14:14

mergify bot requested a review from a team June 8, 2020 14:15

Borda modified the milestones: 0.8.0, 0.9.0 Jun 9, 2020

Borda approved these changes Jun 9, 2020

View reviewed changes

mergify bot requested a review from a team June 9, 2020 22:11

yukw777 added 2 commits June 10, 2020 00:39

allow loading checkpoints from urls

126e0e7

tmpdir_server fixture

08fb93e

separate tests for urls

f25ccdd

yukw777 commented Jun 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

typo

bdd627e

Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com>

Borda requested review from awaelchli, jeremyjordan, justusschock and SkafteNicki June 9, 2020 22:53

Borda reviewed Jun 11, 2020

View reviewed changes

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team June 11, 2020 19:51

Borda reviewed Jun 11, 2020

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ae76d52

mergify bot requested a review from a team June 11, 2020 19:52

Borda reviewed Jun 11, 2020

View reviewed changes

pytorch_lightning/core/saving.py Show resolved Hide resolved

mergify bot requested a review from a team June 11, 2020 19:53

Borda requested a review from williamFalcon June 11, 2020 20:01

williamFalcon merged commit 06cd849 into Lightning-AI:master Jun 11, 2020

yukw777 deleted the feature/1532_load-ckpt-url branch June 11, 2020 21:13

Borda modified the milestones: 0.9.0, 0.8.0 Jun 18, 2020

awaelchli mentioned this pull request Jun 26, 2020

repair CI for Win #2358

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow loading checkpoints from urls #1667

Allow loading checkpoints from urls #1667

yukw777 commented Apr 29, 2020 •

edited

Loading

codecov bot commented Apr 29, 2020 •

edited

Loading

Borda commented May 26, 2020

yukw777 commented May 27, 2020

Borda commented May 27, 2020

yukw777 commented May 27, 2020

jeremyjordan commented May 27, 2020

yukw777 commented May 30, 2020

mergify bot commented May 31, 2020

pep8speaks commented May 31, 2020 •

edited

Loading

yukw777 commented May 31, 2020

awaelchli commented Jun 7, 2020

mergify bot commented Jun 8, 2020

Borda Jun 8, 2020

yukw777 Jun 8, 2020

mergify bot commented Jun 9, 2020

Borda left a comment

williamFalcon commented Jun 9, 2020

Laksh1997 commented Jun 11, 2020

Laksh1997 commented Jun 11, 2020

Borda commented Jun 11, 2020

danielgafni commented Nov 28, 2020

Borda commented Nov 29, 2020

danielgafni commented Dec 5, 2020 •

edited

Loading

danielgafni commented Dec 10, 2020

		from urllib.parse import urlparse


		def load(path_or_url: str, map_location=None):

Allow loading checkpoints from urls #1667

Allow loading checkpoints from urls #1667

Conversation

yukw777 commented Apr 29, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

codecov bot commented Apr 29, 2020 • edited Loading

Codecov Report

Borda commented May 26, 2020

yukw777 commented May 27, 2020

Borda commented May 27, 2020

yukw777 commented May 27, 2020

jeremyjordan commented May 27, 2020

yukw777 commented May 30, 2020

mergify bot commented May 31, 2020

pep8speaks commented May 31, 2020 • edited Loading

Comment last updated at 2020-06-11 19:52:35 UTC

yukw777 commented May 31, 2020

awaelchli commented Jun 7, 2020

mergify bot commented Jun 8, 2020

Borda Jun 8, 2020

Choose a reason for hiding this comment

yukw777 Jun 8, 2020

Choose a reason for hiding this comment

mergify bot commented Jun 9, 2020

Borda left a comment

Choose a reason for hiding this comment

williamFalcon commented Jun 9, 2020

Laksh1997 commented Jun 11, 2020

Laksh1997 commented Jun 11, 2020

Borda commented Jun 11, 2020

danielgafni commented Nov 28, 2020

Borda commented Nov 29, 2020

danielgafni commented Dec 5, 2020 • edited Loading

danielgafni commented Dec 10, 2020

yukw777 commented Apr 29, 2020 •

edited

Loading

codecov bot commented Apr 29, 2020 •

edited

Loading

pep8speaks commented May 31, 2020 •

edited

Loading

danielgafni commented Dec 5, 2020 •

edited

Loading