Replace asserts with exceptions #5587

jdsgomes · 2022-03-10T17:49:10Z

Changes most asserts with exceptions
adresses #5494

facebook-github-bot · 2022-03-10T17:49:18Z

💊 CI failures summary and remediations

As of commit d77739b (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.

…/vision into replace_asserts_with_exceptions

NicolasHug

Thanks @jdsgomes , I only took a brief look at the first few changes. I feel like the translation of assert X to if not X: raise... can feel a bit unnatural in some places, because there is often a more Pythonic way to write not X. I made some comments below. I won't push too hard for this so if you feel like it's not worth it, that's fine. LMK what you think

references/classification/transforms.py

references/optical_flow/transforms.py

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

NicolasHug

Thanks @jdsgomes , I checked all if ... and they look good. I made some comments regarding the use of RuntimeError vs ValueError. Looks like we're a bit inconsistent throughout the code base, but I think most of these should be ValueError. I also made some comments about some else: blocks that can potentially be remove to simplify the PR, LMK what you think

NicolasHug · 2022-03-15T08:38:44Z

references/classification/transforms.py

-        assert num_classes > 0, "Please provide a valid positive value for the num_classes."
-        assert alpha > 0, "Alpha param can't be zero."
+
+        if num_classes <= 1:


The original is equivalent to if num_classes <= 0: but I assume this is actually a fix?

This was an error, I don't think it is a bug so I will change it

NicolasHug · 2022-03-15T08:44:10Z

torchvision/datasets/kinetics.py

            self.split = "unknown"
-            assert not download, "Cannot download the videos using legacy_structure."
+            if download:
+                raise RuntimeError("Cannot download the videos using legacy_structure.")


I think this should be a ValueError rather than a RuntimeError

agree - thanks for spotting these inconsistencies

NicolasHug · 2022-03-15T08:46:07Z

test/test_backbone_utils.py

        )
        # Check must specify return nodes
-        with pytest.raises(AssertionError):
+        with pytest.raises(RuntimeError):


I wonder whether a ValueError makes more sense here, and almost everywhere else a RuntimeError is raised? Usually when there's an issue with user-provided input, it's either a TypeError or a ValueError.

Also, it helps a lot to use the match=expected_err_msg parameter of pytest.raises: we can make sure that the exception that gets raise is indeed the one we expect, and it's helpful to document the tests as well. We don't have to match the entire error message, sometimes just matching the relevant part is good enough. We could leave that part for a follow up though.

I was in doubt in this one. I was using a strict definition of ValueError: "Raised when an operation or function receives an argument that has the right type but an inappropriate value". And in this case each argument might have an appropriate value, but the combination of them is not appropriate. But I am ok with your interpretation as these are user inputs so still fall under ValueError.

As for the expected error messages I would leave it for another PR as it is outside the scope of this one.

torchvision/datasets/mnist.py

torchvision/models/shufflenetv2.py

torchvision/ops/boxes.py

torchvision/ops/poolers.py

NicolasHug · 2022-03-15T09:06:06Z

torchvision/ops/poolers.py

-        )
+    if scales is None or mapper is None:
+        raise ValueError("scales and mapper should not be None")
+    else:


same about else

torchvision/utils.py

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

datumbox

There are a few potential conflicts with multi-weights porting noted below. This is FYI.

Edit:
@jdsgomes With the exception of the first comment below, everything else will be removed by the porting. Shall I cherry pick the first in my branch and then undo the rest to avoid conflicts? No strong opinion if you want to keep them as you are likely to merge faster than I will.

datumbox · 2022-03-15T12:36:17Z

torchvision/models/detection/ssdlite.py

+    if len(out_channels) != len(anchor_generator.aspect_ratios):
+        raise RuntimeError(
+            f"The length of the output channels from the backbone {len(out_channels)} do not match the length of the anchor generator aspect ratios {len(anchor_generator.aspect_ratios)}"
+        )


That's going to conflict with the weight porting.

please se my comment about this conflicts. I reverted all others but will leave this one let me know if you agree with this approach

datumbox · 2022-03-15T12:37:35Z

torchvision/models/quantization/inception.py

    else:
-        assert pretrained in [True, False]
+        if pretrained not in [True, False]:
+            raise ValueError(f"For non quantized models, pretrained should be a bollean value instead of {pretrained}")


This one too.

datumbox · 2022-03-15T12:37:43Z

torchvision/models/quantization/mobilenetv2.py

    else:
-        assert pretrained in [True, False]
+        if pretrained not in [True, False]:
+            raise ValueError(f"For non quantized models, pretrained should be a bollean value instead of {pretrained}")


datumbox · 2022-03-15T12:37:49Z

torchvision/models/quantization/resnet.py

    else:
-        assert pretrained in [True, False]
+        if pretrained not in [True, False]:
+            raise ValueError(f"For non quantized models, pretrained should be a bollean value instead of {pretrained}")


datumbox · 2022-03-15T12:37:59Z

torchvision/models/quantization/shufflenetv2.py

    else:
-        assert pretrained in [True, False]
+        if pretrained not in [True, False]:
+            raise ValueError(f"For non quantized models, pretrained should be a bollean value instead of {pretrained}")


jdsgomes · 2022-03-15T13:03:35Z

@datumbox I will revert the others and keep the first one then. You can cherrypick it or resolve later if this lands first.

jdsgomes · 2022-03-15T15:12:00Z

thank you @NicolasHug for all the comments! I think i've addressed everything but let me know if it looks good!

NicolasHug

Thanks @jdsgomes, LGTM

I still see a few RuntimeError that could be ValueError, from #5587 (comment) I'm not sure whether you were planning to change them here in this PR, or later, or at all. Either way we are not consistent regarding this in the code-base so I will approve regardless. Thanks for the many changes!

NicolasHug · 2022-03-15T15:28:30Z

torchvision/models/feature_extraction.py

+    if not ((return_nodes is None) ^ (train_return_nodes is None)):
+        raise ValueError("If `train_return_nodes` and `eval_return_nodes` are specified, then both should be specified")


Should we fix that now, or do that in another PR?
Previous comment was #5587 (comment)

Sorry you are right, this should be done in this one. Done now. I will wait for the tests to pass and merge. Thanks for the heads up

github-actions · 2022-03-15T18:13:14Z

Hey @jdsgomes!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

Summary: * replace most asserts with exceptions * fix formating issues * fix linting and remove more asserts * fix regresion * fix regresion * fix bug * apply ufmt * apply ufmt * fix tests * fix format * fix None check * fix detection models tests * non scriptable any * add more checks for None values * fix retinanet test * fix retinanet test * Update references/classification/transforms.py * Update references/classification/transforms.py * Update references/optical_flow/transforms.py * Update references/optical_flow/transforms.py * Update references/optical_flow/transforms.py * make value checks more pythonic: * Update references/optical_flow/transforms.py * make value checks more pythonic * make more checks pythonic * fix bug * appy ufmt * fix tracing issues * fib typos * fix lint * remove unecessary f-strings * fix bug * Update torchvision/datasets/mnist.py * Update torchvision/datasets/mnist.py * Update torchvision/ops/boxes.py * Update torchvision/ops/poolers.py * Update torchvision/utils.py * address PR comments * Update torchvision/io/_video_opt.py * Update torchvision/models/detection/generalized_rcnn.py * Update torchvision/models/feature_extraction.py * Update torchvision/models/optical_flow/raft.py * address PR comments * addressing further pr comments * fix bug * remove unecessary else * apply ufmt * last pr comment * replace RuntimeErrors (Note: this ignores all push blocking failures!) Reviewed By: datumbox, YosuaMichael Differential Revision: D35216782 fbshipit-source-id: 5c0aeede7e114e3951fb6867a525162b706f5a60 Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

replace most asserts with exceptions

dfb2862

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

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

jdsgomes added 19 commits March 10, 2022 21:37

fix formating issues

40d0528

fix linting and remove more asserts

13bfd80

Merge branch 'main' into replace_asserts_with_exceptions

45ecd61

fix regresion

f522368

Merge branch 'replace_asserts_with_exceptions' of github.com:jdsgomes…

23bd022

…/vision into replace_asserts_with_exceptions

fix regresion

6a87e4d

fix bug

e179358

apply ufmt

30b1714

apply ufmt

488d2af

fix tests

38d2d01

fix format

7d42574

fix None check

dc6856b

fix detection models tests

2c56adc

non scriptable any

aebca6d

add more checks for None values

d54b582

Merge branch 'main' into replace_asserts_with_exceptions

36d2174

fix retinanet test

98c2702

Merge branch 'replace_asserts_with_exceptions' of github.com:jdsgomes…

bdab5f4

…/vision into replace_asserts_with_exceptions

fix retinanet test

4900653

jdsgomes changed the title ~~[WIP] replace most asserts with exceptions~~ Replace asserts with exceptions Mar 13, 2022

NicolasHug reviewed Mar 14, 2022

View reviewed changes

jdsgomes and others added 5 commits March 14, 2022 11:31

Update references/classification/transforms.py

d5ccbf1

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update references/classification/transforms.py

de2f4b7

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update references/optical_flow/transforms.py

275012a

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update references/optical_flow/transforms.py

fddd2ac

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update references/optical_flow/transforms.py

7e60b46

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

NicolasHug reviewed Mar 15, 2022

View reviewed changes

jdsgomes and others added 12 commits March 15, 2022 10:27

Merge branch 'main' into replace_asserts_with_exceptions

8dc76e2

Update torchvision/datasets/mnist.py

e68c1be

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/datasets/mnist.py

d92a0f9

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/ops/boxes.py

4a30fc9

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/ops/poolers.py

e4f214d

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/utils.py

9e9ca6d

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

address PR comments

b234e08

Update torchvision/io/_video_opt.py

1a45e1e

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/models/detection/generalized_rcnn.py

8437088

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/models/feature_extraction.py

cff417a

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Update torchvision/models/optical_flow/raft.py

1d9e3d3

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

address PR comments

ce06c29

datumbox reviewed Mar 15, 2022

View reviewed changes

jdsgomes added 5 commits March 15, 2022 13:27

addressing further pr comments

2b1870f

fix bug

851adb2

remove unecessary else

a915f1f

apply ufmt

f41e115

last pr comment

ee21d2e

NicolasHug approved these changes Mar 15, 2022

View reviewed changes

jdsgomes added 2 commits March 15, 2022 16:35

replace RuntimeErrors

d000238

Merge branch 'main' into replace_asserts_with_exceptions

d77739b

jdsgomes merged commit 289fce2 into pytorch:main Mar 15, 2022

jdsgomes added the enhancement label Mar 15, 2022

jdsgomes mentioned this pull request Mar 17, 2022

replace remaining asserts #5636

Closed

datumbox mentioned this pull request May 25, 2022

Refactor swin transfomer so later we can reuse component for 3d version #6088

Merged

		if not ((return_nodes is None) ^ (train_return_nodes is None)):
		raise ValueError("If `train_return_nodes` and `eval_return_nodes` are specified, then both should be specified")

Replace asserts with exceptions #5587

Replace asserts with exceptions #5587

Uh oh!

Conversation

jdsgomes commented Mar 10, 2022

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

datumbox left a comment • 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!

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!

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

Uh oh!

Uh oh!

jdsgomes commented Mar 15, 2022

Uh oh!

NicolasHug 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!

github-actions bot commented Mar 15, 2022

Uh oh!

Uh oh!

facebook-github-bot commented Mar 10, 2022 •

edited

Loading

datumbox left a comment •

edited

Loading

jdsgomes commented Mar 15, 2022 •

edited

Loading