[testing] Support input samples where `self` is broadcasted. #53014

kshitij12345 · 2021-03-01T11:07:39Z

Fixes #50747

Reference #50006

facebook-github-bot · 2021-03-01T11:07:50Z

💊 CI failures summary and remediations

As of commit 64883fa (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-scanned failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm4.1-py3.6

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

kshitij12345 · 2021-03-04T09:41:30Z

test/test_ops.py

            self.skipTest(f"Skipped! {op.name} does not support dtype {str(dtype)}")

+        def is_inplace(variant):
+            return variant.__name__.endswith('_')


Is there a better way to check this?

Or should we trickle variant type and variant from the function that calls this helper?

pytorch/test/test_ops.py

Line 141 in c4c77e2

self._grad_test_helper(device, dtype, op, self._get_safe_inplace(op.get_inplace()))

pytorch/test/test_ops.py

Line 161 in c4c77e2

self._gradgrad_test_helper(device, dtype, op, self._get_safe_inplace(op.get_inplace()))

The correct way to check this is by testing if the variant is the inplace variant acquired from the OpInfo.

Unfortunately that won't work as the op.get_inplace() is wrapped by _get_safe_inplace.

pytorch/test/test_ops.py

Lines 68 to 73 in c4c77e2

def _get_safe_inplace(self, inplace_variant):

@wraps(inplace_variant)

def _fn(t, *args, **kwargs):

return inplace_variant(t.clone(), *args, **kwargs)

return _fn

Can you access it through the dunder wrapped attribute?

foo.__wrapped__ is op.get_inplace()

(if foo may or may not be wrapped you'll need to check for the attr first, of course)

Yup it works with that. Thanks!!

def is_inplace(variant): if hasattr(variant, "__wrapped__"): return variant.__wrapped__ is op.get_inplace() return variant is op.get_inplace()

mruberry · 2021-03-04T09:50:26Z

torch/testing/_internal/common_methods_invocations.py

+    __slots__ = ['input', 'args', 'kwargs', 'output_process_fn_grad', 'broadcasts_self']

-    def __init__(self, input, *, args=tuple(), kwargs=None, output_process_fn_grad=None):
+    def __init__(self, input, *, args=tuple(), kwargs=None, output_process_fn_grad=None, broadcasts_self=False):


This approach is reasonable but I still think we should filter sample_inputs() by whether the samples will be used for inplace computations or not. However, let me get a few other opinions on this. Maybe this is a better approach.

Sorry I neglected to update this, @kshitij12345. General consensus is that it'd be preferable to filter sample_inputs() by what's inplace for now. We may want to revise that decision later, however.

IIUC,
We will have to update the signature for every sample_inputs_* with

sample_inputs_*(op_info, device, dtype, requires_grad, is_inplace_variant)

?

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 255 to 261 in afe339d

def sample_inputs(self, device, dtype, requires_grad=False):

"""Returns an iterable of SampleInputs.

These samples should be sufficient to test the function works correctly

with autograd, TorchScript, etc.

"""

return self.sample_inputs_func(self, device, dtype, requires_grad)

And update sample_inputs to

def sample_inputs(self, device, dtype, requires_grad=False, is_inplace_variant=False):

We could do that (and make is_inplace_variant kwarg-only). That would be the most discoverable thing.

But we don't have to. The operations for testing the inplace variant can request sample inputs by passing the for_inplace_variant (we can work on the name) kwarg like this:

try: samples = op.sample_inputs(..., for_inplace_variant=True) except TypeError as te: samples = op.sample_inputs(...)

Then functions which support this option can implement it as:

def sample_inputs_foo(..., *, for_inplace_variant=False)

A nicer (but more disruptive) solution would be to make sample_inputs take **kwargs, and then the functions that want to use "for_inplace_variant" or similar options can query for it from the kwarg dict.

What would you think of an approach like that?

I like the second approach (though more disruptive). Will try that.
Thanks!

For test_variant_consistency_eager,

pytorch/test/test_ops.py

Lines 204 to 242 in 0d81528

@_variant_ops(op_db)

def test_variant_consistency_eager(self, device, dtype, op):

samples = op.sample_inputs(device, dtype, requires_grad=op.supports_autograd)

for sample in samples:

# Acquires variants (method variant, inplace variant, aliases)

method = op.get_method()

inplace = op.get_inplace()

# list of all inplace ops: inplace variant + alias inplace variants if exist

inplace_ops = [inplace, ]

aliases = []

for a_op in op.aliases:

aliases.append(a_op.op)

aliases.append(a_op.method_variant)

aliases.append(a_op.inplace_variant)

inplace_ops.append(a_op.inplace_variant)

aliases = tuple(aliases)

inplace_ops = tuple(v for v in inplace_ops if v is not None)

variants = (v for v in (method, inplace) + aliases if v is not None)

# Computes function forward and backward values

sample.input.grad = None

expected_forward = op(sample.input, *sample.args, **sample.kwargs)

expected_grad = None

# TODO: backward consistency only supported for single tensor outputs

# TODO: backward consistency only checked on sample.input, not all

# tensor inputs

# TODO: update to handle checking grads of all tensor inputs as

# derived from each tensor output

if (op.supports_autograd and isinstance(expected_forward, torch.Tensor)):

expected_forward.sum().backward()

expected_grad = sample.input.grad

# Test eager consistency

for variant in variants:

We will have to put call for sample_inputs inside the variants loop. Since call to sample_input has to actually materialize all the tensors and perform forward multiple times on the same input sample (for different variant if that sample is valid for multiple variants), this will result in performance regression for this test.

I am feeling a bit sceptical about this now.
Let me know if that sounds acceptable. (or if i missed anything)

That's a great point, and that function could probably use a small refactoring. I agree with you but think we can mitigate the damage:

samples = op.sample_inputs(device, dtype, requires_grad=op.supports_autograd) for sample in samples: # Acquires variants (method variant, inplace variant, aliases) method = op.get_method() inplace = op.get_inplace() # list of all inplace ops: inplace variant + alias inplace variants if exist inplace_ops = [inplace, ] aliases = [] for a_op in op.aliases: aliases.append(a_op.op) aliases.append(a_op.method_variant) aliases.append(a_op.inplace_variant) inplace_ops.append(a_op.inplace_variant) aliases = tuple(aliases) inplace_ops = tuple(v for v in inplace_ops if v is not None) variants = (v for v in (method, inplace) + aliases if v is not None)

What's weird about this is that the test is rebuilding its understand of the operator's variants on each sample. This is unnecessary since variants are sample-independent. We should lift that section out of the for loop and acquire the variants once upfront.

Now let's look at the rest of the function:

# Computes function forward and backward values sample.input.grad = None expected_forward = op(sample.input, *sample.args, **sample.kwargs) expected_grad = None if (op.supports_autograd and isinstance(expected_forward, torch.Tensor)): expected_forward.sum().backward() expected_grad = sample.input.grad for variant in variants: sample.input.grad = None cloned = clone_input_helper(sample.input) if variant in inplace_ops else sample.input variant_forward = variant(cloned, *sample.args, **sample.kwargs) self.assertEqual(expected_forward, variant_forward) if expected_grad is not None and (variant not in inplace_ops or op.supports_inplace_autograd): variant_forward.sum().backward() self.assertEqual(expected_grad, sample.input.grad)

I think we can get rid of the for variant in variants loop here by using a itertools.product(sample inputs, variants) after the sample input and variant acquisition at the start of the test. Then we can create a helper function that executes this test body. The helper function takes a variant, a sample input, and whether the operation is inplace or not (so it knows whether to perform the copy. Then the test works like this:

the variants are identified

sample inputs are acquired

if the op has an inplace variant, inplace sample inputs are acquired

an itertools product of sample inputs x non-inplace variants invokes the helper function

an itertools product of inplace sample inputs x inplace variants invokes the helper function

Does that make sense? Looking forward to hearing your thoughts.

You are correct that this will still redundantly create sample inputs that are common to both the out-of-place and in-place variant. Which is unfortunate from a performance standpoint, and your idea of marking SampleInputs as safe-for-inplace would have handled this problem better. However I think this is an OK penalty (for now, anyway).

It does make sense. I'll try to do that. Will ping here if I stumble into another block.

Thanks!

test/test_ops.py

kshitij12345 · 2021-03-31T09:19:47Z

@mruberry, have fixed the merge conflicts. I think this is ready for another round. PTAL :)

Note:
One thing to note is that this PR is volatile, in the sense that if a new samle_input_* is added with current signature (without **kwargs) then this PR will lead to errors as it will pass an invalid argument (so it should be rebased prior to landing and no new sample_input_* should be added in between).

Or should we add a try except (similar to the one mentioned above) temporarily and then remove once this signature becomes the norm?

try:
  samples = op.sample_inputs(..., **kwargs)
except TypeError as te:
  samples = op.sample_inputs(...)

Thanks!

mruberry · 2021-04-05T06:59:19Z

test/test_ops.py

+                return variant.__wrapped__ is op.get_inplace()
+            return variant is op.get_inplace()
+
+        samples = op.sample_inputs(device, dtype, requires_grad=True,


I think your proposal to wrap this in a try/except so not every sample_input needs to be updated to use **kwargs when this lands is a good idea.

mruberry · 2021-04-05T06:59:34Z

test/test_ops.py

+
+        if len(inplace_ops) > 0:
+            inplace_samples = op.sample_inputs(device, dtype, requires_grad=_requires_grad,
+                                               for_inplace_variant=True)


Can also wrap this in try/except to minimize logical merge conflicts

mruberry · 2021-04-05T07:02:12Z

torch/testing/_internal/common_methods_invocations.py

+            yield SampleInput(make_arg((S,)),
+                              args=(torch.randn(S, S, device=device) > 0, make_arg(())))
+            yield SampleInput(make_arg((S,)),
+                              args=(torch.randn(S, S, device=device) > 0, 10))


Why this change instead of using the bernoulli op?

bernoulli produces scalar tensor however we want (S, S) tensor.

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 3908 to 3909 in 3d492b0

def bernoulli_scalar():

return torch.tensor(0, dtype=torch.bool).bernoulli_()

('masked_fill', (M,), (torch.BoolTensor(M, M).bernoulli_(), 10), 'broadcast_lhs'),

I guess I mean where did this case go? We don't have to worry about it in this PR.

Ah, replaced it with ( semantically both are doing the same thing),

SampleInput(make_arg((S,)), args=(torch.randn(S, S, device=device) > 0, 10)

mruberry

Hey @kshitij12345! Overall this looks good, as usual.

I made a few comments; I like your suggestion to reduce logical merge conflicts by using a try/except.

I think the OpInfos for masked_fill and masked_scatter can also be updated with this change, right?

pytorch/torch/testing/_internal/common_methods_invocations.py

Line 3039 in f16c827

SkipInfo('TestOpInfo', 'test_duplicate_method_tests'),

pytorch/torch/testing/_internal/common_methods_invocations.py

Line 3048 in f16c827

SkipInfo('TestOpInfo', 'test_duplicate_method_tests'),

kshitij12345 · 2021-04-05T09:40:51Z

torch/testing/_internal/common_methods_invocations.py

+            samples = self.sample_inputs_func(self, device, dtype, requires_grad, **kwargs)
+        except TypeError:
+            samples = self.sample_inputs_func(self, device, dtype, requires_grad)
+        return samples


Have added the try/except here. (we don't need to add try/except in test_ops)

@mruberry can you please see if this is ok? Will rebase accordingly then. Thanks!

Yep, this seems fine

mruberry · 2021-04-07T06:03:03Z

test/test_ops.py

-        inplace_ops = tuple(v for v in inplace_ops if v is not None)
-        variants = (v for v in (method, inplace) + aliases if v is not None)
+        inplace_variants = tuple(v for v in inplace_ops if v is not None)
+        variants = tuple(v for v in (method, inplace) + aliases if v is not None)


mruberry

Cool! And thanks for including the fix for the variant generator. If you rebase this now I think we can get it landed, @kshitij12345.

I made one comment, but we don't need to worry about in this PR even if it is an issue.

facebook-github-bot · 2021-04-07T06:06:47Z

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-04-07T06:23:37Z

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

kshitij12345 · 2021-04-07T06:31:05Z

With this, we might want to update/add entry to mention this new change at https://github.com/pytorch/pytorch/wiki/Writing-tests-in-PyTorch-1.8 and #54261 (OpInfo porting tracker issue)

codecov · 2021-04-07T10:59:03Z

Codecov Report

Merging #53014 (64883fa) into master (bc05867) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #53014      +/-   ##
==========================================
- Coverage   77.42%   77.23%   -0.20%     
==========================================
  Files        1895     1895              
  Lines      187524   187556      +32     
==========================================
- Hits       145194   144859     -335     
- Misses      42330    42697     +367

mruberry · 2021-04-07T15:00:28Z

Edit: nevermind, ROCm failures are in the base!

Hmm... I was hoping to land this, but the ROCm build failure is worrying. @kshitij12345, would you take a look?

facebook-github-bot · 2021-04-07T15:22:18Z

@mruberry merged this pull request in 17e5ba4.

kshitij12345 added 2 commits March 1, 2021 05:04

support input samples where self is broadcasted

cf39918

remove redundant test

0fe301d

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

pytorchbot added the open source label Mar 1, 2021

Merge branch 'master' into develop/testing/inplace-broadcast

2e9ebcf

kshitij12345 commented Mar 4, 2021

View reviewed changes

kshitij12345 mentioned this pull request Mar 4, 2021

test_inplace_grad and test_variant_consistency_eager have to be disabled while testing broadcasting semantics in OpInfo based tests #50747

Closed

mruberry reviewed Mar 4, 2021

View reviewed changes

anjali411 self-requested a review March 4, 2021 14:02

mattip mentioned this pull request Mar 11, 2021

Operator and module tests in PyTorch 1.10 tracker #50006

Closed

16 tasks

kshitij12345 added 4 commits March 23, 2021 23:38

Merge branch 'master' into develop/testing/inplace-broadcast

42c78dc

try updated approach

9a0f3c7

remove stray entry

86dc333

make mypy happy

75e7e98

kshitij12345 commented Mar 24, 2021

View reviewed changes

test/test_ops.py Show resolved Hide resolved

kshitij12345 marked this pull request as ready for review March 31, 2021 06:26

kshitij12345 added 5 commits March 31, 2021 02:06

Merge branch 'master' into develop/testing/inplace-broadcast

5f8e998

fix merge issues

e96c512

remove stray print

c640bfc

fix incorrect merge

af7772e

use generator for sampler internally

7cb0cbb

Merge branch 'master' into develop/testing/inplace-broadcast

f16c827

mruberry reviewed Apr 5, 2021

View reviewed changes

kshitij12345 added 2 commits April 5, 2021 04:22

Merge branch 'master' into develop/testing/inplace-broadcast

98dd0d1

Merge branch 'master' into develop/testing/inplace-broadcast

aa4fd5b

kshitij12345 added 2 commits April 5, 2021 04:32

remove unnecessary skips

90cc29b

add comment

d3dd360

kshitij12345 commented Apr 5, 2021

View reviewed changes

kshitij12345 mentioned this pull request Apr 5, 2021

testing: run eager consistency test on all samples #55300

Closed

kshitij12345 added 2 commits April 5, 2021 07:02

update sampler for copysign

e3c0c24

fix the condition

55076c7

RockingJavaBean mentioned this pull request Apr 5, 2021

Port torch.copysign method_tests() to OpInfo #54945

Closed

kshitij12345 mentioned this pull request Apr 7, 2021

OpInfo & test_torch.py cleanup #55201

Closed

mruberry reviewed Apr 7, 2021

View reviewed changes

mruberry approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into develop/testing/inplace-broadcast

64883fa

facebook-github-bot closed this in 17e5ba4 Apr 7, 2021

facebook-github-bot added the Merged label Apr 7, 2021

imaginary-person mentioned this pull request Apr 7, 2021

Add OpInfo tests for torch.addbmm #55378

Closed

This was referenced Apr 8, 2021

Verify that attempting to resize a tensor with an inplace operation throws a runtime error #55595

Closed

[hackathon] port addmv to OpInfo #55545

Closed

	def _get_safe_inplace(self, inplace_variant):
	@wraps(inplace_variant)
	def _fn(t, args, *kwargs):
	return inplace_variant(t.clone(), args, *kwargs)

	return _fn

	def sample_inputs(self, device, dtype, requires_grad=False):
	"""Returns an iterable of SampleInputs.

	These samples should be sufficient to test the function works correctly
	with autograd, TorchScript, etc.
	"""
	return self.sample_inputs_func(self, device, dtype, requires_grad)

	@_variant_ops(op_db)
	def test_variant_consistency_eager(self, device, dtype, op):
	samples = op.sample_inputs(device, dtype, requires_grad=op.supports_autograd)

	for sample in samples:
	# Acquires variants (method variant, inplace variant, aliases)
	method = op.get_method()
	inplace = op.get_inplace()

	# list of all inplace ops: inplace variant + alias inplace variants if exist
	inplace_ops = [inplace, ]

	aliases = []
	for a_op in op.aliases:
	aliases.append(a_op.op)
	aliases.append(a_op.method_variant)
	aliases.append(a_op.inplace_variant)
	inplace_ops.append(a_op.inplace_variant)
	aliases = tuple(aliases)

	inplace_ops = tuple(v for v in inplace_ops if v is not None)
	variants = (v for v in (method, inplace) + aliases if v is not None)

	# Computes function forward and backward values
	sample.input.grad = None
	expected_forward = op(sample.input, sample.args, *sample.kwargs)
	expected_grad = None

	# TODO: backward consistency only supported for single tensor outputs
	# TODO: backward consistency only checked on sample.input, not all
	# tensor inputs
	# TODO: update to handle checking grads of all tensor inputs as
	# derived from each tensor output
	if (op.supports_autograd and isinstance(expected_forward, torch.Tensor)):
	expected_forward.sum().backward()
	expected_grad = sample.input.grad

	# Test eager consistency
	for variant in variants:

	def bernoulli_scalar():
	return torch.tensor(0, dtype=torch.bool).bernoulli_()

[testing] Support input samples where self is broadcasted. #53014

[testing] Support input samples where self is broadcasted. #53014

Uh oh!

Conversation

kshitij12345 commented Mar 1, 2021

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

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!

mruberry Mar 4, 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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

kshitij12345 commented Mar 31, 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!

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

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 7, 2021

Uh oh!

facebook-github-bot commented Apr 7, 2021

Uh oh!

[testing] Support input samples where `self` is broadcasted. #53014

[testing] Support input samples where `self` is broadcasted. #53014

facebook-github-bot commented Mar 1, 2021 •

edited

Loading

mruberry Mar 4, 2021 •

edited

Loading

kshitij12345 Mar 24, 2021 •

edited

Loading

kshitij12345 Mar 24, 2021 •

edited

Loading

kshitij12345 Mar 24, 2021 •

edited

Loading

mruberry commented Apr 7, 2021 •

edited

Loading