fix eurosat prototype mock data setup #5549

pmeier · 2022-03-04T07:28:31Z

torchdata reported CI failures for Windows and macOS for the EuroSAT prototype from #5452 cc @NivekT @Dbhasin1. The mock data is setup slightly wrong, but that shouldn't matter were it not for a quirk on Windows and macOS. Details follow inline.

We didn't detect that, because our prototype tests only run on Linux. I've switched to Windows for debugging. Passing Windows CI run with the patch from this PR: https://app.circleci.com/pipelines/github/pytorch/vision/15300/workflows/c9eb674a-a3fa-4bcc-9824-c047beb47579/jobs/1235498

facebook-github-bot · 2022-03-04T07:28:37Z

💊 CI failures summary and remediations

As of commit 3062c71 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

pmeier · 2022-03-04T08:51:53Z

test/builtin_dataset_mocks.py

@@ -1329,20 +1329,20 @@ def cub200(info, root, config):

 @register_mock
 def eurosat(info, root, config):
-    data_folder = pathlib.Path(root, "eurosat", "2750")
+    data_folder = pathlib.Path(root, "2750")


This extra eurosat folder is a remnant from the legacy mock data and is not present in the actual archive.

pmeier · 2022-03-04T09:02:37Z

test/builtin_dataset_mocks.py

-            name=cls,
-            file_name_fn=lambda idx: f"{cls}_{idx}.jpg",
+            name=category,
+            file_name_fn=lambda idx: f"{category}_{idx + 1}.jpg",
            num_examples=num_examples_per_class,
        )
    make_zip(root, "EuroSAT.zip", data_folder)


By using data_folder here, it is assumed that this folder and all of its contents should be included in the archive. By default, make_zip removes all files and folders it includes. Since the extra eurosat folder is not included, it is left as is. Thus, there are now an empty eurosat folder as well as the EuroSAT.zip file in the root directory.

Although not correct, that wouldn't be an issue if weren't for a quirk on Windows and macOS: pathlib treats file and folder names case-insensitive 🤯 This means,

vision/torchvision/prototype/datasets/utils/_resource.py

Lines 92 to 99 in e836b3d

stem = path.name.replace("".join(path.suffixes), "")

# In a first step, we check for a folder with the same stem as the raw file. If it exists, we use it since

# extracted files give the best I/O performance. Note that OnlineResource._extract() makes sure that an archive

# is always extracted in a folder with the corresponding file name.

folder_candidate = path.parent / stem

if folder_candidate.exists() and folder_candidate.is_dir():

return self._loader(folder_candidate)

is picking up the empty eurosat folder as a candidate for EuroSAT.zip although it should only look for an EuroSAT folder. Since we prefer extracted files over archives, it now loads the empty folder and thus all tests fail.

Windows and macOS: pathlib treats file and folder names case-insensitive

Is this a bug, or a documented "feature" ? Are we likely to hit more bugs in the future because of this?

This is not a bug nor documented, since this is part of system. Neither macOS nor Windows support case sensitive files.

IMO the only places where we might hit this is in the test suite if the mock data is setup wrong. A scenario like above cannot happen during normal operation unless a user manually manipulates the data folders.

NicolasHug · 2022-03-14T08:57:16Z

test/test_prototype_builtin_datasets.py

+        except StopIteration:
+            raise AssertionError("Unable to draw any sample.") from None


Can you explain how this relates to the fix?

It makes this case more expressive. Before you got an error message like Drawing a sample resulted in the error above while "above" is a plain StopIteration. If you are familiar with datapipes, you should be able to interpret this, but new contributors might not be. I've fixed this here, because we were hitting this exact case.

NicolasHug · 2022-03-14T08:58:17Z

test/builtin_dataset_mocks.py

-            name=cls,
-            file_name_fn=lambda idx: f"{cls}_{idx}.jpg",
+            name=category,
+            file_name_fn=lambda idx: f"{category}_{idx + 1}.jpg",


Can you explain how the idx + 1 part relates to the fix?

It doesn't, but it aligns the mock data more with real data. I just found this while looking into the real data to debug this issue and didn't want to have an extra PR for it.

NicolasHug · 2022-03-14T09:01:10Z

test/builtin_dataset_mocks.py

-            name=cls,
-            file_name_fn=lambda idx: f"{cls}_{idx}.jpg",
+            name=category,
+            file_name_fn=lambda idx: f"{category}_{idx + 1}.jpg",
            num_examples=num_examples_per_class,
        )
    make_zip(root, "EuroSAT.zip", data_folder)


Windows and macOS: pathlib treats file and folder names case-insensitive

