rework device type filter rule #55753
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💊 CI failures summary and remediationsAs of commit 07ac4bd (more details on the Dr. CI page):
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. |
walterddr
force-pushed
the
be_test_expect_and_only_rule_change
branch
2 times, most recently
from
902f965 to
19e49b0
Compare
seemethere
approved these changes
Apr 11, 2021
walterddr
force-pushed
the
be_test_expect_and_only_rule_change
branch
from
19e49b0 to
7e0ca0a
Compare
|
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
walterddr
commented
Apr 12, 2021
seemethere
approved these changes
Apr 12, 2021
mruberry
reviewed
Apr 12, 2021
There was a problem hiding this comment.
There's no list of all available device types because 3rd parties can programmatically extend the bases (like XLA does).
There was a problem hiding this comment.
oh... I see. then this approach is problematic. since 3rd party could also be using common_device_type.py to test their code outside of pytorch core repo
mruberry
reviewed
Apr 12, 2021
There was a problem hiding this comment.
Use filter:
if 'except_for' is not None:
device_types = filter(lambda x: x not in except_for, device_types)
if 'only_for' is not None:
device_types = filter(lambda x: x in only_for, device_types)
return list(device_types)
a81629f to
1f6ecd8
Compare
mruberry
reviewed
Apr 12, 2021
There was a problem hiding this comment.
Hey @walterddr! I made a few inline comments for your review. Looking forward to hearing your thoughts!
There was a problem hiding this comment.
Can't this filter just be applied directly to the list of test bases?
There was a problem hiding this comment.
I would look at applying the filter here so you don't need DEFAULT_DEVICE_TYPE_LIST.
walterddr
force-pushed
the
be_test_expect_and_only_rule_change
branch
from
1f6ecd8 to
2706444
Compare
walterddr
force-pushed
the
be_test_expect_and_only_rule_change
branch
from
2706444 to
07ac4bd
Compare
|
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Codecov Report
@@ Coverage Diff @@
## master #55753 +/- ##
==========================================
- Coverage 77.44% 73.53% -3.92%
==========================================
Files 1896 1896
Lines 187935 187937 +2
==========================================
- Hits 145555 138196 -7359
- Misses 42380 49741 +7361 |
|
@walterddr merged this pull request in d0cd168. |
|
This again broke running tests locally without setting environment variables. |
Closed
walterddr
pushed a commit
to walterddr/pytorch
that referenced
this pull request
Apr 14, 2021
Summary: This is a reflection of recent failures in pytorch#55753 and pytorch#55522. We are lacking a test to safeguard these test env var. Pull Request resolved: pytorch#55931 Test Plan: 1. CI 2. Run locally using `python test/test_testing.py -k test_filtering_env_var -v` - gives failure on 2ca45cb and d0cd168 - passes on 159e110 and current master Reviewed By: jbschlosser Differential Revision: D27747537 Pulled By: walterddr fbshipit-source-id: 9b70470f1cb8cb6440f930e7e52119aed5374dfc
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 15, 2021
Summary: This is a reflection of recent failures in #55753 and #55522. We are lacking a test to safeguard these test env var. Pull Request resolved: #55931 Test Plan: 1. CI 2. Run locally using `python test/test_testing.py -k test_filtering_env_var -v` - gives failure on 2ca45cb and d0cd168 - passes on 159e110 and current master Reviewed By: jbschlosser Differential Revision: D27747537 Pulled By: walterddr fbshipit-source-id: c88e1c818199c7838866037d702d4012cacf510e
krshrimali
pushed a commit
to krshrimali/pytorch
that referenced
this pull request
May 19, 2021
Summary: Currently common_device_type generates device-specific test based on vague rules. see pytorch#55707. This should fix pytorch#55707 # Changes included This PR changes the rule: 1. First user provided args (`except_for` and `only_for`) are processed to filter out desired device type from a ALL_AVAILABLE_LIST 2. Then environment variables are processed the exact same way. tests are generated based on the final filtered list. Pull Request resolved: pytorch#55753 Test Plan: CI Reviewed By: seemethere, ngimel Differential Revision: D27709192 Pulled By: walterddr fbshipit-source-id: 1d5378ef013b22a7fb9fdae24b486730b2e67401
krshrimali
pushed a commit
to krshrimali/pytorch
that referenced
this pull request
May 19, 2021
…orch#55880) Summary: Fixes breakage caused by pytorch#55753 Pull Request resolved: pytorch#55880 Reviewed By: nikithamalgifb Differential Revision: D27735299 Pulled By: mruberry fbshipit-source-id: f8f927f95e4f7fe5f00448ed25d23dac3b7104a4
krshrimali
pushed a commit
to krshrimali/pytorch
that referenced
this pull request
May 19, 2021
Summary: This is a reflection of recent failures in pytorch#55753 and pytorch#55522. We are lacking a test to safeguard these test env var. Pull Request resolved: pytorch#55931 Test Plan: 1. CI 2. Run locally using `python test/test_testing.py -k test_filtering_env_var -v` - gives failure on 2ca45cb and d0cd168 - passes on 159e110 and current master Reviewed By: jbschlosser Differential Revision: D27747537 Pulled By: walterddr fbshipit-source-id: c88e1c818199c7838866037d702d4012cacf510e
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Currently common_device_type generates device-specific test based on vague rules. see #55707.
This should fix #55707
Changes included
This PR changes the rule:
except_forandonly_for) are processed to filter out desired device type from a ALL_AVAILABLE_LISTtests are generated based on the final filtered list.
Test Plan
CI