patching clamp for one sided clamp by jjsjann123 · Pull Request #75558 · pytorch/pytorch

jjsjann123 · 2022-04-09T08:09:59Z

Fixes #75088

The solution is just to avoid putting random value for non-specified clamp as pointed out in #75088 (comment)

facebook-github-bot · 2022-04-09T08:10:05Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/75558
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

💊 CI failures summary and remediations

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

ngimel · 2022-04-11T18:08:18Z

Cool, can you do a more systematic testing for extreme values? E.g. min/max look like they should be codegening fmin/fmax and thus handle nans correctly, but without systematic testing it's hard to be sure, there might be other ops like relu/hardtanh/hardsigmoid and the like that might or might not do correct things for nan/inf propagation, or produces 0-s in the non-differentiable points?

jjsjann123 · 2022-04-11T18:20:22Z

Cool, can you do a more systematic testing for extreme values? E.g. min/max look like they should be codegening fmin/fmax and thus handle nans correctly, but without systematic testing it's hard to be sure, there might be other ops like relu/hardtanh/hardsigmoid and the like that might or might not do correct things for nan/inf propagation, or produces 0-s in the non-differentiable points?

I think our unary/binary test do cover all these special numbers.

pytorch/test/test_jit_cuda_fuser.py

Lines 126 to 130 in 1af7105

    
           self.special_values = torch.tensor( 
        
               [float("-inf"), -10, -math.pi, 
        
                   -1, -0.5, 0, 1, 0.5, 
        
                   math.pi, 10, float("inf"), 
        
                   float("nan")], dtype=torch.float, device=dev)

pytorch/test/test_jit_cuda_fuser.py

Lines 544 to 567 in 1af7105

    
           if random_data: 
        
               x = torch.rand(shape, dtype=torch.float32, device="cuda", requires_grad=gradient_check) 
        
               if dtype in self.int_types: 
        
                   # prefer a larger variance for integer types 
        
                   x = x * 5 
        
               x = x.to(dtype=dtype) 
        
           else: 
        
               x = self.special_values.to(dtype=dtype) 
        
           try: 
        
               ref = t(x, y) 
        
           except Exception: 
        
               # same way as TE checker, if eager mode throws, ignore this test 
        
               return 
        
           t_jit = torch.jit.script(t) 
        
           jit_o = t_jit(x, y) 
        
           jit_o = t_jit(x, y) 
        
           jit_o = t_jit(x, y) 
        
           if gradient_check: 
        
               gradcheck(t_jit, [x, y], nondet_tol=1e-5) 
        
           elif dtype in self.support_tensor_dtypes: 
        
               self.assertGraphContains(t_jit.graph_for(x, y), FUSION_GUARD) 
        
           o = t(x, y) 
        
           self.assertEqual(o.dtype, jit_o.dtype) 
        
           self.assertTrue(self._compare("failing case {}\n{}\n{}\n{}".format(dtype, operation, x, y), o, jit_o, 1e-2))

pytorch/test/test_jit_cuda_fuser.py

Lines 791 to 796 in 1af7105

    
           if random_data: 
        
               x = (torch.randn(shapex, dtype=torch.float, device="cuda") * 5).to(dtype_arg1) 
        
               y = (torch.randn(shapey, dtype=torch.float, device="cuda") * 5).to(dtype_arg2) 
        
           else: 
        
               x = self.special_values.to(dtype=dtype_arg1) 
        
               y = (torch.rand_like(self.special_values) * 5).to(dtype_arg2)

hardtanh is not in our parser yet. But I do agree that we should probably having a generalized helper function for ternary ops as well. I'll update the tests then.

ngimel · 2022-04-11T18:23:22Z

Apparently, they weren't covering these special numbers, or these failure wouldn't have happened?
Another case where I see discrepancy between nvfuser and eager is when min is greater than max:

In [25]: def fn(x):
    ...:     x=x.clamp(min=1., max=0.5)*.1
    ...:     return x
    ...: 
    ...: a=torch.tensor([1.,float('inf'), 2., float('inf')], device="cuda")
    ...: scripted = torch.jit.script(fn)
    ...: fn(a)
    ...: with torch.jit.fuser("fuser2"):
    ...:     for _ in range(10):
    ...:         scripted(a)
    ...: print(fn(a))
    ...: print(scripted(a))
tensor([0.0500, 0.0500, 0.0500, 0.0500], device='cuda:0')
tensor([0.0500, 0.0500, 0.0500, 0.0500], device='cuda:0')

In [26]: print(fn(x.cuda()))
tensor([0.0500, 0.0500, 0.0500, 0.0500], device='cuda:0')

In [27]: print(scripted(x.cuda()))
tensor([0.1000, 0.1000, 0.1000, 0.0500], device='cuda:0')

In [28]: x
Out[28]: tensor([-0.3980, -0.2727,  0.1300,  2.0310])

Note, although this is arguably an error case, numpy, jax and torch eager all have the same behavior in this case, with nvfuser being an outlier.

jjsjann123 · 2022-04-11T21:36:18Z

clamp is a ternary op which is not covered by unary/binary tests and that's what I was promising to update in our tests.

x=x.clamp(min=1., max=0.5) That's a confusing case... I think we just need to add two lines to regulate the min/max range. I'll update that to aten logic... But if these cases are defined behavior, maybe we should update opinfo tests to have it backed up by CI?

ngimel · 2022-04-11T21:49:22Z

As far as NVFuser is concerned, clamp is a unary op with kwargs, NVFuser accepts only (Tensor, Scalar, Scalar) overload.

ngimel · 2022-04-11T21:52:00Z

And yes, adding a test for this behavior (since it's documented) is necessary, I wasn't able to find existing one. cc @mruberry

ngimel · 2022-04-11T21:56:10Z

torch/csrc/jit/codegen/cuda/parser.cpp

+            bool has_high = value_map.count(node->inputs()[2]->unique()) != 0;
+            Val* high = has_high
                ? *value_map[node->inputs()[2]->unique()]
                : IrBuilder::create<Double>(std::numeric_limits<float>::max());


You still have these very misleading high and low assignments, you should be able to remove them?

jjsjann123 · 2022-04-12T03:01:07Z

@pytorchbot merge this

github-actions · 2022-04-12T03:03:05Z

Hey @jjsjann123.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Fixes #75088 The solution is just to avoid putting random value for non-specified clamp as pointed out in #75088 (comment) Pull Request resolved: #75558 Approved by: https://github.com/ngimel Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/0203341bbde7cebdb9a04d9f021797b8bea7de2f Reviewed By: mehtanirav Differential Revision: D35582770 fbshipit-source-id: d7781249f568c8cf28ecbf4bbce3c4d3b0f947ce

Fixes pytorch#75558 (comment) Updated clamp logic to be consistent with aten. This avoids us producing different result when clamp was given a min/max argument where min > max. We don't have an issue opened for this. It is also tricky to argue the right behavior, but getting better consistency with eager is always(?!) a good thing.

Fixes pytorch#75088 The solution is just to avoid putting random value for non-specified clamp as pointed out in pytorch#75088 (comment) Pull Request resolved: pytorch#75558 Approved by: https://github.com/ngimel

Fixes #75088 The solution is just to avoid putting random value for non-specified clamp as pointed out in pytorch/pytorch#75088 (comment) Pull Request resolved: pytorch/pytorch#75558 Approved by: https://github.com/ngimel

patching clamp for one sided clamp

e28101c

jjsjann123 requested a review from ngimel April 9, 2022 08:09

facebook-github-bot added the cla signed label Apr 9, 2022

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 9, 2022

pytorchbot added the open source label Apr 9, 2022

samdow added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 11, 2022

jjsjann123 added 2 commits April 11, 2022 11:01

fixing test cases

0eef054

removing print

1af7105

ngimel reviewed Apr 11, 2022

View reviewed changes

jjsjann123 added 3 commits April 11, 2022 15:49

update per review comment

c2a357c

Merge remote-tracking branch 'upstream/master' into clamp_patch

addfe76

clang-format

17c2c80

jjsjann123 requested a review from ngimel April 11, 2022 23:24

ngimel approved these changes Apr 11, 2022

View reviewed changes

pytorchmergebot closed this in 0203341 Apr 12, 2022

jjsjann123 mentioned this pull request Apr 12, 2022

consistent clamp for reversed min/max bound csarofeen/pytorch#1572

Merged

jjsjann123 mentioned this pull request Apr 17, 2022

patching clamp for one sided clamp csarofeen/pytorch#1586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patching clamp for one sided clamp#75558

patching clamp for one sided clamp#75558
jjsjann123 wants to merge 6 commits intopytorch:masterfrom
jjsjann123:clamp_patch

jjsjann123 commented Apr 9, 2022

Uh oh!

facebook-github-bot commented Apr 9, 2022 •

edited

Loading

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

jjsjann123 commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

jjsjann123 commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

ngimel Apr 11, 2022

Uh oh!

jjsjann123 commented Apr 12, 2022

Uh oh!

github-actions bot commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jjsjann123 commented Apr 9, 2022

Uh oh!

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

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

jjsjann123 commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

jjsjann123 commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

ngimel commented Apr 11, 2022

Uh oh!

ngimel Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

jjsjann123 commented Apr 12, 2022

Uh oh!

github-actions bot commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

facebook-github-bot commented Apr 9, 2022 •

edited

Loading