Better fix for load_from_checkpoint() not working with absolute path on Windows #2294

airium · 2020-06-20T00:07:41Z

What does this PR do?

Discussed in #2243, PR #2244 fixed that load_from_checkpoint() not working with absolute path on Windows, but introduced a new bug that on Windows Path(path_or_url).is_file() will always raise OSError on any valid URL, so blocked:

import torch
from pathlib import Path
from urllib.parse import urlparse
def load(path_or_url: str, map_location=None):
    parsed = urlparse(path_or_url)
    if parsed.scheme == '' or Path(path_or_url).is_file():
        # no scheme or local file
        return torch.load(path_or_url, map_location=map_location)
    return torch.hub.load_state_dict_from_url(path_or_url, map_location=map_location)

>>> load('http://somedomainname.tld/somenames.ckpt')
Traceback (most recent call last):
  File "cloud_io.py", line 11, in <module>
    load('http://somedomainname.tld/somenames.ckpt')
  File "cloud_io.py", line 6, in load
    if parsed.scheme == '' or Path(path_or_url).is_file():
  File "C:\Users\xxxxxx\AppData\Local\Programs\Python\Python37\lib\pathlib.py", line 1382, in is_file
    return S_ISREG(self.stat().st_mode)
  File "C:\Users\xxxxx\AppData\Local\Programs\Python\Python37\lib\pathlib.py", line 1178, in stat
    return self._accessor.stat(self)
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: 'http:\\somedomainname.tld\\somenames.ckpt'

This PR makes a better fix:

import torch
from pathlib import Path
from urllib.parse import urlparse
def load(path_or_url: str, map_location=None):
    if urlparse(path_or_url).scheme == '' or Path(path_or_url).drive:
        return torch.load(path_or_url, map_location=map_location)
    else:
        return torch.hub.load_state_dict_from_url(path_or_url, map_location=map_location)

Fixes #2243
Fixes #2244

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 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 🙃

pep8speaks · 2020-06-20T00:07:44Z

Hello @airium! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-26 22:02:35 UTC

codecov · 2020-06-20T03:37:59Z

Codecov Report

Merging #2294 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2294   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          70      70           
  Lines        5501    5500    -1     
======================================
- Hits         4834    4833    -1     
  Misses        667     667

pytorch_lightning/utilities/cloud_io.py

mergify · 2020-06-20T11:40:02Z

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

awaelchli · 2020-06-21T10:25:24Z

@airium I'm getting this error when I run tests on this branch locally:
FAILED test_trainer.py::test_no_val_module[True] - urllib.error.URLError: <urlopen error [WinError 10049] The requested address is not valid in its context>
Do you get the same?

airium · 2020-06-21T20:19:37Z

@airium I'm getting this error when I run tests on this branch locally:
FAILED test_trainer.py::test_no_val_module[True] - urllib.error.URLError: <urlopen error [WinError 10049] The requested address is not valid in its context>
Do you get the same?

No I didn't notice any similar error. Maybe you could paste longer logs? @awaelchli

awaelchli · 2020-06-21T20:25:23Z

Here it is, haven't had the time to look for the cause.
PL version: your branch
Python: 3.7.7
error.xt.txt

airium · 2020-06-22T07:01:04Z

Here it is, haven't had the time to look for the cause.
PL version: your branch
Python: 3.7.7
error.xt.txt

I think it's something wrong at the networking part, and the load() function works as expected if you looked into it.
Here's my log: test_trainer_log.txt

yukw777 · 2020-06-22T14:04:54Z

pytorch_lightning/utilities/cloud_io.py

        return torch.load(path_or_url, map_location=map_location)
-    return torch.hub.load_state_dict_from_url(path_or_url, map_location=map_location)
+    else:


stylistic nitpick: we don't need to have this else clause. I personally find it more "assuring" in a way without the else clause b/c you can be sure that the return statement will catch everything haha

pytorch_lightning/utilities/cloud_io.py

CHANGELOG.md

awaelchli

yeah, looks like the problem is on my side only.
LGTM, but we have to look into the windows CI because that guy never fails tests even though they fail locally (e.g. the yaml test) xD

CHANGELOG.md

mergify · 2020-06-23T11:22:10Z

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

airium · 2020-06-25T13:29:22Z

Hi I am a little confused as I see more unrelated commits are now placed on top of my branch before I want to rebase and resolve conflict. What should I do now before the PR is ready to merge? Is that already OK, or I still need to remove the else part as per @yukw777 suggested and combine multiple README updates into one commit?

mergify · 2020-06-26T13:32:17Z

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

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

awaelchli · 2020-06-26T22:27:15Z

