Added complete support for pathlib #1678

pioneerHitesh · 2023-10-18T10:40:37Z

Added pathlib (path object) support for the entire dataset module

Closes #1616

pioneerHitesh · 2023-10-18T10:41:42Z

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree

torchgeo/datasets/agb_live_woody_density.py

torchgeo/datasets/geo.py

… added ``Iterable[os.PathLike]`` support.

…rchgeo into pathlib-support

pioneerHitesh · 2023-10-18T14:14:36Z

@adamjstewart There is an urllib.error.HTTPError: HTTP Error 500: Internal Server Error occuring irrespective of path being str or os.PathLike . This issue occures when compiling from github and installing using pip3. It is occuring for AbovegroundLiveWoodyBiomassDensity which is a module from torchgeo.datasets.agb_live_woody_density

Here are the logs

Traceback (most recent call last):
  File "/home/hitesh/torchgeo/test.py", line 13, in <module>
    AgLWBD = AbovegroundLiveWoodyBiomassDensity(paths="/home/hitesh/test", download=True)
  File "/home/hitesh/torchgeo/torchgeo/datasets/agb_live_woody_density.py", line 91, in __init__
    self._verify()
  File "/home/hitesh/torchgeo/torchgeo/datasets/agb_live_woody_density.py", line 114, in _verify
    self._download()
  File "/home/hitesh/torchgeo/torchgeo/datasets/agb_live_woody_density.py", line 123, in _download
    download_url(self.url, self.paths, self.base_filename)
  File "/home/hitesh/torchgeo/env/lib/python3.10/site-packages/torchvision/datasets/utils.py", line 134, in download_url
    url = _get_redirect_url(url, max_hops=max_redirect_hops)
  File "/home/hitesh/torchgeo/env/lib/python3.10/site-packages/torchvision/datasets/utils.py", line 82, in _get_redirect_url
    with urllib.request.urlopen(urllib.request.Request(url, headers=headers)) as response:
  File "/usr/lib/python3.10/urllib/request.py", line 216, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.10/urllib/request.py", line 525, in open
    response = meth(req, response)
  File "/usr/lib/python3.10/urllib/request.py", line 634, in http_response
    response = self.parent.error(
  File "/usr/lib/python3.10/urllib/request.py", line 563, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.10/urllib/request.py", line 496, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.10/urllib/request.py", line 643, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 500: Internal Server Error

adamjstewart · 2023-10-18T14:19:51Z

@pioneerHitesh can you open a separate issue for the download problems? Maybe the URL changed.

torchgeo/datasets/geo.py

torchgeo/datasets/utils.py

torchgeo/datasets/agb_live_woody_density.py

torchgeo/datasets/geo.py

torchgeo/datasets/utils.py

pioneerHitesh · 2023-10-23T04:37:25Z

@adamjstewart any additional changes ?

adamjstewart

At the moment, it seems like 99% of the time, we accept any Path as input, then immediately convert it to a str. I think this is still useful, but it's a lot of extra work to ensure valid types and cast everything. Is torchvision's download_url the only reason we need to cast to a str? I wonder if we could just update download_url to accept any Path as input. I'm willing to submit a PR to torchvision if you think that's worth it. We could even have a wrapper in TorchGeo that just converts the path to a str and calls the torchvision version. Let me know what you think of this idea.

Also, this PR is becoming massive. Is there any way we can split this up into multiple PRs to make it easier on both of us to update/review? Maybe 1 PR to remove os.path.expanduser, another PR to fix root={self.paths} to paths={self.paths!r}, another PR to add support to the datasets, and another PR to update the tests? Or would that be too much work for you?

adamjstewart · 2023-10-25T19:26:56Z

tests/datasets/test_eudem.py

-        assert isinstance(dataset.paths, str)
-        zipfile = os.path.join(dataset.paths, "eu_dem_v11_E30N10.zip")
-        shutil.unpack_archive(zipfile, dataset.paths, "zip")
+        assert check_instance_type(dataset.paths)


I think we can just assert that this is a pathlib.Path

@adamjstewart We may not be able to do that because we have to assert for str,bytes , os.PathLike[str] and
os.PathLike[bytes].

tests/datasets/test_landcoverai.py

tests/datasets/test_mapinwild.py

torchgeo/datasets/agb_live_woody_density.py

torchgeo/datasets/astergdem.py

torchgeo/datasets/nlcd.py

torchgeo/datasets/openbuildings.py

torchgeo/datasets/utils.py

adamjstewart · 2023-10-25T19:46:07Z

torchgeo/datasets/utils.py


 import numpy as np
 import rasterio
 import torch
 from torch import Tensor
 from torchvision.datasets.utils import check_integrity, download_url
 from torchvision.utils import draw_segmentation_masks
+from typing_extensions import TypeAlias


We don't have a dep on typing_extensions right now. Let's just remove the TypeAlias usage, we don't actually need it.

adamjstewart · 2023-10-25T19:50:22Z

torchgeo/datasets/utils.py

@@ -737,3 +740,25 @@ def percentile_normalization(
        (img - lower_percentile) / (upper_percentile - lower_percentile + 1e-5), 0, 1
    )
    return img_normalized
+
+
+def check_instance_type(paths: Path | Iterable[Path]) -> bool:


I don't love this function:

The name is way too vague

The docstring doesn't tell me when it's True and when it's False

Do we really need this complex of a check?

It seems like we're really just checking if paths is a Path (True) or Iterable[Path] (False). Can't we just do:

return not isinstance(paths, Iterable)

? In that case, we wouldn't even need a function. Basically, if we can figure out a reasonable way to remove this function, I would be happier.

@adamjstewart Unfortunately isinstance(paths, Iterable) will return True for bytes and str. We will have to find another way to eliminate this function. One way would be to have List[Path] instead of Iterable[Path].
Then we may use isinstance(paths,List).

pioneerHitesh · 2023-10-26T07:48:29Z

At the moment, it seems like 99% of the time, we accept any Path as input, then immediately convert it to a str. I think this is still useful, but it's a lot of extra work to ensure valid types and cast everything. Is torchvision's download_url the only reason we need to cast to a str? I wonder if we could just update download_url to accept any Path as input. I'm willing to submit a PR to torchvision if you think that's worth it. We could even have a wrapper in TorchGeo that just converts the path to a str and calls the torchvision version. Let me know what you think of this idea.

Also, this PR is becoming massive. Is there any way we can split this up into multiple PRs to make it easier on both of us to update/review? Maybe 1 PR to remove os.path.expanduser, another PR to fix root={self.paths} to paths={self.paths!r}, another PR to add support to the datasets, and another PR to update the tests? Or would that be too much work for you?

In addition to the torchvision's download_url , the os.path.join also needs str. If we can drop support for Path being of type bytes and pathlib.Path[bytes] then we do not need to cast to str. The problem with bytes or pathlib.Path[bytes] is if the second argument passed in the join method is a string it is unable to combine the paths. It also does not support merging of pathlib.Path[bytes] and bytes. Another issue i have discovered is pathlib.path does not support bytes. So we will have to drop support for atleast os.PathLike[bytes] and if feasible also bytes.

My suggestion would be that we add support for os.PathLike[str] in torchvision's download_url as that would benefit others as well and will eliminate the need for writing a wrapper function just to cast to str. I concur with you regarding the PR becoming unviable to update/review and will split the PR. However we cannot split the datasets and tests separately as that would lead to failing of tests instead we could split datasets and tests by limiting the number of datasets and tests files changed in a commit .

adamjstewart · 2023-10-26T13:15:17Z

However we cannot split the datasets and tests separately as that would lead to failing of tests instead we could split datasets and tests by limiting the number of datasets and tests files changed in a commit .

We could fix the datasets in one PR and then update the tests in a second PR after the first one is merged.

pioneerHitesh · 2023-10-26T14:32:16Z

However we cannot split the datasets and tests separately as that would lead to failing of tests instead we could split datasets and tests by limiting the number of datasets and tests files changed in a commit .

We could fix the datasets in one PR and then update the tests in a second PR after the first one is merged.

Should I start implementing the changes?

adamjstewart · 2023-10-26T17:28:22Z

Yeah, let's start with PRs for os.path.expanduser and root={self.paths}, then once those are merged a PR for Path.

pioneerHitesh · 2023-10-26T17:30:37Z

Creating a separate issue would be better? This PR already has 75 comments.

adamjstewart · 2023-10-26T17:53:59Z

We can either close this PR and start new PRs or mark this one as a draft and rebase once other PRs are merged. I have no preference.

pioneerHitesh · 2023-10-26T18:00:57Z

We can either close this PR and start new PRs or mark this one as a draft and rebase once other PRs are merged. I have no preference.

I think we should convert this to draft.

pioneerHitesh · 2023-10-26T18:21:43Z

Yeah, let's start with PRs for os.path.expanduser and root={self.paths}, then once those are merged a PR for Path.

should i rename root to path in __init__ methods?

adamjstewart · 2023-10-26T20:21:07Z

For NonGeoDatasets we want to keep using root. For GeoDatasets, we should use paths everywhere, including in error messages.

pioneerHitesh · 2023-10-27T09:38:16Z

@adamjstewart please take a look at this PR.

adamjstewart · 2023-11-18T15:06:58Z

Closing in favor of #1731

added selective pathlib support

4477422

github-actions bot added the datasets Geospatial or benchmark datasets label Oct 18, 2023

Merge branch 'main' into pathlib-support

570d757

adamjstewart reviewed Oct 18, 2023

View reviewed changes

torchgeo/datasets/agb_live_woody_density.py Outdated Show resolved Hide resolved

torchgeo/datasets/agb_live_woody_density.py Show resolved Hide resolved

torchgeo/datasets/geo.py Outdated Show resolved Hide resolved

pioneerHitesh added 3 commits October 18, 2023 19:12

added pathlib support for torchgeo.datasets.geo.NonGeoDataset and…

92afe94

… added ``Iterable[os.PathLike]`` support.

Merge branch 'pathlib-support' of https://github.com/pioneerHitesh/to…

123a90a

…rchgeo into pathlib-support

Fixed formatting

b839aed

adamjstewart reviewed Oct 18, 2023

View reviewed changes

torchgeo/datasets/geo.py Outdated Show resolved Hide resolved

Added Path type alias

afa7c53

pioneerHitesh requested a review from adamjstewart October 18, 2023 15:37

Fixed TypeAlias issue

11394aa

adamjstewart reviewed Oct 18, 2023

View reviewed changes

torchgeo/datasets/utils.py Outdated Show resolved Hide resolved

pioneerHitesh added 2 commits October 18, 2023 21:51

fixed Typing issues

e33cf05

Fixed TypeErrors in geo.py

6bf458e

adamjstewart reviewed Oct 18, 2023

View reviewed changes

torchgeo/datasets/agb_live_woody_density.py Outdated Show resolved Hide resolved

pioneerHitesh added 3 commits October 18, 2023 22:57

Fixed formatting and Type issues

a1bc278

Fixed Type Errors

15604d7

Fixed formatting and type annotations

b7e0450

github-actions bot added the testing Continuous integration testing label Oct 18, 2023

Fixed Formatting

3bdab08

pioneerHitesh requested a review from adamjstewart October 18, 2023 21:11

adamjstewart reviewed Oct 18, 2023

View reviewed changes

pioneerHitesh added 2 commits October 19, 2023 10:51

Fixed formatting and Type annotations

2e84e13

Fixeed Formatting

2b2455f

pioneerHitesh requested a review from adamjstewart October 19, 2023 05:48

Merge branch 'main' into pathlib-support

ad7140e

pioneerHitesh closed this Oct 19, 2023

github-actions bot added the testing Continuous integration testing label Oct 21, 2023

pioneerHitesh added 2 commits October 21, 2023 20:58

Fixed formatting

1efc19b

Updated pyproject.toml

44ceca2

github-actions bot added the dependencies Packaging and dependencies label Oct 21, 2023

pioneerHitesh added 5 commits October 21, 2023 23:22

Fixed cdl.py

229b4f1

Fixed cdl.py

5533f4d

Fixed bugs in utils.py

26edb85

Fixed formatting

f623ed1

bug fixes

fed0e46

pioneerHitesh requested a review from adamjstewart October 21, 2023 20:20

adamjstewart reviewed Oct 25, 2023

View reviewed changes

adamjstewart added this to the 0.6.0 milestone Oct 25, 2023

pioneerHitesh marked this pull request as draft October 26, 2023 18:09

pioneerHitesh marked this pull request as ready for review October 27, 2023 10:59

pioneerHitesh requested a review from adamjstewart October 27, 2023 11:03

pioneerHitesh mentioned this pull request Nov 12, 2023

Added pathlib support and resolved conflicts. #1726

Closed

adamjstewart removed this from the 0.6.0 milestone Nov 18, 2023

adamjstewart closed this Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added complete support for pathlib #1678

Added complete support for pathlib #1678

pioneerHitesh commented Oct 18, 2023 •

edited

pioneerHitesh commented Oct 18, 2023

pioneerHitesh commented Oct 18, 2023 •

edited by adamjstewart

adamjstewart commented Oct 18, 2023

pioneerHitesh commented Oct 23, 2023

adamjstewart left a comment

adamjstewart Oct 25, 2023

pioneerHitesh Oct 26, 2023

adamjstewart Oct 25, 2023

adamjstewart Oct 25, 2023

pioneerHitesh Oct 26, 2023 •

edited

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023 •

edited

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 27, 2023

adamjstewart commented Nov 18, 2023

Added complete support for pathlib #1678

Added complete support for pathlib #1678

Conversation

pioneerHitesh commented Oct 18, 2023 • edited

pioneerHitesh commented Oct 18, 2023

pioneerHitesh commented Oct 18, 2023 • edited by adamjstewart

adamjstewart commented Oct 18, 2023

pioneerHitesh commented Oct 23, 2023

adamjstewart left a comment

Choose a reason for hiding this comment

adamjstewart Oct 25, 2023

Choose a reason for hiding this comment

pioneerHitesh Oct 26, 2023

Choose a reason for hiding this comment

adamjstewart Oct 25, 2023

Choose a reason for hiding this comment

adamjstewart Oct 25, 2023

Choose a reason for hiding this comment

pioneerHitesh Oct 26, 2023 • edited

Choose a reason for hiding this comment

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023 • edited

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

pioneerHitesh commented Oct 26, 2023

adamjstewart commented Oct 26, 2023

pioneerHitesh commented Oct 27, 2023

adamjstewart commented Nov 18, 2023

pioneerHitesh commented Oct 18, 2023 •

edited

pioneerHitesh commented Oct 18, 2023 •

edited by adamjstewart

pioneerHitesh Oct 26, 2023 •

edited

pioneerHitesh commented Oct 26, 2023 •

edited