Skip to content

Conversation

@ailzhang
Copy link
Contributor

@ailzhang ailzhang commented May 8, 2020

I'm mostly done with cleaning up test/ folder. There're a bunch of remaining callsites but they're "valid" in testing type() functionalities. We cannot remove them until it's fully deprecated.
Next PR would mainly focus on move some callsites to an internal API.

@ailzhang ailzhang requested review from albanD and apaszke as code owners May 8, 2020 18:56
@ailzhang ailzhang requested review from gchanan and ngimel May 8, 2020 19:26
@mruberry mruberry self-requested a review May 8, 2020 19:29
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.

I didn't even know we had assertEqualTypeString()!

Copy link
Collaborator

@albanD albanD 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.
Just random comments.

@dr-ci
Copy link

dr-ci bot commented May 8, 2020

💊 CI failures summary and remediations

As of commit b1dfaa3 (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 on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 6 times.

Copy link
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.

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

Copy link
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.

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

@facebook-github-bot
Copy link
Contributor

@ailzhang merged this pull request in 86d2870.

@mruberry
Copy link
Collaborator

mruberry commented May 9, 2020

Hey @ailzhang, sorry but had to unland this because it broke the slow-test build:

May 09 04:18:27 ======================================================================
May 09 04:18:27 ERROR [0.002s]: test_addmm_cpu (__main__.TestTorchDeviceTypeCPU)
May 09 04:18:27 ----------------------------------------------------------------------
May 09 04:18:27 Traceback (most recent call last):
May 09 04:18:27   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 210, in instantiated_test
May 09 04:18:27     return test(self, device_arg)
May 09 04:18:27   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 440, in wrapper
May 09 04:18:27     fn(*args, **kwargs)
May 09 04:18:27   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 465, in only_fn
May 09 04:18:27     return fn(slf, device, *args, **kwargs)
May 09 04:18:27   File "test_torch.py", line 15062, in test_addmm
May 09 04:18:27     for dtype, prec in types.items():
May 09 04:18:27 AttributeError: module 'types' has no attribute 'items'
May 09 04:18:27 
May 09 04:18:28 ----------------------------------------------------------------------
May 09 04:18:28 Ran 5172 tests in 458.438s
May 09 04:18:28 
May 09 04:18:28 FAILED (errors=1, skipped=5125)

@mruberry
Copy link
Collaborator

Part of addressing #37863.

facebook-github-bot pushed a commit that referenced this pull request May 12, 2020
Summary:
Reland of #38140. It got reverted since it broke slow tests which were only run on master branch(thanks mruberry !). Enabling all CI tests in this PR to make sure they pass.
Pull Request resolved: #38288

Reviewed By: mruberry

Differential Revision: D21524923

Pulled By: ailzhang

fbshipit-source-id: 3a9ecc7461781066499c677249112434b08d2783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants