Refs/heads/add nightly test multi gpu configs #12870
Conversation
- Add nightly-4-gpu, nightly-8-gpu-h200, and nightly-8-gpu-h20 suites to run_suite.py - Update workflow to use nightly-* suite names instead of per-commit-* - per-commit suites are for PR testing, nightly suites are for scheduled nightly tests
- Add --continue-on-error flag to run_suite.py - Update test_utils.py to use flag instead of assert/break - Apply flag to all nightly test jobs in workflow
alisonshao
requested review from
Fridge003,
ispobock and
merrymercy
as code owners
Summary of ChangesHello @alisonshao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to the test execution framework, primarily by enabling more robust and informative nightly test runs. It allows tests to continue running despite individual failures, which is crucial for gathering complete test coverage data. Additionally, it enhances test reporting with detailed summaries and lays the groundwork for expanding multi-GPU testing capabilities. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the test runner by introducing a continue_on_error flag, which is particularly useful for nightly tests to gather all failures instead of stopping at the first one. The implementation is solid, and the addition of a detailed test summary at the end significantly improves the clarity of test results. I have one suggestion to refactor the failure handling logic to reduce code duplication, which should improve maintainability.
| ret_code = run_with_timeout( | ||
| run_one_file, args=(filename,), timeout=timeout_per_file | ||
| ) | ||
| assert ( | ||
| ret_code == 0 | ||
| ), f"expected return code 0, but {filename} returned {ret_code}" | ||
| if ret_code != 0: | ||
| print( | ||
| f"\n✗ FAILED: {filename} returned exit code {ret_code}\n", | ||
| flush=True, | ||
| ) | ||
| success = False | ||
| failed_tests.append((filename, f"exit code {ret_code}")) | ||
| if not continue_on_error: | ||
| # Stop at first failure for PR tests | ||
| break | ||
| # Otherwise continue to next test for nightly tests | ||
| else: | ||
| passed_tests.append(filename) | ||
| except TimeoutError: | ||
| kill_process_tree(process.pid) | ||
| time.sleep(5) | ||
| print( | ||
| f"\nTimeout after {timeout_per_file} seconds when running {filename}\n", | ||
| f"\n✗ TIMEOUT: {filename} after {timeout_per_file} seconds\n", | ||
| flush=True, | ||
| ) | ||
| success = False | ||
| break | ||
| failed_tests.append((filename, f"timeout after {timeout_per_file}s")) | ||
| if not continue_on_error: | ||
| # Stop at first timeout for PR tests | ||
| break | ||
| # Otherwise continue to next test for nightly tests |
There was a problem hiding this comment.
The failure handling logic for non-zero exit codes and timeouts is very similar and contains duplicated code. You can refactor this to reduce duplication and improve readability by handling the failure condition after the try...except block.
failure_reason = None
try:
ret_code = run_with_timeout(
run_one_file, args=(filename,), timeout=timeout_per_file
)
if ret_code == 0:
passed_tests.append(filename)
else:
print(
f"\n✗ FAILED: {filename} returned exit code {ret_code}\n",
flush=True,
)
failure_reason = f"exit code {ret_code}"
except TimeoutError:
kill_process_tree(process.pid)
time.sleep(5)
print(
f"\n✗ TIMEOUT: {filename} after {timeout_per_file} seconds\n",
flush=True,
)
failure_reason = f"timeout after {timeout_per_file}s"
if failure_reason:
success = False
failed_tests.append((filename, failure_reason))
if not continue_on_error:
break
|
Please fix lint with |
Motivation
Modifications
Accuracy Tests
Benchmarking and Profiling
Checklist