Alias for `polygamma` #59691

krshrimali · 2021-06-09T07:57:14Z

See #50345

cc: @mruberry @kshitij12345

facebook-github-bot · 2021-06-09T07:57:21Z

💊 CI failures summary and remediations

As of commit 6195f5a (more details on the Dr. CI page and at hud.pytorch.org/pr/59691):

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_py3_clang5_asan_test1 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Jul 09 06:15:46 SUMMARY: UndefinedBehaviorSanit.../jenkins/workspace/aten/src/ATen/Utils.cpp:20:3 in

Jul 09 06:15:46     #9 0x55f6a043d8f2 in PyEval_EvalCode /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Python/ceval.c:731
Jul 09 06:15:46     #10 0x55f6a04a5cd5 in run_mod /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Python/pythonrun.c:1025
Jul 09 06:15:46     #11 0x55f6a04a7d5d in PyRun_StringFlags /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Python/pythonrun.c:949
Jul 09 06:15:46     #12 0x55f6a04a7dbb in PyRun_SimpleStringFlags /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Python/pythonrun.c:445
Jul 09 06:15:46     #13 0x55f6a04a8926 in run_command /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Modules/main.c:301
Jul 09 06:15:46     #14 0x55f6a04a8926 in Py_Main /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Modules/main.c:749
Jul 09 06:15:46     #15 0x55f6a03e2196 in main /home/builder/ktietz/cos6/ci_cos6/python_1622833237666/work/Programs/python.c:69
Jul 09 06:15:46     #16 0x7fbaa4fe083f in __libc_start_main /build/glibc-S7Ft5T/glibc-2.23/csu/../csu/libc-start.c:291
Jul 09 06:15:46     #17 0x55f6a047233d in _start (/opt/conda/bin/python3.6+0x1a733d)
Jul 09 06:15:46 
Jul 09 06:15:46 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/aten/src/ATen/Utils.cpp:20:3 in 
Jul 09 06:15:46 + retcode=1
Jul 09 06:15:46 + set -e
Jul 09 06:15:46 + return 1
Jul 09 06:15:46 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 == *-NO_AVX-* ]]
Jul 09 06:15:46 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 == *-NO_AVX2-* ]]
Jul 09 06:15:46 + '[' -n https://github.com/pytorch/pytorch/pull/59691 ']'
Jul 09 06:15:46 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 != *coverage* ]]
Jul 09 06:15:46 ++ mktemp
Jul 09 06:15:46 + DETERMINE_FROM=/tmp/tmp.F57h7VqpJk
Jul 09 06:15:46 + file_diff_from_base /tmp/tmp.F57h7VqpJk

Preview docs built from this PR

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.

Click here to manually regenerate this comment.

…ant consistency checks I need to add comments.

krshrimali · 2021-06-11T20:59:38Z

torch/testing/_internal/common_methods_invocations.py

-                       SkipInfo('TestCommon', 'test_variant_consistency_jit'),),
+                       SkipInfo('TestCommon', 'test_variant_consistency_jit'),
+                       SkipInfo('TestCommon', 'test_jit_alias_remapping'),
+                       SkipInfo('TestCommon', 'test_variant_consistency_eager')),


This test fails because of the ordering of args in polygamma/special_polygamma: (int, tensor) but JIT tests expect first input to be a tensor.

====================================================================== ERROR: test_variant_consistency_eager_polygamma_polygamma_n_0_cpu_float32 (__main__.TestCommonCPU) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/krshrimali/git/krshrimali/pytorch/torch/testing/_internal/common_device_type.py", line 292, in instantiated_test result = test_fn(self, *args) File "/home/krshrimali/git/krshrimali/pytorch/torch/testing/_internal/common_device_type.py", line 266, in test_wrapper return test(*args, **kwargs) File "/home/krshrimali/git/krshrimali/pytorch/test/test_ops.py", line 345, in test_variant_consistency_eager _test_consistency_helper(samples, variants) File "/home/krshrimali/git/krshrimali/pytorch/test/test_ops.py", line 334, in _test_consistency_helper variant_forward = variant(cloned, TypeError: special_polygamma(): argument 'n' (position 1) must be int, not Tensor

This is not related to JIT but we use lambda to reorder x, n while calling polygamma

op=lambda x, n, **kwargs: torch.polygamma(n, x, **kwargs)

This is because we only verify gradients in consistency_eager for input=Tensor (but not the ones in arg).

This test now fails because, we don't reorder the arguments for the alias. (thus it expects first argument to be int and not a Tensor)

To mitigate should we allow specifying lambda wrappers around aliases? Not sure but might need to investigate.

Relevant lines of test

Here we acquire the actual operator of the mentioned alias.

pytorch/test/test_ops.py

Lines 288 to 292 in cf38b20

for a_op in op.aliases:

variants.append(a_op.op)

variants.append(a_op.method_variant)

variants.append(a_op.inplace_variant)

inplace_ops.append(a_op.inplace_variant)

Here we call the alias variant

pytorch/test/test_ops.py

Line 354 in cf38b20

variant_forward = variant(cloned,

After an offline discussion with @krshrimali and thinking a bit more. We might as well just add a new entry for special.polygamma in OpInfo and maybe add a handcoded alias test for polygamma?

Reason:

Even if we go through the hassle of making sure test_variant_consistency_eager works, we still don't get the alias_remapping test due to the use of lambda.

I don't think there are other operators which might need this op_aliases. (It is particularly due to the peculiar signature of polygamma that we need this)

@mruberry what is your opinion?
Thanks!

I see the issue. Sure, implementing a new OpInfo for it (with a comment) sounds like a fine workaround.

torch/testing/_internal/common_methods_invocations.py

krshrimali · 2021-06-13T13:42:21Z

The Windows CI (pytorch-win-vs2019-cpu-py3) / test error is real (once all tests run - I'm expecting more tests to fail for the same reason), this was debugged for logsumexp PR and will be fixed there (#58838). Shouldn't be a long blocker though, logsumexp PR should be ready for another review in a day. :)

…amma

kshitij12345

Overall looks good. Thanks @krshrimali

Have one minor nit regarding the signature of special::polygamma_out.

However, I am concerned about disabling the consistency_eager test. Have put some pointers below as to why the test is failing. Would be nice if we can have a workaround without disabling it. Can you please have a look at that.

Thanks!

kshitij12345 · 2021-06-14T11:17:41Z

torch/csrc/api/include/torch/special.h

+  return torch::special_polygamma(n, self);
+}
+
+inline Tensor& polygamma_out(int64_t n, Tensor& result, const Tensor& self) {


We should stick to the signature of polygamma_out which is Tensor& polygamma_out(Tensor& result, int64_t n, const Tensor& self)

kshitij12345 · 2021-06-14T11:26:22Z

torch/testing/_internal/common_methods_invocations.py

-                       SkipInfo('TestCommon', 'test_variant_consistency_jit'),),
+                       SkipInfo('TestCommon', 'test_variant_consistency_jit'),
+                       SkipInfo('TestCommon', 'test_jit_alias_remapping'),
+                       SkipInfo('TestCommon', 'test_variant_consistency_eager')),


This is not related to JIT but we use lambda to reorder x, n while calling polygamma

op=lambda x, n, **kwargs: torch.polygamma(n, x, **kwargs)

This is because we only verify gradients in consistency_eager for input=Tensor (but not the ones in arg).

This test now fails because, we don't reorder the arguments for the alias. (thus it expects first argument to be int and not a Tensor)

To mitigate should we allow specifying lambda wrappers around aliases? Not sure but might need to investigate.

Relevant lines of test

Here we acquire the actual operator of the mentioned alias.

pytorch/test/test_ops.py

Lines 288 to 292 in cf38b20

for a_op in op.aliases:

variants.append(a_op.op)

variants.append(a_op.method_variant)

variants.append(a_op.inplace_variant)

inplace_ops.append(a_op.inplace_variant)

Here we call the alias variant

pytorch/test/test_ops.py

Line 354 in cf38b20

variant_forward = variant(cloned,

…amma

… into dev/special_polygamma

mruberry · 2021-06-23T23:21:36Z

Hey @krshrimali! Just checking in here. Looks like some tests are failing -- everything going OK on this PR? Is there something I'm supposed to do for it?

krshrimali · 2021-06-24T00:28:00Z

Hey @krshrimali! Just checking in here. Looks like some tests are failing -- everything going OK on this PR? Is there something I'm supposed to do for it?

Thanks, @mruberry for the question. test_overrides failures are the only relevant test failures in this PR and this has been fixed in #58838 (logsumexp PR). (also mentioned here: #59691 (comment)).

If you could take a look at logsumexp whenever possible, and once that is in - this PR should be ready for final review/importing as well.

P.S: If we think that logsumexp is gonna take significant time, I can put that relevant change from that PR to this one so that this isn't blocked at least.

mruberry · 2021-06-24T05:43:35Z

Hey @krshrimali! Just checking in here. Looks like some tests are failing -- everything going OK on this PR? Is there something I'm supposed to do for it?

Thanks, @mruberry for the question. test_overrides failures are the only relevant test failures in this PR and this has been fixed in #58838 (logsumexp PR). (also mentioned here: #59691 (comment)).

If you could take a look at logsumexp whenever possible, and once that is in - this PR should be ready for final review/importing as well.

P.S: If we think that logsumexp is gonna take significant time, I can put that relevant change from that PR to this one so that this isn't blocked at least.

Got it. No, I don't think logsumexp will take too long. If the jit team doesn't get back to us soon I'll ping them

mruberry · 2021-07-08T22:50:45Z

torch/testing/_internal/common_methods_invocations.py

        self.name = alias_name
-        self.op = _getattr_qual(torch, alias_name)
+        self.op = alias_op if alias_op else _getattr_qual(torch, alias_name)


Nice extension

Apologies, earlier we decided to keep this to avoid extra OpInfo entry for special alias. Later, we decided to instead have an extra OpInfo entry since it's only required for ops like polygamma which need re-ordering of the arguments. (context: #59691 (comment))

This has been removed now.

mruberry · 2021-07-08T22:52:55Z

Unfortunately it looks like the jit test is still failing:

test_variant_consistency_jit_special_polygamma_special_polygamma_n_0_cpu_float32

krshrimali · 2021-07-09T03:22:19Z

Thanks @mruberry for taking a look. The skipped test had the wrong class mentioned, hence the errors. This should be fixed now. Should be ready for review once all the tests pass. :) Thank you!

torch/testing/_internal/common_methods_invocations.py

krshrimali · 2021-07-12T11:25:37Z

Gentle ping, PTAL @mruberry - whenever you find time. The failing test seems unrelated to the PR.

mruberry · 2021-07-15T06:19:37Z

torch/testing/_internal/common_methods_invocations.py

@@ -7029,6 +7029,34 @@ def gradcheck_wrapper_triangular_input(op, input, *args, upper=False, **kwargs):
                       #            ~~~~~~~~~~~~~~~ <--- HERE
                       SkipInfo('TestJit', 'test_variant_consistency_jit'),),
                   sample_kwargs=lambda device, dtype, input: ({'n': 0}, {'n': 0})),
+    # A separate OpInfo entry for special.polygamma is needed to reorder the arguments


Just testing this for the n=0 case makes sense

mruberry

Cool! Thanks @krshrimali

facebook-github-bot · 2021-07-15T06:20:06Z

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

facebook-github-bot · 2021-07-16T07:08:01Z

@mruberry merged this pull request in 7e1f01d.

krshrimali added 3 commits June 9, 2021 13:01

Polygamma special migration, to be tested

34fb9ab

Add alias entry to OpInfo

3b313dd

Polygamma special migration

91c8149

krshrimali added the module: special Functions with no exact solutions, analogous to those in scipy.special label Jun 9, 2021

krshrimali requested a review from ezyang as a code owner June 9, 2021 07:57

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Jun 9, 2021

minor typo fix

8392a2e

krshrimali removed the request for review from ezyang June 9, 2021 07:59

pytorchbot added the open source label Jun 9, 2021

krshrimali changed the title ~~Alias for polygamma~~ [WIP] Alias for polygamma Jun 9, 2021

krshrimali marked this pull request as draft June 9, 2021 08:50

krshrimali added 5 commits June 10, 2021 10:20

Still fixing special_polygamma's signature

8192e1e

Change ordering of args to match with polygamma

c1760bf

Ordering fix and skip jit - eager and variant consistency checks

458ed13

Re-ordering arguments to special_polygamma, skip jit - eager and vari…

7d17d64

…ant consistency checks I need to add comments.

Merge branch 'master' into dev/special_polygamma

ed1cfcd

krshrimali marked this pull request as ready for review June 11, 2021 10:38

krshrimali changed the title ~~[WIP] Alias for polygamma~~ Alias for polygamma Jun 11, 2021

krshrimali commented Jun 11, 2021

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Outdated Show resolved Hide resolved

krshrimali requested review from kshitij12345 and mruberry June 13, 2021 09:44

Remove print statements

9371a09

Merge remote-tracking branch 'upstream/master' into dev/special_polyg…

bde9217

…amma

kshitij12345 reviewed Jun 14, 2021

View reviewed changes

krshrimali added 2 commits June 14, 2021 20:02

Allow adding lambda wrappers around aliases

0abeb67

Merge branch 'master' into dev/special_polygamma

620e028

krshrimali added 10 commits June 14, 2021 20:15

Typo fix

3907b88

ignore added to make mypy happy

d19142d

Minor typo fix

f71041a

Add op to op_skip and add separate OpInfo for special.polygamma

6d963b4

Add OpInfo entry for special polygamma

b7ccf53

Minor correction in syntax

651039a

Merge branch 'master' into dev/special_polygamma

37292ce

Merge remote-tracking branch 'upstream/master' into dev/special_polyg…

a15aa15

…amma

Add special.polygamma to known_no_schema

5e011f1

Merge branch 'dev/special_polygamma' of github.com:krshrimali/pytorch…

4ef94c1

… into dev/special_polygamma

kshitij12345 mentioned this pull request Jun 24, 2021

torch.special tracking issue #50345

Open

17 tasks

Fix merge conflict with upstream

5d2743a

mruberry reviewed Jul 8, 2021

View reviewed changes

krshrimali added 2 commits July 9, 2021 03:12

Fix test class for variant consistency jit

0503d20

Remove alias remapping skip, not needed anymore

b381c37

kshitij12345 reviewed Jul 9, 2021

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Outdated Show resolved Hide resolved

Remove aliases from other opinfo variants

6195f5a

mruberry reviewed Jul 15, 2021

View reviewed changes

mruberry approved these changes Jul 15, 2021

View reviewed changes

facebook-github-bot closed this in 7e1f01d Jul 16, 2021

facebook-github-bot added the Merged label Jul 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alias for `polygamma` #59691

Alias for `polygamma` #59691

krshrimali commented Jun 9, 2021 •

edited

facebook-github-bot commented Jun 9, 2021 •

edited

krshrimali Jun 11, 2021

kshitij12345 Jun 14, 2021

kshitij12345 Jun 14, 2021

mruberry Jun 16, 2021

krshrimali commented Jun 13, 2021 •

edited

kshitij12345 left a comment

kshitij12345 Jun 14, 2021

kshitij12345 Jun 14, 2021

mruberry commented Jun 23, 2021

krshrimali commented Jun 24, 2021

mruberry commented Jun 24, 2021

mruberry Jul 8, 2021

krshrimali Jul 9, 2021 •

edited

mruberry commented Jul 8, 2021

krshrimali commented Jul 9, 2021

krshrimali commented Jul 12, 2021

mruberry Jul 15, 2021

mruberry left a comment

facebook-github-bot commented Jul 15, 2021

facebook-github-bot commented Jul 16, 2021

	for a_op in op.aliases:
	variants.append(a_op.op)
	variants.append(a_op.method_variant)
	variants.append(a_op.inplace_variant)
	inplace_ops.append(a_op.inplace_variant)

Alias for polygamma #59691

Alias for polygamma #59691

Conversation

krshrimali commented Jun 9, 2021 • edited

facebook-github-bot commented Jun 9, 2021 • edited

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_linux_xenial_py3_clang5_asan_test1 (1/1)

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

krshrimali commented Jun 13, 2021 • edited

kshitij12345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry commented Jun 23, 2021

krshrimali commented Jun 24, 2021

mruberry commented Jun 24, 2021

Choose a reason for hiding this comment

krshrimali Jul 9, 2021 • edited

Choose a reason for hiding this comment

mruberry commented Jul 8, 2021

krshrimali commented Jul 9, 2021

krshrimali commented Jul 12, 2021

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 15, 2021

facebook-github-bot commented Jul 16, 2021

Alias for `polygamma` #59691

Alias for `polygamma` #59691

krshrimali commented Jun 9, 2021 •

edited

facebook-github-bot commented Jun 9, 2021 •

edited

krshrimali commented Jun 13, 2021 •

edited

krshrimali Jul 9, 2021 •

edited