Skip to content

Opinfos for avg_pooling#64214

Closed
zou3519 wants to merge 11 commits intogh/zou3519/376/basefrom
gh/zou3519/376/head
Closed

Opinfos for avg_pooling#64214
zou3519 wants to merge 11 commits intogh/zou3519/376/basefrom
gh/zou3519/376/head

Conversation

@zou3519
Copy link
Contributor

@zou3519 zou3519 commented Aug 30, 2021

Stack from ghstack:

Added OpInfos for:

  • F.adapative_avg_pool{1, 3}d
  • F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:

  • run tests

Differential Revision: D30667797

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 30, 2021

🔗 Helpful links

💊 CI failures summary and remediations

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


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
GitHub Actions linux-xenial-py3.6-gcc5.4 / test (default, 1, 2, linux.2xlarge) Unknown 🔁 rerun

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.

zou3519 added a commit that referenced this pull request Aug 30, 2021
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

ghstack-source-id: 866a33e
Pull Request resolved: #64214
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

[ghstack-poisoned]
zou3519 added a commit that referenced this pull request Aug 30, 2021
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

ghstack-source-id: 0600152
Pull Request resolved: #64214
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

[ghstack-poisoned]
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

[ghstack-poisoned]
zou3519 added a commit that referenced this pull request Aug 31, 2021
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

ghstack-source-id: e037549
Pull Request resolved: #64214
@zou3519
Copy link
Contributor Author

zou3519 commented Aug 31, 2021

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

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
Copy link
Contributor

@heitorschueroff heitorschueroff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
@pytorch-probot
Copy link

pytorch-probot bot commented Sep 30, 2021

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/pytorch/pytorch/blob/aaa8dee50cfcec2e50940f810cebb900dcf8838f/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default

Workflows Labels (bold enabled) Status
Triggered Workflows
linux-bionic-py3.6-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/noarch, ciflow/xla ✅ triggered
linux-vulkan-bionic-py3.6-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/vulkan ✅ triggered
linux-xenial-cuda11.3-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/default, ciflow/linux ✅ triggered
linux-xenial-py3.6-clang7-asan ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/sanitizers ✅ triggered
linux-xenial-py3.6-clang7-onnx ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/onnx ✅ triggered
linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
linux-xenial-py3.6-gcc7-bazel-test ciflow/all, ciflow/bazel, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
periodic-pytorch-linux-xenial-cuda10.2-cudnn7-py3-gcc7-slow-gradcheck ciflow/all, ciflow/cuda, ciflow/default, ciflow/linux, ciflow/slow, ciflow/slow-gradcheck ✅ triggered
win-vs2019-cpu-py3 ciflow/all, ciflow/cpu, ciflow/default, ciflow/win ✅ triggered
win-vs2019-cuda11.3-py3 ciflow/all, ciflow/cuda, ciflow/default, ciflow/win ✅ triggered
Skipped Workflows
libtorch-linux-xenial-cuda10.2-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux 🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux 🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7 ciflow/all, ciflow/cuda, ciflow/linux, ciflow/slow 🚫 skipped
linux-xenial-cuda10.2-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/linux, ciflow/slow 🚫 skipped
parallelnative-linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux 🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-linux-xenial-cuda11.1-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-win-vs2019-cuda11.1-py3 ciflow/all, ciflow/cuda, ciflow/scheduled, ciflow/win 🚫 skipped
puretorch-linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux 🚫 skipped

You can add a comment to the PR and tag @pytorchbot with the following commands:
# ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun

# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow

For more information, please take a look at the CI Flow Wiki.

@zou3519 zou3519 requested review from mruberry and pmeier and removed request for mruberry October 4, 2021 19:20
@zou3519
Copy link
Contributor Author

zou3519 commented Oct 4, 2021

Re-requesting review because it has been a while since the last review

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks good, but I have a few comments inline to make this more readable.

dtypes=floating_types(),
dtypesIfCUDA=floating_types_and(torch.half, torch.bfloat16),
skips=(
DecorateInfo(unittest.skip("Skipped!"), 'TestJit', 'test_variant_consistency_jit'),
Copy link
Collaborator

@pmeier pmeier Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other OpInfo's always include the dtypes to be skipped here

Suggested change
DecorateInfo(unittest.skip("Skipped!"), 'TestJit', 'test_variant_consistency_jit'),
DecorateInfo(unittest.skip("Skipped!"), 'TestJit', 'test_variant_consistency_jit', dtypes=(torch.float32,)),

This comment applies to all occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell test_variant_consistency_jit only runs on float32 dtype. However, if it ran on more dtypes (if someone decides to turn that on in the future), then this test will definitely fail because the JIT has problems with this operator (and not just the (operator, dtype) combination). I'd prefer to leave the skip without specifying the dtype here, what do you think?

(Also, it looks like there are OpInfos that don't include the dtypes to be skipped, like

# TODO: review with var_mean tests in test_autograd.py
DecorateInfo(unittest.skip("Skipped!"), 'TestJit', 'test_variant_consistency_jit'),
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping a general skip is safer. A comment explaining the skip would be nice, though ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining the skip would be nice, though ;)

A comment doesn't hurt, but this is the test that is failing for a lot of operators. This is most likely an error in the test rather than in the operator. Do we have functionality to xfail a test?

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
@zou3519 zou3519 requested a review from pmeier October 6, 2021 15:46
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I didn't review the sample inputs thoroughly for coverage (if you'd like to let's take some time briefly together to go over them, @zou3519)

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
@zou3519
Copy link
Contributor Author

zou3519 commented Oct 6, 2021

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

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
@zou3519
Copy link
Contributor Author

zou3519 commented Oct 6, 2021

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

Added OpInfos for:
- F.adapative_avg_pool{1, 3}d
- F.avg_pool{1, 3}d

The 2d variants already had OpInfos.

Test Plan:
- run tests

Differential Revision: [D30667797](https://our.internmc.facebook.com/intern/diff/D30667797)

[ghstack-poisoned]
@zou3519
Copy link
Contributor Author

zou3519 commented Oct 7, 2021

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

@facebook-github-bot facebook-github-bot deleted the gh/zou3519/376/head branch October 12, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants