-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ci: make CI resilient to pip/uv network timeouts #5118
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,13 +38,50 @@ def is_flaky_failure(concat_allranks_logs: str) -> bool: | |
| or "zmq.error.ZMQError: Address already in use" in concat_allranks_logs | ||
| or "We couldn't connect to 'https://huggingface.co'" in concat_allranks_logs | ||
| or "Unpack failed: incomplete input" in concat_allranks_logs | ||
| or "The read operation timed out" in concat_allranks_logs | ||
| or "Read timed out" in concat_allranks_logs | ||
| or "TimeoutError" in concat_allranks_logs | ||
| or "Connection broken" in concat_allranks_logs | ||
| or "Temporary failure in name resolution" in concat_allranks_logs | ||
| or "unspecified launch failure" in concat_allranks_logs | ||
| or "free(): corrupted unsorted chunks" in concat_allranks_logs | ||
| or "Segfault encountered" in concat_allranks_logs | ||
| or "The following metrics failed" in concat_allranks_logs | ||
| ) | ||
|
|
||
|
|
||
| def _collect_failure_logs(workdir: pathlib.Path) -> list[str]: | ||
| """Reads every log file that may carry a flaky-failure signature. | ||
|
|
||
| The per-rank ``attempt_0/*/std*.log`` files only contain torchrun training | ||
| output. The golden-value comparison runs in ``run_ci_test.sh`` and emits its | ||
| assertion (e.g. ``The following metrics failed``) to the nemo-run task log, | ||
| which lives outside that per-rank tree. Globbing both ensures harness-side | ||
| failures are seen by ``is_flaky_failure`` and therefore eligible for retry. | ||
|
|
||
| Args: | ||
| workdir: Working directory under which nemo-run writes all log files. | ||
|
|
||
| Returns: | ||
| The concatenated lines of every discovered log file, deduplicated by | ||
| resolved path to avoid double-counting overlapping globs. | ||
| """ | ||
| seen_paths = set() | ||
| collected_lines: list[str] = [] | ||
| for pattern in ("**/attempt_0/*/std*.log", "**/*.log"): | ||
| for log_file_path in workdir.glob(pattern): | ||
| resolved = log_file_path.resolve() | ||
| if resolved in seen_paths or not log_file_path.is_file(): | ||
| continue | ||
| seen_paths.add(resolved) | ||
| try: | ||
| with open(log_file_path, "r", errors="replace") as f: | ||
| collected_lines.extend(f.readlines()) | ||
|
Comment on lines
+71
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| except OSError as error: | ||
| logger.warning("Could not read log file %s: %s", log_file_path, error) | ||
| return collected_lines | ||
|
|
||
|
|
||
| @click.command() | ||
| @click.option("--scope", required=True, type=str, help="Scope of the workload") | ||
| @click.option("--model", required=True, type=str, help="Model of the workload") | ||
|
|
@@ -194,12 +231,8 @@ def __getattr__(self, name): | |
| sys.exit(0) | ||
|
|
||
| logger.error(f"Job failed with status: {job_dict['status']}") | ||
| log_file_paths = pathlib.Path(os.getcwd()).glob("**/attempt_0/*/std*.log") | ||
| all_ranks_all_logs = [tee_buffer.getvalue()] | ||
| for log_file_path in log_file_paths: | ||
| with open(log_file_path, "r") as f: | ||
| all_logs = f.readlines() | ||
| all_ranks_all_logs.extend(all_logs) | ||
| all_ranks_all_logs.extend(_collect_failure_logs(pathlib.Path(os.getcwd()))) | ||
| all_ranks_all_logs_string = "\n".join(all_ranks_all_logs) | ||
| if is_flaky_failure(all_ranks_all_logs_string): | ||
| logger.warning("Detected flaky failure, attempt restart.") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"TimeoutError"match is too broadTimeoutErroris a Python builtin that appears in stack traces for any kind of timeout — NCCL collective timeouts, test-harness wait timeouts, asyncio cancellations, custom training deadlines — not just network I/O. Any genuine performance regression that causes a training job to exceed its allotted time will match this string, get classified as flaky, and be silently retried up to the configured number of attempts, masking the regression entirely. The other two new network patterns ("The read operation timed out"and"Read timed out") are already specific enough to catch the pip/uv scenario described in the PR. Consider removing the bare"TimeoutError"or replacing it with a more qualified string such as"TimeoutError: The read operation"to keep it scoped to network I/O.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!