Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all downloading logic out of common_utils.py #61479

Closed

Conversation

walterddr
Copy link
Contributor

and into tools/ folder

Currently run_tests.py invokes tools/test_selections.py

  1. download and analyze what test_file to run
  2. download and parse S3 stats and pass the info to local files.
  3. common_utils.py uses download S3 stats to determine what test cases to run.

…nd into tools/ folder

Currently it does:
run_tests.py invokes tools/test_selections.py to
1. download and analyze what test_file to run
2. download and parse S3 stats and pass the info to local files.
3. common_utils.py uses download S3 stats to determine what test cases to run.
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 9, 2021

💊 CI failures summary and remediations

As of commit d6c55b8 (more details on the Dr. CI page and at hud.pytorch.org/pr/61479):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


Preview docs built from this PR

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.

@@ -451,6 +452,9 @@ def run_test(test_module, test_directory, options, launcher_cmd=None, extra_unit
# If using pytest, replace -f with equivalent -x
if options.pytest:
unittest_args = [arg if arg != '-f' else '-x' for arg in unittest_args]
elif IS_IN_CI:
# use the downloaded test cases configuration, not supported in pytest
unittest_args.extend(['--import-slow-tests', '--import-disabled-tests'])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not something I think should be fixed in this PR, but the name unittest_args is slightly misleading as we use it for pytest too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a larger pytest/unittest initiative, so I will defer this one to that

Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

Overall great! How do you plan to make sure slow_tests are good?

test/run_test.py Outdated Show resolved Hide resolved
tools/stats/import_test_stats.py Outdated Show resolved Hide resolved
torch/testing/_internal/common_utils.py Show resolved Hide resolved
torch/testing/_internal/common_utils.py Outdated Show resolved Hide resolved
raise unittest.SkipTest("test is fast; we disabled it with PYTORCH_TEST_SKIP_FAST")
check_disabled(str(self))

check_if_enable(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Rong Rong and others added 2 commits July 12, 2021 08:43
Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>
@facebook-github-bot
Copy link
Contributor

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


if TEST_SKIP_FAST:
if not getattr(test, test._testMethodName).__dict__.get('slow_test', False):
raise unittest.SkipTest("test is fast; we disabled it with PYTORCH_TEST_SKIP_FAST")
Copy link
Contributor

Choose a reason for hiding this comment

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

I still do think this should be grouped with lines 866~, but can be done later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no exactly. slow_test can be marked with @slowTest wrapper from common_utils.py as well. so the test_name might not be a key in the slow_tests_dicts

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean like this code itself should come before the disabled stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. that make sense actually . will incorporate in the next batch of refactoring

@facebook-github-bot
Copy link
Contributor

@walterddr merged this pull request in a5a10fe.

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.

None yet

3 participants