KEMBAR78
TST Adds inplace checks to module_info by thomasjpfan · Pull Request #63739 · pytorch/pytorch · GitHub
Skip to content

Conversation

@thomasjpfan
Copy link
Contributor

@thomasjpfan thomasjpfan commented Aug 22, 2021

Follow up to #61935

This PR adds inplace checks to test_modules. This version checks the constructor for inplace and performs the check automatically.

cc @albanD @mruberry @jbschlosser @walterddr

@thomasjpfan thomasjpfan added module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: testing Issues related to the torch.testing module (not tests) labels Aug 22, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 22, 2021

🔗 Helpful links

💊 CI failures summary and remediations

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


  • 1/2 failures introduced in this PR
  • 1/2 broken upstream at merge base c6505cc on Sep 01 from 11:30am to 12:50pm

1 failure not recognized by patterns:

Job Step Action
GitHub Actions Lint / mypy Run mypy 🔁 rerun

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

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.

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #63739 (18244c0) into master (c6505cc) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #63739      +/-   ##
==========================================
+ Coverage   66.60%   66.67%   +0.07%     
==========================================
  Files         705      708       +3     
  Lines       92215    92412     +197     
==========================================
+ Hits        61420    61619     +199     
+ Misses      30795    30793       -2     

Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Looking pretty good - couple minor comments below

@modules(module_db)
def test_pickle(self, device, dtype, module_info):
# Test that module can be pickled and unpickled.
def test_check_inplace(self, device, dtype, module_info):
Copy link
Contributor

Choose a reason for hiding this comment

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

@thomasjpfan Hm, looks like a merge conflict happened or something - we've now got 2 definitions for test_check_inplace that the linter is complaining about

Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@jbschlosser merged this pull request in 43c0f03.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: nn Related to torch.nn module: testing Issues related to the torch.testing module (not tests) open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants