Add --backend and --use-v2 support to detection refs #7732

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

@@ -30,42 +33,42 @@ def __init__(
        backend="pil",
        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

@@ -293,11 +293,13 @@ def __init__(
        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

@@ -126,10 +126,6 @@ def _has_valid_annotation(anno):
            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

@@ -97,7 +97,7 @@ def evaluate(model, data_loader, device):
        outputs = [{k: v.to(cpu_device) for k, v in t.items()} for t in outputs]
        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

@@ -126,10 +126,6 @@ def _has_valid_annotation(anno):
            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

@@ -196,12 +192,15 @@ def convert_to_coco_api(ds):


 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

@@ -26,7 +26,7 @@ def train_one_epoch(model, optimizer, data_loader, device, epoch, print_freq, sc

    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

@@ -177,8 +185,8 @@ def main(args):
    # Data loading code
    print("Loading data")

-    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

@@ -293,11 +293,13 @@ def __init__(
        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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --backend and --use-v2 support to detection refs #7732

Add --backend and --use-v2 support to detection refs #7732

NicolasHug commented Jul 11, 2023 •

edited

Loading

pytorch-bot bot commented Jul 11, 2023 •

edited

Loading

NicolasHug Jul 11, 2023

NicolasHug Jul 11, 2023

pmeier Jul 13, 2023

NicolasHug Jul 11, 2023

NicolasHug Jul 11, 2023

pmeier Jul 13, 2023

NicolasHug Jul 11, 2023

pmeier left a comment

pmeier Jul 13, 2023

pmeier Jul 13, 2023

NicolasHug Jul 13, 2023

pmeier Jul 13, 2023

pmeier Jul 13, 2023

pmeier Jul 13, 2023

NicolasHug Jul 13, 2023

pmeier Jul 13, 2023

pmeier Jul 13, 2023

pmeier commented Jul 13, 2023

NicolasHug commented Jul 13, 2023

pmeier commented Jul 13, 2023

NicolasHug commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

		@@ -196,12 +192,15 @@ def convert_to_coco_api(ds):


		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),

Add --backend and --use-v2 support to detection refs #7732

Add --backend and --use-v2 support to detection refs #7732

Conversation

NicolasHug commented Jul 11, 2023 • edited Loading

pytorch-bot bot commented Jul 11, 2023 • edited Loading

🔗 Helpful Links

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

❌ 18 New Failures

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier commented Jul 13, 2023

NicolasHug commented Jul 13, 2023

pmeier commented Jul 13, 2023

NicolasHug commented Jul 13, 2023

github-actions bot commented Jul 13, 2023

NicolasHug commented Jul 11, 2023 •

edited

Loading

pytorch-bot bot commented Jul 11, 2023 •

edited

Loading