Normalize, LinearTransformation are scriptable #2645

vfdev-5 · 2020-09-04T10:21:42Z

Description:

Normalize, LinearTransformation are scriptable, support batch tensors and cuda tensors
removed object from class definition
updated docs

OLD

Description:

Compose, RandomApply, Normalize can be jit scripted
all transforms are now derived from torch.nn.Module
Checked on
- references/classification
- references/similarity
- pytorch/examples
  - dcgan
  - fast neural style
  - super-resolution
  - imagenet
- swav

TODO:

Update docs and put a note about the change and possible problems if encountered...

We need an extensive checking on possible data aug pipelines to stress the code. Any help with that and reporting bugs here is appreciated.

cc @pmeier @fmassa

- Compose, RandomApply, Normalize can be jit scripted

pmeier

I have no other pipeline to test this against. Thus, my comments are only based on this PR.

torchvision/transforms/transforms.py

pmeier · 2020-09-07T13:41:29Z

torchvision/transforms/transforms.py

+                    t = Lambda(t)
+                new_transforms.append(t)
+
+            self.transforms = torch.nn.ModuleList(new_transforms)


What is the motivation for using a ModuleList here? Given that _forward_impl simply passes the input sequentially through all transforms, would a Sequential a better fit?

I think the reason is that in the case of an exception, you'll still have a list of transforms, so you will need perform the forward implementation yourself

Here is another point for using ModuleList instead of Sequential. Previously, Compose.transforms is of the same type as input transforms: tuple, list. User can create transforms as a compose and then append/extend etc its content.
However, in those cases, we can not simply wrap the input transforms by Lambda...
=> Maybe, we can subclass nn.ModuleList here and automatically perform the same wrapping by Lambda as in the constructor but for append/extend and other methods ...

vfdev-5 · 2020-09-07T14:04:12Z

torchvision/transforms/transforms.py

+            new_transforms = []
+            for t in transforms:
+                if callable(t) and not isinstance(t, torch.nn.Module):
+                    t = Lambda(t)


An issue here can be the following. For example, user would like to configure during the training some options of its custom transform and if it is wrapped by Lambda then previous code wont work...

transforms = Compose([ ... CustomTransform(some_config), # at index 5 ... ]) # assert hasattr(CustomTransform, "update_config") # assert isinstance(transforms[5], Lambda) for epoch in range(100): if epoch == 25: transforms[5].update_config(new_config)

As suggested by Philip, that we can "either enables attribute access to the underlying object or at least gives a meaningful error message".

…upport-2641

- added getattr to Lambda and tests - updated code and docs of Compose - added failing test with append/extend on Composed.transforms

fmassa · 2020-09-09T10:12:24Z

torchvision/transforms/transforms.py

@@ -443,7 +467,7 @@ def __repr__(self):
 class RandomOrder(RandomTransforms):
    """Apply a list of transformations in a random order
    """
-    def __call__(self, img):
+    def forward(self, img):
        order = list(range(len(self.transforms)))
        random.shuffle(order)


nit and not for this PR: this doesn't support torchscript

RandomTransforms becomes nn.Module, shouldn't we use forward instead of __call__ ?

Yes, we should. I was just pointing out that this is not torchscriptable

fmassa · 2020-09-09T10:13:25Z

test/test_transforms_tensor.py

+            ], p=0.3),
+            T.Compose([
+                T.RandomResizedCrop(15),
+                T.RandomApply([


nit: We are missing tests for RandomOrder

Well, I can add a basic test for that as RandomOrder is not scriptable

fmassa · 2020-09-09T10:14:38Z

torchvision/transforms/transforms.py

+        except TypeError:
+            self.transforms = transforms
+
+    def _forward_impl(self, img: Tensor) -> Tensor:
        for t in self.transforms:
            img = t(img)
        return img


Why not put the implementation inside forward?

fmassa · 2020-09-09T10:17:08Z

torchvision/transforms/transforms.py

+                    t = Lambda(t)
+                new_transforms.append(t)
+
+            self.transforms = torch.nn.ModuleList(new_transforms)


I think the reason is that in the case of an exception, you'll still have a list of transforms, so you will need perform the forward implementation yourself

torchvision/transforms/transforms.py

fmassa · 2020-09-16T08:10:52Z

torchvision/transforms/transforms.py

    """Apply a user-defined lambda as a transform.

+    .. Note::
+        This class exposes ``lambd`` attributes as its own attributes:


Question: do we want to mention it in the doc? Does this make it less of an implementation detail?

Maybe we can rephrase to say that custom ops wrapped by Lambda can behave similarly to original custom op and we do not have to use op.lambd to access attributes.

torchvision/transforms/transforms.py

fmassa · 2020-09-16T08:14:09Z

torchvision/transforms/transforms.py

-        for t in self.transforms:
-            img = t(img)
-        return img
+        return self._forward_impl(img)


Now I see why you implemented Compose with _forward_impl. I wonder if this is really necessary, given that we only save two lines but it adds additional complexity to the user to read the function?

Personnally, I'd prefer to refactor... If you think that it is better for transparency reasons to put explicit code, I'm fine with that too.

fmassa · 2020-09-16T08:15:51Z

test/test_transforms.py

+        ])
+
+        t.transforms.append(transforms.ToTensor())
+        t.transforms.append(lambda x: x + 2)  # THIS DOES NOT WORK


Can you explain what this doesn't work?

This fails due to the fact that lambda x: x + 2 is not nn.Module. Here we modify constructed Compose with custom ops that are not wrapped by Lambda.
See also the discussion above.

fmassa · 2020-09-16T08:24:18Z

test/test_transforms.py

+        t = MyTransform(10)
+        t_lambda = transforms.Lambda(t)
+        self.assertEqual(t_lambda.s, t.s)
+        t_lambda.s = 5
+        self.assertEqual(t_lambda.s, t.s)
+        self.assertEqual(t_lambda(10), t(10))


Hum, now I think I see why you override __getattr__. This wasn't the case before, I suppose this is to try to keep BC with Compose, if the user wants to access those fields after we wrapped they non-nn module in a Lambda?

My first thought would be to avoid this type of magic as it could be confusing to the user, but I'm not sure I see a better solution for this yet, apart from encouraging all new use-cases to use nn.Sequential, and letting Compose be not recommended module to use.

Thoughts?

Yes, the reason to override __getattr__ and __setattr__ is to be BC with Compose. More details above.

apart from encouraging all new use-cases to use nn.Sequential, and letting Compose be not recommended module to use.

I'm not sure that this can be a solution for custom ops where users can call for example opencv to transform things...

vfdev-5 · 2020-09-18T14:51:48Z

According to the discussion with @fmassa :

Let's keep Compose as is => no BC-break
Update its doc to say about using nn.Sequential for torch.jit.script
Remove __get/setattr__ from Lambda

…upport-2641

codecov · 2020-09-22T14:14:08Z

Codecov Report

Merging #2645 into master will increase coverage by 0.02%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##           master    #2645      +/-   ##
==========================================
+ Coverage   72.79%   72.82%   +0.02%     
==========================================
  Files          95       95              
  Lines        8203     8212       +9     
  Branches     1280     1283       +3     
==========================================
+ Hits         5971     5980       +9     
+ Misses       1841     1838       -3     
- Partials      391      394       +3

Impacted Files	Coverage Δ
torchvision/transforms/functional.py	`82.13% <42.85%> (ø)`
torchvision/transforms/transforms.py	`80.89% <70.37%> (+0.30%)`	⬆️

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 8dfcff7...5fbdbbd. Read the comment docs.

fmassa

Thanks a lot!

* [WIP] All transforms are now derived from torch.nn.Module - Compose, RandomApply, Normalize can be jit scripted * Fixed flake8 * Updated code and docs - added getattr to Lambda and tests - updated code and docs of Compose - added failing test with append/extend on Composed.transforms * Fixed flake8 * Updated code, tests and docs

vfdev-5 added 2 commits September 4, 2020 12:18

[WIP] All transforms are now derived from torch.nn.Module

fc07314

- Compose, RandomApply, Normalize can be jit scripted

Fixed flake8

185e85b

pmeier reviewed Sep 7, 2020

View reviewed changes

vfdev-5 commented Sep 7, 2020

View reviewed changes

Merge branch 'master' of github.com:pytorch/vision into vfdev-5/jit-s…

e7fa382

…upport-2641

vfdev-5 mentioned this pull request Sep 14, 2020

Add float support to ColorJitter #2672

Open

vfdev-5 added 2 commits September 14, 2020 15:50

Updated code and docs

a9b47ec

- added getattr to Lambda and tests - updated code and docs of Compose - added failing test with append/extend on Composed.transforms

Fixed flake8

a81f84f

fmassa reviewed Sep 16, 2020

View reviewed changes

vfdev-5 added 2 commits September 22, 2020 09:04

Merge branch 'master' of github.com:pytorch/vision into vfdev-5/jit-s…

3fa9ccf

…upport-2641

Updated code, tests and docs

186948f

vfdev-5 changed the title ~~[WIP][BC-breaking] All transforms are now derived from torch.nn.Module (2641)~~ All transforms are now derived from torch.nn.Module (2641) Sep 22, 2020

Merge branch 'master' into vfdev-5/jit-support-2641

5fbdbbd

vfdev-5 changed the title ~~All transforms are now derived from torch.nn.Module (2641)~~ Normalize, LinearTransformation are scriptable (2641) Sep 22, 2020

vfdev-5 changed the title ~~Normalize, LinearTransformation are scriptable (2641)~~ Normalize, LinearTransformation are scriptable Sep 22, 2020

vfdev-5 requested a review from fmassa September 22, 2020 14:37

fmassa approved these changes Sep 23, 2020

View reviewed changes

fmassa merged commit 1b41525 into pytorch:master Sep 23, 2020

fmassa mentioned this pull request May 6, 2021

Could T.Lambda be nn.Module? #3784

Open

vfdev-5 mentioned this pull request Aug 11, 2021

WIP Allowing for grouped (random) transformations of tensors/tensor-like objects [proof-of-concept] #4267

Closed

vfdev-5 mentioned this pull request Jan 21, 2022

[feature request] Derive all transforms classes from nn.Module? #5236

Closed

vfdev-5 deleted the vfdev-5/jit-support-2641 branch January 21, 2022 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize, LinearTransformation are scriptable #2645

Normalize, LinearTransformation are scriptable #2645

vfdev-5 commented Sep 4, 2020 •

edited

Loading

pmeier left a comment

pmeier Sep 7, 2020

fmassa Sep 9, 2020

vfdev-5 Sep 14, 2020 •

edited

Loading

vfdev-5 Sep 7, 2020

fmassa Sep 9, 2020

vfdev-5 Sep 16, 2020

fmassa Sep 18, 2020

fmassa Sep 9, 2020

vfdev-5 Sep 16, 2020

fmassa Sep 9, 2020

fmassa Sep 9, 2020

fmassa Sep 16, 2020

vfdev-5 Sep 16, 2020

fmassa Sep 16, 2020

vfdev-5 Sep 16, 2020

fmassa Sep 16, 2020

vfdev-5 Sep 16, 2020

fmassa Sep 16, 2020

vfdev-5 Sep 16, 2020 •

edited

Loading

vfdev-5 commented Sep 18, 2020

codecov bot commented Sep 22, 2020 •

edited

Loading

fmassa left a comment

Normalize, LinearTransformation are scriptable #2645

Normalize, LinearTransformation are scriptable #2645

Conversation

vfdev-5 commented Sep 4, 2020 • edited Loading

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

vfdev-5 Sep 14, 2020 • edited Loading

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

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

vfdev-5 Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

vfdev-5 commented Sep 18, 2020

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

fmassa left a comment

Choose a reason for hiding this comment

vfdev-5 commented Sep 4, 2020 •

edited

Loading

vfdev-5 Sep 14, 2020 •

edited

Loading

vfdev-5 Sep 16, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading