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

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

Merged
merged 4 commits into from
Jun 26, 2020

Conversation

airium
Copy link
Contributor

@airium airium commented Jun 20, 2020

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
Copy link

pep8speaks commented Jun 20, 2020

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

@mergify mergify bot requested a review from a team June 20, 2020 00:08
@codecov
Copy link

codecov bot commented Jun 20, 2020

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           

@Borda Borda added the bug Something isn't working label Jun 20, 2020
@Borda Borda added this to the 0.8.x milestone Jun 20, 2020
@mergify mergify bot requested a review from a team June 20, 2020 05:50
@mergify mergify bot requested a review from a team June 20, 2020 09:00
@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2020

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

@awaelchli
Copy link
Contributor

awaelchli commented Jun 21, 2020

@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?

@Borda Borda self-requested a review June 21, 2020 12:31
@airium
Copy link
Contributor Author

airium commented Jun 21, 2020

@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
Copy link
Contributor

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
Copy link
Contributor Author

airium commented Jun 22, 2020

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

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

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

@mergify mergify bot requested a review from a team June 23, 2020 00:50
CHANGELOG.md Show resolved Hide resolved
@mergify mergify bot requested a review from a team June 23, 2020 05:23
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2020

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

@williamFalcon williamFalcon added the ready PRs ready to be merged label Jun 25, 2020
@airium
Copy link
Contributor Author

airium commented Jun 25, 2020

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
Copy link
Contributor

mergify bot commented Jun 26, 2020

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

@Borda Borda mentioned this pull request Jun 26, 2020
7 tasks
@awaelchli
Copy link
Contributor

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

@Borda Borda changed the base branch from master to tests/ci-win June 26, 2020 22:38
@Borda Borda changed the base branch from tests/ci-win to master June 26, 2020 22:38
@Borda Borda changed the base branch from master to tests/ci-win June 26, 2020 22:39
@Borda Borda merged commit 2e37e26 into Lightning-AI:tests/ci-win Jun 26, 2020
williamFalcon pushed a commit that referenced this pull request Jun 27, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model.load_from_checkpoint tries to open file path as URL and fail
6 participants