Adds an OpInfo note #57428

mruberry · 2021-05-02T01:37:47Z

Like the title says. The OpInfo pattern can be confusing when first encountered, so this note links the Developer Wiki and tracking issue, plus elaborates on the goals and structure of the OpInfo pattern.

cc @imaginary-person, who I can't add as a reviewer, unfortunately

facebook-github-bot · 2021-05-02T01:37:52Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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.

kshitij12345

Just asked one question.
LGTM!

Thanks

kshitij12345 · 2021-05-03T07:57:28Z

torch/testing/_internal/common_methods_invocations.py

+# Most OpInfos are collected in the "op_db" sequence, although some specialized
+#   tests have their own OpInfo-based datastructures.
+#
+# OpInfos are intended to replace custom test generators, like "method_tests()"


Is this paragraph needed? I believe gradually (hopefully soon) the mentioned test generators will be removed and will no longer serve as a reference for this paragraph. (Plus this just doesn't describe anything about the usage and details of OpInfo)

pmeier

Awesome write-up! I think this makes it a lot easier for someone starting to work on OpInfo's.

heitorschueroff

Great comment, this is surely going to make it easier for people to get started with OpInfos.

anjali411 · 2021-05-03T14:41:46Z

torch/testing/_internal/common_methods_invocations.py

+#   of metadata related to a PyTorch operator, like torch.add. This metadata
+#   can be separated into three categories:
+#
+#     1) Metadata describing the operator, like "dtypesIfCPU", which describes


Metadata describing the operator

dtypesIfCPU does not quite describe the operator. maybe Metadata defining the operator support?

anjali411 · 2021-05-03T14:43:04Z

torch/testing/_internal/common_methods_invocations.py

+#     1) Metadata describing the operator, like "dtypesIfCPU", which describes
+#          the dtypes the operator supports on the CPU. This metadata
+#          is used by generated tests to understand what they should test
+#          and what the expected results of the test should be.


maybe give the example of ref here to indicate that operators can be compared to a function from another library

oh I see you mention it below.

anjali411 · 2021-05-03T14:48:49Z

torch/testing/_internal/common_methods_invocations.py

+#     2) That an operator's aliases, if any, perform the same computation.
+#     3) That an operator implements its gradient and second-order gradient
+#          correctly.
+#     4) That an operators works when traced or scripted by the jit.


And that functions in eager mode and when scripted produce the same output for a given input.

also tests JIT autodifferentiation and alias remapping

anjali411 · 2021-05-03T14:50:42Z

torch/testing/_internal/common_methods_invocations.py

+#   to verify the operator computes what it's expected to. Even though there
+#   are many linalg ops with OpInfos, for instance, there are still
+#   comprehensive handwritten tests for these operators in test_linalg.py.
+#   What doesn't need to handwritten testing are the propertes listed above,


Suggested change

# What doesn't need to handwritten testing are the propertes listed above,

# What doesn't need to be handwritten are the properties listed above,

@anjali411, that suggested revision seems to convey incorrect information.

What about Validating the aforementioned properties doesn't require handwritten tests?

I'll tweak it; good grammar catch

anjali411 · 2021-05-03T14:51:40Z

torch/testing/_internal/common_methods_invocations.py

+#   are many linalg ops with OpInfos, for instance, there are still
+#   comprehensive handwritten tests for these operators in test_linalg.py.
+#   What doesn't need to handwritten testing are the propertes listed above,
+#   except in rare cases where the generated tests are insufficient or


maybe give an example here?

anjali411 · 2021-05-03T14:52:14Z

torch/testing/_internal/common_methods_invocations.py

+# Adding an OpInfo may seem confusing because the base OpInfo class has
+#   a lot of attributes, but luckily most OpInfos don't require all of them,
+#   or even a majority of them, be changed from their defaults.
+#   Looking at OpInfos for similar operators to the one you're adding


Suggested change

# Looking at OpInfos for similar operators to the one you're adding

# Looking at OpInfos for the operators similar to the one you're adding

anjali411

Left a few comments, but looks great overall. Thanks @mruberry !

anjali411 · 2021-05-03T15:01:53Z

torch/testing/_internal/common_methods_invocations.py

+#   PyTorch's "device generic test framework". See the developer Wiki article
+#   above for more information on this test instantiation process.
+#
+# In addition to the OpInfo base class, there are subclasses of OpInfos, like


It might be useful to list all the OpInfo subclasses in master at the time of writing, to show the breadth of operator coverage.

imaginary-person · 2021-05-03T15:22:50Z

torch/testing/_internal/common_methods_invocations.py

+# Most OpInfos are collected in the "op_db" sequence, although some specialized
+#   tests have their own OpInfo-based datastructures.


although some specialized tests have their own OpInfo-based datastructures

A suggestion: having an example here.

ngimel

Thanks for this note!

ngimel · 2021-05-04T00:07:28Z

torch/testing/_internal/common_methods_invocations.py

+#   automated test coverage by consolidating these lists into one structured and
+#   documented list.
+#
+# Most tests that use every OpInfo are in test_ops.py. More correctly,


every OpInfo in op_db

ngimel · 2021-05-04T00:11:43Z

torch/testing/_internal/common_methods_invocations.py

+#     4) That an operators works when traced or scripted by the jit.
+#     5) That operators supporting out= do so correctly.
+#     6) That operators specify metadata, like which dtypes they support,
+#          properly.


It would make sense to mention here that some (or all?) of these tests run only on the first element of tuple returned by sample_inputs_func

ngimel · 2021-05-04T00:12:49Z

torch/testing/_internal/common_methods_invocations.py

+#     3) That an operator implements its gradient and second-order gradient
+#          correctly.
+#     4) That an operators works when traced or scripted by the jit.
+#     5) That operators supporting out= do so correctly.


link to wiki on out behavior?

ngimel · 2021-05-04T00:17:41Z

torch/testing/_internal/common_methods_invocations.py

+#   a lot of attributes, but luckily most OpInfos don't require all of them,
+#   or even a majority of them, be changed from their defaults.
+#   Looking at OpInfos for similar operators to the one you're adding
+#   is usually a good way to start. OpInfo tests should also be designed so


"Existing OpInfo tests are designed"? Otherwise, this sentence may read as "If you are adding a new OpInfo test, it should be designed...", but this note doesn't give nearly enough information to do that, so don't go there.

ngimel · 2021-05-04T00:21:05Z

torch/testing/_internal/common_methods_invocations.py

+#     2) Test directives, like "skips", which indicates that some generated
+#          tests should be skipped for this operator. Test directives
+#          directly control the behavior of generated tests.
+#     3) A "sample_inputs_func", a function that can generate valid inputs to


sample_inputs_func deserves more comments - does it provide several inputs cases? How one should go about deciding how many input variants to add? How are kwargs handled? How can inputs for inplace operation variants be filtered?

facebook-github-bot · 2021-06-18T05:28:19Z

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

facebook-github-bot · 2021-06-18T09:05:55Z

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

facebook-github-bot · 2021-06-18T09:32:22Z

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

facebook-github-bot · 2021-06-18T10:42:07Z

@mruberry merged this pull request in 5609c2e.

facebook-github-bot added the cla signed label May 2, 2021

mruberry requested review from ngimel, zou3519, kshitij12345, pmeier, albanD, heitorschueroff, anj-s, anjali411 and jbschlosser and removed request for anj-s May 2, 2021 01:38

kshitij12345 approved these changes May 3, 2021

View reviewed changes

pmeier approved these changes May 3, 2021

View reviewed changes

heitorschueroff approved these changes May 3, 2021

View reviewed changes

anjali411 reviewed May 3, 2021

View reviewed changes

imaginary-person reviewed May 3, 2021

View reviewed changes

ngimel approved these changes May 4, 2021

View reviewed changes

mruberry mentioned this pull request May 12, 2021

[OpInfo] Make it easier to add OpInfos #57577

Closed

docs updates

84a12c4

mruberry force-pushed the op_info_comment branch from 48c9c9b to 84a12c4 Compare June 18, 2021 09:05

lint

b7dff0a

facebook-github-bot closed this in 5609c2e Jun 18, 2021

facebook-github-bot added the Merged label Jun 18, 2021

mruberry deleted the op_info_comment branch January 14, 2022 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds an OpInfo note #57428

Adds an OpInfo note #57428

mruberry commented May 2, 2021 •

edited

facebook-github-bot commented May 2, 2021 •

edited

kshitij12345 left a comment

kshitij12345 May 3, 2021

pmeier left a comment

heitorschueroff left a comment

anjali411 May 3, 2021

anjali411 May 3, 2021

anjali411 May 3, 2021

anjali411 May 3, 2021 •

edited

anjali411 May 3, 2021

anjali411 May 3, 2021 •

edited

imaginary-person May 3, 2021 •

edited

mruberry May 3, 2021

anjali411 May 3, 2021

anjali411 May 3, 2021 •

edited

anjali411 left a comment

anjali411 May 3, 2021

imaginary-person May 3, 2021

ngimel left a comment

ngimel May 4, 2021

ngimel May 4, 2021

ngimel May 4, 2021

ngimel May 4, 2021

ngimel May 4, 2021

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

	# What doesn't need to handwritten testing are the propertes listed above,
	# What doesn't need to be handwritten are the properties listed above,

	# Looking at OpInfos for similar operators to the one you're adding
	# Looking at OpInfos for the operators similar to the one you're adding

		# Most OpInfos are collected in the "op_db" sequence, although some specialized
		# tests have their own OpInfo-based datastructures.

Adds an OpInfo note #57428

Adds an OpInfo note #57428

Conversation

mruberry commented May 2, 2021 • edited

facebook-github-bot commented May 2, 2021 • edited

💊 CI failures summary and remediations

kshitij12345 left a 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

heitorschueroff 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

anjali411 May 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjali411 May 3, 2021 • edited

Choose a reason for hiding this comment

imaginary-person May 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjali411 May 3, 2021 • edited

Choose a reason for hiding this comment

anjali411 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngimel 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

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

facebook-github-bot commented Jun 18, 2021

mruberry commented May 2, 2021 •

edited

facebook-github-bot commented May 2, 2021 •

edited

anjali411 May 3, 2021 •

edited

anjali411 May 3, 2021 •

edited

imaginary-person May 3, 2021 •

edited

anjali411 May 3, 2021 •

edited