Is this a bug, or a documented "feature" ? Are we likely to hit more bugs in the future because of this?

NicolasHug · 2022-03-14T09:01:38Z

test/builtin_dataset_mocks.py

@@ -1329,20 +1329,20 @@ def cub200(info, root, config):

 @register_mock
 def eurosat(info, root, config):
-    data_folder = pathlib.Path(root, "eurosat", "2750")
+    data_folder = root / "2750"


How come the linux tests were passing before with the extra eurosat folder?
I would assume that if we change the structure in the , this would have to be reflected somehow in the dataset code?

Since the prototype datasets never look at the absolute path, but only relative starting from the right

vision/torchvision/prototype/datasets/_builtin/eurosat.py

Line 38 in 27745e5

category = pathlib.Path(path).parent.name

adding more folders on the left side makes no difference.

NicolasHug

Thanks @pmeier

Summary: * [DEBUG] eurosat prototype on windows and macos * print paths * fix eurosat mock data setup * revert changes * minor cleanup Reviewed By: vmoens Differential Revision: D34878969 fbshipit-source-id: 6784c6119b65ec719683195e7d1ebbf22a9d1b2d

pytorch-bot bot added the ciflow/default label Mar 4, 2022

facebook-github-bot added the cla signed label Mar 4, 2022

pmeier force-pushed the eurosat-windows-macos branch from fa803fb to ed0e2a6 Compare March 4, 2022 07:30

[DEBUG] eurosat prototype on windows and macos

a5b4f2d

pmeier force-pushed the eurosat-windows-macos branch from ed0e2a6 to a5b4f2d Compare March 4, 2022 07:33

pmeier added 3 commits March 4, 2022 08:46

print paths

ac5a25f

fix eurosat mock data setup

6847414

revert changes

3b9b6e9

pmeier commented Mar 4, 2022

View reviewed changes

pmeier changed the title ~~[DEBUG] eurosat prototype on windows and macos~~ fix eurosat prototype mock data setup Mar 4, 2022

pmeier requested a review from NicolasHug March 4, 2022 09:05

pmeier added module: datasets module: tests prototype labels Mar 4, 2022

pmeier marked this pull request as ready for review March 4, 2022 09:05

minor cleanup

5d427fe

pmeier mentioned this pull request Mar 4, 2022

Run prototype datasets test also on Windows and macOS? #5550

Closed

NicolasHug reviewed Mar 14, 2022

View reviewed changes

NicolasHug approved these changes Mar 14, 2022

View reviewed changes

Merge branch 'main' into eurosat-windows-macos

3062c71

pmeier merged commit 7bb8186 into pytorch:main Mar 14, 2022

pmeier deleted the eurosat-windows-macos branch March 14, 2022 10:38

pmeier mentioned this pull request Mar 30, 2022

allow preprocessed mock data in prototype datasets tests #5706

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix eurosat prototype mock data setup #5549

fix eurosat prototype mock data setup #5549

Uh oh!

pmeier commented Mar 4, 2022 •

edited

Loading

Uh oh!

facebook-github-bot commented Mar 4, 2022 •

edited

Loading

Uh oh!

pmeier Mar 4, 2022

Uh oh!

pmeier Mar 4, 2022

Uh oh!

NicolasHug Mar 14, 2022

Uh oh!

pmeier Mar 14, 2022

Uh oh!

NicolasHug Mar 14, 2022

Uh oh!

pmeier Mar 14, 2022

Uh oh!

NicolasHug Mar 14, 2022

Uh oh!

pmeier Mar 14, 2022

Uh oh!

NicolasHug Mar 14, 2022

Uh oh!

NicolasHug Mar 14, 2022

Uh oh!

pmeier Mar 14, 2022

Uh oh!

NicolasHug left a comment

Uh oh!

Uh oh!

	stem = path.name.replace("".join(path.suffixes), "")

	# In a first step, we check for a folder with the same stem as the raw file. If it exists, we use it since
	# extracted files give the best I/O performance. Note that OnlineResource._extract() makes sure that an archive
	# is always extracted in a folder with the corresponding file name.
	folder_candidate = path.parent / stem
	if folder_candidate.exists() and folder_candidate.is_dir():
	return self._loader(folder_candidate)

		except StopIteration:
		raise AssertionError("Unable to draw any sample.") from None

fix eurosat prototype mock data setup #5549

fix eurosat prototype mock data setup #5549

Uh oh!

Conversation

pmeier commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmeier commented Mar 4, 2022 •

edited

Loading

facebook-github-bot commented Mar 4, 2022 •

edited

Loading