@airium we merged your PR into #2358 and it seems to work fine with windows CI there. just fyi

* no cov * no cov * ReduceOp * group * reduce_op.sum * Update sklearns.py * formatting * horovod * Apply suggestions from code review * horovod * horovod * horovod * horovod * ci * print * ci * timeout * timeout * time * fix * distributed cpu * pipes * time * cpu * spawn * spawn * spawn * tp * separate * os * os * npm * Fix load_from_checkpoint() not working with URL on Windows * Update CHANGELOG * Update CHANGELOG.md Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com> * Apply suggestions from code review * fix * fix meta tags creating empty lines * pyright * node * fix httpserver address * drop tutils.default_trainer_options * imports * Better fix for load_from_checkpoint() not working with absolute path on Windows (#2294) * Fix load_from_checkpoint() not working with URL on Windows * Update CHANGELOG * Update CHANGELOG.md 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> Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com> * drop duplicate Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> Co-authored-by: airium <airium@outlook.com> Co-authored-by: Peter Yu <2057325+yukw777@users.noreply.github.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: AIRIUM <38249940+airium@users.noreply.github.com>

mergify bot requested a review from a team June 20, 2020 00:08

Borda added the bug Something isn't working label Jun 20, 2020

Borda added this to the 0.8.x milestone Jun 20, 2020

Borda approved these changes Jun 20, 2020

View reviewed changes

mergify bot requested a review from a team June 20, 2020 05:50

awaelchli requested changes Jun 20, 2020

View reviewed changes

pytorch_lightning/utilities/cloud_io.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team June 20, 2020 09:00

Borda self-requested a review June 21, 2020 12:31

yukw777 reviewed Jun 22, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

awaelchli approved these changes Jun 23, 2020

View reviewed changes

mergify bot requested a review from a team June 23, 2020 00:50

Borda reviewed Jun 23, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

mergify bot requested a review from a team June 23, 2020 05:23

williamFalcon added the ready PRs ready to be merged label Jun 25, 2020

Borda mentioned this pull request Jun 26, 2020

repair CI for Win #2358

Merged

7 tasks

airium and others added 4 commits June 27, 2020 00:02

Fix load_from_checkpoint() not working with URL on Windows

4ee5435

Update CHANGELOG

aaa43e2

Update CHANGELOG.md

e26592c

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

Apply suggestions from code review

c7292b4

Borda changed the base branch from master to tests/ci-win June 26, 2020 22:38

Borda changed the base branch from tests/ci-win to master June 26, 2020 22:38

Borda changed the base branch from master to tests/ci-win June 26, 2020 22:39

Borda merged commit 2e37e26 into Lightning-AI:tests/ci-win Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better fix for load_from_checkpoint() not working with absolute path on Windows #2294

Better fix for load_from_checkpoint() not working with absolute path on Windows #2294

airium commented Jun 20, 2020 •

edited

Loading

pep8speaks commented Jun 20, 2020 •

edited

Loading

codecov bot commented Jun 20, 2020 •

edited

Loading

mergify bot commented Jun 20, 2020

awaelchli commented Jun 21, 2020 •

edited

Loading

airium commented Jun 21, 2020 •

edited

Loading

awaelchli commented Jun 21, 2020

airium commented Jun 22, 2020

yukw777 Jun 22, 2020

awaelchli left a comment

mergify bot commented Jun 23, 2020

airium commented Jun 25, 2020

mergify bot commented Jun 26, 2020

awaelchli commented Jun 26, 2020

Better fix for load_from_checkpoint() not working with absolute path on Windows #2294

Better fix for load_from_checkpoint() not working with absolute path on Windows #2294

Conversation

airium commented Jun 20, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jun 20, 2020 • edited Loading

Comment last updated at 2020-06-26 22:02:35 UTC

codecov bot commented Jun 20, 2020 • edited Loading

Codecov Report

mergify bot commented Jun 20, 2020

awaelchli commented Jun 21, 2020 • edited Loading

airium commented Jun 21, 2020 • edited Loading

awaelchli commented Jun 21, 2020

airium commented Jun 22, 2020

yukw777 Jun 22, 2020

Choose a reason for hiding this comment

awaelchli left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 23, 2020

airium commented Jun 25, 2020

mergify bot commented Jun 26, 2020

awaelchli commented Jun 26, 2020

airium commented Jun 20, 2020 •

edited

Loading

pep8speaks commented Jun 20, 2020 •

edited

Loading

codecov bot commented Jun 20, 2020 •

edited

Loading

awaelchli commented Jun 21, 2020 •

edited

Loading

airium commented Jun 21, 2020 •

edited

Loading