Improve error handling in make_dataset by pmeier · Pull Request #3496 · pytorch/vision

pmeier · 2021-03-03T15:50:19Z

Closes #3495, fixes #2903.

pmeier · 2021-03-03T15:52:29Z

test/test_datasets.py

    def test_imagefolder_empty(self):
        with get_tmp_dir() as root:
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(FileNotFoundError):


This BC breaking if someone relies on DatasetFolder or ImageFolder to raise a RuntimeError if no samples are found. Since the error type was never documented anywhere, I think this is fine.

I'd say this is fine but might be worth checking if it breaks something in FBCODE.

pmeier · 2021-03-03T15:55:26Z

torchvision/datasets/folder.py

+    if class_to_idx is None:
+        _, class_to_idx = find_classes(directory)
+    elif not class_to_idx:
+        raise ValueError("'class_to_index' must have at least one entry to collect any samples.")


This is BC breaking, but in a "right" way. Before make_dataset silently returned an empty list in case class_to_idx was empty. IMO it is a reasonable assumption that no user calls make_dataset without having at least a single class. If you disagree with this assumption, we can get BC back by return [] here.

I think this makes sense, especially for the folder dataset.
Is there any other dataset that inherits this other than videodatasets?

Is there any other dataset that inherits this other than videodatasets?

Nothing built-in.

Sounds good to me then

@pmeier DatasetFolder.make_dataset is public and the docstring is:

class_to_idx (Optional[Dict[str, int]]): Dictionary mapping class name to class index. If omitted, is generated by :func:`find_classes`

The problem is that users can override DatasetFolder.find_classes too, so there's a conflict with the find_classes() function: DatasetFolder.find_classes will rely on the function rather than on the method.

Should we make class_to_idx a mandatory parameter in DatasetFolder.make_dataset to avoid any potential issue?

To better explain myself: imagine a scenario where someone has a class MyCoolNewDataset(DatasetFolder) and they override MyCoolNewDataset.find_classes with a custom class_to_idx logic.

If they call MyCoolNewDataset.make_dataset while passing None to the class_to_idx parameter, what will be used is the class_to_idx logic from the find_classes function, instead of using the logic from the find_classes method - which is different since they overrode it.

Does that make sense?

To avoid such issues I'm suggesting to force the user to pass class_to_idx in DatasetFolder.make_dataset, or more accurately to raise an error if None is passed in DatasetFolder.make_dataset

codecov · 2021-03-03T16:23:44Z

Codecov Report

Merging #3496 (87f151a) into master (19ad0bb) will increase coverage by 0.16%.
The diff coverage is 68.96%.

❗ Current head 87f151a differs from pull request most recent head 9fa4a7a. Consider uploading reports for the commit 9fa4a7a to get more accurate results

@@            Coverage Diff             @@
##           master    #3496      +/-   ##
==========================================
+ Coverage   78.83%   78.99%   +0.16%     
==========================================
  Files         105      105              
  Lines        9816     9793      -23     
  Branches     1581     1573       -8     
==========================================
- Hits         7738     7736       -2     
+ Misses       1588     1573      -15     
+ Partials      490      484       -6

Impacted Files	Coverage Δ
torchvision/datasets/folder.py	`78.57% <60.86%> (-7.48%)`	⬇️
torchvision/datasets/hmdb51.py	`94.82% <100.00%> (-0.18%)`	⬇️
torchvision/datasets/kinetics.py	`95.65% <100.00%> (-0.35%)`	⬇️
torchvision/datasets/ucf101.py	`93.33% <100.00%> (-0.29%)`	⬇️
torchvision/utils.py	`59.57% <0.00%> (-2.53%)`	⬇️
torchvision/transforms/transforms.py	`84.30% <0.00%> (ø)`
torchvision/datasets/voc.py	`94.50% <0.00%> (+0.06%)`	⬆️
torchvision/transforms/functional_tensor.py	`79.84% <0.00%> (+0.56%)`	⬆️
torchvision/models/detection/anchor_utils.py	`94.66% <0.00%> (+1.33%)`	⬆️
torchvision/ops/deform_conv.py	`72.30% <0.00%> (+3.07%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19ad0bb...9fa4a7a. Read the comment docs.

bjuncek

Looks good to me. Should we add a test for make-dataset with no classes as well just to be on the safe side?

Also, I've added a comment, but are there any other datasets that rely on folder dataset?

bjuncek · 2021-03-09T10:50:06Z

torchvision/datasets/folder.py

+    if class_to_idx is None:
+        _, class_to_idx = find_classes(directory)
+    elif not class_to_idx:
+        raise ValueError("'class_to_index' must have at least one entry to collect any samples.")


I think this makes sense, especially for the folder dataset.
Is there any other dataset that inherits this other than videodatasets?

bjuncek · 2021-03-09T10:52:08Z

test/test_datasets.py

    def test_imagefolder_empty(self):
        with get_tmp_dir() as root:
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(FileNotFoundError):


I'd say this is fine but might be worth checking if it breaks something in FBCODE.

bjuncek

Most of my questions answered. LGTM

fmassa

Thanks!

fmassa · 2021-03-24T10:13:36Z

torchvision/datasets/folder.py

+    @staticmethod
+    def _find_classes(dir: str) -> Tuple[List[str], Dict[str, int]]:


nit: I wouldn't have made this a staticmethod, as other instantiations of this dataset could rely on self for generating a custom set of classes and class ids

But this is not really important, so let's move forward with this PR and get this merged

Summary: * factor out find_classes * use find_classes in video datasets * adapt old tests Reviewed By: fmassa Differential Revision: D27433918 fbshipit-source-id: 60d8da2f222a19e0757197f5d38b6a9cce7694a8

pmeier added 3 commits March 3, 2021 16:47

factor out find_classes

6a16b81

use find_classes in video datasets

dbb9d9a

adapt old tests

bacf2a6

facebook-github-bot added the cla signed label Mar 3, 2021

pmeier commented Mar 3, 2021

View reviewed changes

pmeier mentioned this pull request Mar 3, 2021

add tests for (Dataset|Image)Folder #3477

Merged

bjuncek reviewed Mar 9, 2021

View reviewed changes

bjuncek approved these changes Mar 11, 2021

View reviewed changes

pmeier requested a review from fmassa March 16, 2021 13:34

pmeier added 2 commits March 16, 2021 14:34

Merge branch 'master' into improve-make-dataset

87f151a

Merge branch 'master' into improve-make-dataset

9fa4a7a

fmassa approved these changes Mar 24, 2021

View reviewed changes

fmassa merged commit 0818c68 into pytorch:master Mar 24, 2021

pmeier deleted the improve-make-dataset branch March 24, 2021 11:59

NicolasHug added improvement module: datasets labels Mar 31, 2021

pmeier mentioned this pull request Apr 1, 2021

Select subset of classes to sample from #3627

Closed

NicolasHug mentioned this pull request Apr 1, 2021

Make DatasetFolder.find_classes public #3628

Merged

NicolasHug mentioned this pull request Apr 26, 2021

Prevent potential bug in DatasetFolder.make_dataset #3733

Merged

datumbox added enhancement and removed improvement labels Jun 1, 2021

chr5tphr mentioned this pull request Jun 28, 2021

feed_forward is broken for torchvision 0.10.0 chr5tphr/zennit#37

Closed

		@staticmethod
		def _find_classes(dir: str) -> Tuple[List[str], Dict[str, int]]:

Conversation

pmeier commented Mar 3, 2021

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!

codecov bot commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bjuncek left a comment

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!

bjuncek left a comment

Choose a reason for hiding this comment

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

fmassa Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Mar 3, 2021 •

edited

Loading

fmassa Mar 24, 2021 •

edited

Loading