Skip to content

[testing] assert no duplicate in method_tests for an OpInfo entry#53492

Closed
kshitij12345 wants to merge 6 commits intopytorch:masterfrom
kshitij12345:testing/duplicate-method-test-entry
Closed

[testing] assert no duplicate in method_tests for an OpInfo entry#53492
kshitij12345 wants to merge 6 commits intopytorch:masterfrom
kshitij12345:testing/duplicate-method-test-entry

Conversation

@kshitij12345
Copy link
Copy Markdown
Collaborator

Assert no duplicate in method_tests for an OpInfo entry

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Mar 8, 2021

💊 CI failures summary and remediations

As of commit 102e20c (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).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 kshitij12345 requested a review from mruberry March 8, 2021 07:23
@kshitij12345 kshitij12345 marked this pull request as ready for review March 8, 2021 07:23
Comment thread test/test_ops.py
# Verifies that ops do not have an entry in
# `method_tests` (legacy testing infra).
@onlyCPU
@ops(op_db, allowed_dtypes=[torch.float32])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Smart implementation to support skips so this PR doesn't have to update all the sample inputs, too. If there were no skips then this could probably just compare the op_db and method_tests() directly.

@mruberry mruberry self-requested a review March 8, 2021 07:41
Copy link
Copy Markdown
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.

Smart work, @kshitij12345. Let's just wait for tests.

@kshitij12345
Copy link
Copy Markdown
Collaborator Author

Note: Failures look irrelevant :)

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@mruberry
Copy link
Copy Markdown
Collaborator

Unfortunately this picked up a merge conflict, likely from the out= update :(. Sorry about that @kshitij12345 -- would you rebase this, please?

@kshitij12345
Copy link
Copy Markdown
Collaborator Author

@mruberry No worries :) Thanks for the heads-up

skips=(
# `test_duplicate_method_tests` doesn't raise any warning, as it doesn't actually
# call the operator.
SkipInfo('TestOpInfo', 'test_duplicate_method_tests'),),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@mruberry, think there is a nicer way to avoid the skip?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, this seems fine, although it is a humorous niche interaction.

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@mruberry
Copy link
Copy Markdown
Collaborator

Unfortunately our internal tools were unable to merge this properly; sorry about that @kshitij12345. Would you please rebase this?

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 14, 2021

Codecov Report

Merging #53492 (102e20c) into master (319ab58) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #53492      +/-   ##
==========================================
- Coverage   77.34%   77.14%   -0.20%     
==========================================
  Files        1887     1887              
  Lines      184777   184777              
==========================================
- Hits       142918   142551     -367     
- Misses      41859    42226     +367     

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@mruberry merged this pull request in df7c0a0.

xsacha pushed a commit to xsacha/pytorch that referenced this pull request Mar 31, 2021
…torch#53492)

Summary:
Assert no duplicate in method_tests for an OpInfo entry

Pull Request resolved: pytorch#53492

Reviewed By: izdeby

Differential Revision: D26882441

Pulled By: mruberry

fbshipit-source-id: f0631ea2b46b74285c76365c679bd45abc917d63
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
…torch#53492)

Summary:
Assert no duplicate in method_tests for an OpInfo entry

Pull Request resolved: pytorch#53492

Reviewed By: izdeby

Differential Revision: D26882441

Pulled By: mruberry

fbshipit-source-id: f0631ea2b46b74285c76365c679bd45abc917d63
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.

4 participants