Add --backend and --use-v2 support to detection refs by NicolasHug · Pull Request #7732 · pytorch/vision

NicolasHug · 2023-07-11T11:32:59Z

This probably won't be an easy review as there's a bunch of renaming, sorry.

There are a few things I've intentionally left out of this PR:

presets support (those used with --test-only)
mask-rcnn support

It's also fairly likely that I broke some stuff in the process or that not everything works 100% correctly straight away. It's very hard to test everything considering we have no unit test, and the cross-product of all combinations is high. It shouldn't be too critical though, as we'll be addressing any outstanding issue in the near future as we run more training jobs.

pytorch-bot · 2023-07-11T11:33:02Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7732

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 18 New Failures

As of commit 56dc431:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

NicolasHug · 2023-07-11T11:33:42Z

references/classification/presets.py

        use_v2=False,
    ):
-        module = get_module(use_v2)
+        T = get_module(use_v2)


I just did s/module/T/ in the file to make it consistent with the detection one

NicolasHug · 2023-07-11T11:34:56Z

references/detection/transforms.py

        target_size: Tuple[int, int],
        scale_range: Tuple[float, float] = (0.1, 2.0),
        interpolation: InterpolationMode = InterpolationMode.BILINEAR,
+        antialias=True,


Had to add antialias support because it'd be False otherwise by default for tensors. There's no BC requirements so we could just hard-code antialias=True below in the calls to resize() instead of adding a parameter here, but it doesn't change much. LMK what you prefer.

IDC. Unless there is some other opinion, let's keep it the way it is.

NicolasHug · 2023-07-11T13:32:30Z

references/detection/coco_utils.py

+            t.append(transforms)
+        transforms = T.Compose(t)
+
+        dataset = CocoDetection(img_folder, ann_file, transforms=transforms)


I wonder if we could get rid of this custom CocoDetection dataset here. Ideally we would always call wrap_dataset_for_transforms_v2 and just "unwrap" the datapoints classes into pure-tensors etc...? But we can't use it without silencing the V2 warning first :/

Not sure what to do to clean that up.

NicolasHug · 2023-07-11T13:34:05Z

references/detection/coco_utils.py

            return True
        return False

-    if not isinstance(dataset, torchvision.datasets.CocoDetection):


Instead of removing this (seemingly useless check) I could just add the same workaround as elsewhere i.e. add

of isinstance( getattr(dataset, "_dataset", None), torchvision.datasets.CocoDetection ):

We still have #7239. Maybe we should go at it again?

NicolasHug · 2023-07-11T13:35:10Z

references/detection/engine.py

        model_time = time.time() - model_time

-        res = {target["image_id"].item(): output for target, output in zip(targets, outputs)}
+        res = {target["image_id"]: output for target, output in zip(targets, outputs)}


This is for consistency with the V2 wrapper which leaves image_id as an int. In our references we used to manually wrap it into a tensor (why, IDK), and I removed that as well below in coco_utils

references/detection/presets.py

pmeier

LGTM, thanks Nicolas! I'm ok with not testing it on all configurations right now, but to make sure: you have tested it on least one and it works, correct?

pmeier · 2023-07-13T08:13:36Z

references/detection/coco_utils.py

            return True
        return False

-    if not isinstance(dataset, torchvision.datasets.CocoDetection):


We still have #7239. Maybe we should go at it again?

pmeier · 2023-07-13T08:14:30Z

references/detection/coco_utils.py



 def get_coco_api_from_dataset(dataset):
+    # FIXME: This is... awful?


Yeah. Happy for you to address it here, but not required.

I would if I knew what to do lol. (I'm gonna leave this out for now I think)

pmeier · 2023-07-13T08:24:21Z

references/detection/engine.py

    for images, targets in metric_logger.log_every(data_loader, print_freq, header):
        images = list(image.to(device) for image in images)
-        targets = [{k: v.to(device) for k, v in t.items()} for t in targets]
+        targets = [{k: v.to(device) if isinstance(v, torch.Tensor) else v for k, v in t.items()} for t in targets]


This is for the image ID, right?

pmeier · 2023-07-13T08:28:29Z

references/detection/presets.py

+                # TODO: FixedSizeCrop below doesn't work on tensors!
+                reference_transforms.FixedSizeCrop(size=(1024, 1024), fill=mean),


In v2 we have RandomCrop that does what FixedSizedCrop does minus the clamping and sanitizing bounding boxes.

pmeier · 2023-07-13T08:29:37Z

references/detection/presets.py

+        if use_v2:
+            transforms += [
+                T.ConvertBoundingBoxFormat(datapoints.BoundingBoxFormat.XYXY),
+                T.SanitizeBoundingBox(),


Do we also need ClampBoundingBox here?

I don't think so since we established that all transforms should clamp already (those that need to, at least)?

pmeier · 2023-07-13T08:32:12Z

references/detection/train.py


-    dataset, num_classes = get_dataset(args.dataset, "train", get_transform(True, args), args.data_path)
-    dataset_test, _ = get_dataset(args.dataset, "val", get_transform(False, args), args.data_path)
+    dataset, num_classes = get_dataset(args.dataset, "train", get_transform(True, args), args.data_path, args.use_v2)


Not required here, but can we maybe use keyword args here? The call is really hard to parse.

pmeier · 2023-07-13T08:33:48Z

references/detection/transforms.py

        target_size: Tuple[int, int],
        scale_range: Tuple[float, float] = (0.1, 2.0),
        interpolation: InterpolationMode = InterpolationMode.BILINEAR,
+        antialias=True,


IDC. Unless there is some other opinion, let's keep it the way it is.

pmeier · 2023-07-13T08:37:40Z

Test failures are real. Maybe related to antialias? I'll have a look.

NicolasHug · 2023-07-13T08:40:49Z

Thanks for the review!

Which failure are relevant? We have no tests for the reference folder and this PR doesn't touch any file outside of it.

pmeier · 2023-07-13T08:43:21Z

We have no tests for the reference folder and this PR doesn't touch any file outside of it.

We do. We have v2 consistency tests that checks the transforms that we have added to our package against the stuff that we have in our references:

vision/test/test_transforms_v2_consistency.py

Line 1083 in 08c9938

class TestRefDetTransforms:
vision/test/test_transforms_v2_consistency.py

Line 1188 in 08c9938

class TestRefSegTransforms:

I've send a fix in 72da655.

NicolasHug · 2023-07-13T14:46:33Z

but to make sure: you have tested it on least one and it works, correct?

Yeah, I tested it on a few combinations, but I wouldn't be surprised if there's a few edge-cases I missed. We'll find out soon enough. Thanks for the review!

github-actions · 2023-07-13T14:47:40Z

Hey @NicolasHug!

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: Co-authored-by: Philip Meier <github.pmeier@posteo.de> Reviewed By: matteobettini Differential Revision: D48642258 fbshipit-source-id: 7d99fb2ea5effde79ee59d259f902fcf145ae64c

Add --backend support to detection refs

6443e6a

facebook-github-bot added the cla signed label Jul 11, 2023

NicolasHug commented Jul 11, 2023

View reviewed changes

NicolasHug added 3 commits July 11, 2023 13:28

Add --use-v2 support to detection refs

d10dd56

remove comment

f956d01

uuguuguguguuuu

06ab751

NicolasHug commented Jul 11, 2023

View reviewed changes

NicolasHug marked this pull request as ready for review July 11, 2023 13:38

NicolasHug requested review from pmeier and vfdev-5 July 11, 2023 13:38

remove TODO

c6913d2

NicolasHug commented Jul 11, 2023

View reviewed changes

references/detection/presets.py Show resolved Hide resolved

pmeier approved these changes Jul 13, 2023

View reviewed changes

use antialias=True and consistency test

72da655

NicolasHug mentioned this pull request Jul 13, 2023

add support for instance checks on dataset wrappers #7239

Merged

clean up parameter passing

56dc431

NicolasHug merged commit bb3aae7 into pytorch:main Jul 13, 2023

vfdev-5 mentioned this pull request Aug 22, 2023

Update coco_utils.py #7869

Merged



		def get_coco_api_from_dataset(dataset):
		# FIXME: This is... awful?

		# TODO: FixedSizeCrop below doesn't work on tensors!
		reference_transforms.FixedSizeCrop(size=(1024, 1024), fill=mean),

Conversation

NicolasHug commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7732

❌ 18 New Failures

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!

Uh oh!

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

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!

pmeier commented Jul 13, 2023

Uh oh!

NicolasHug commented Jul 13, 2023

Uh oh!

pmeier commented Jul 13, 2023

Uh oh!

NicolasHug commented Jul 13, 2023

Uh oh!

github-actions bot commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NicolasHug commented Jul 11, 2023 •

edited

Loading

pytorch-bot bot commented Jul 11, 2023 •

edited

Loading