Skip to content

feat(training): add NVIDIA training delivery - #14

Merged
waple0820 merged 3 commits into
mainfrom
agent/nvidia-training-main
Jul 14, 2026
Merged

feat(training): add NVIDIA training delivery#14
waple0820 merged 3 commits into
mainfrom
agent/nvidia-training-main

Conversation

@waple0820

@waple0820 waple0820 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Scope

Moves the verified training chain into main as a self-contained NVIDIA delivery package, without merging unrelated divergent evaluation history from sxh-train.

Delivery behavior

  • Defaults to 8 nodes x 8 GPUs; --nodes, --gpus-per-node, and --gpu-family select the actual target topology.
  • Carries the validated 60-second Lexmount session-create timeout and late-session close/delete cleanup.
  • Fails closed on topology, GPU-family, NCCL, shared-storage, browser, model, and comparison-contract mismatches.
  • Emits manifests, reward/loss reports, and per-node CPU/RAM/GPU/disk/network samples.
  • Requires outbound DNS/HTTPS access to NGC, PyPI, Hugging Face, Lexmount, judge APIs, and live WebVoyager sites.
  • Ignores private secrets.env; no credential values are committed.

Local validation

  • ruff format --check training/nvidia/scripts tests/training
  • ruff check training/nvidia/scripts tests/training
  • python3 -m py_compile training/nvidia/scripts/*.py tests/training/test_nvidia_delivery.py
  • python3 -m unittest tests.training.test_nvidia_delivery
  • Shell syntax checks for every NVIDIA launcher script

No multi-node cluster run was attempted from this workstation.

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @waple0820's task in 2m 44s —— View job


PR Review

  • Gather context
  • Review key files for bugs and security issues
  • Post findings

Findings

Bug: Broken regex in smoke_lexmount_cdp.py:132 — network error detection is silently disabled

# Line 132 — current (broken)
error = re.search(r"\\bERR_[A-Z_]+\\b", text)

The raw string r"\\b" passes two characters (\ + b) to the regex engine, which interprets them as a literal backslash followed by bnot a word-boundary metacharacter. Chrome error codes like ERR_TUNNEL_CONNECTION_FAILED will never be matched. The comment directly above (line 108) says "a preflight cannot falsely pass on ERR_TUNNEL_CONNECTION_FAILED", but that is exactly what happens with this regex. The check always returns None, so the if url.startswith("chrome-error://") or error: branch on line 133 only catches the explicit chrome-error:// prefix — any ERR_ code on a normal-looking URL silently passes.

# Fix
error = re.search(r"\bERR_[A-Z_]+\b", text)

Fix this →


Low: --resume PATH argument is unvalidated and interpolated bare into run.env

run_nvidia.sh accepts --resume PATH with no character validation (contrast with --run-id which enforces ^[A-Za-z0-9][A-Za-z0-9._-]*$). The value reaches slurm_entrypoint.sh's heredoc:

# slurm_entrypoint.sh:41
export LEXBROWSER_RESUME_FROM='${LEXBROWSER_RESUME_FROM:-}'

A path containing a single quote (e.g. /checkpoints/it's-here) breaks the generated run.env shell syntax. The blast radius is a failed run, not privilege escalation, but the inconsistency with other validated arguments is worth fixing.


No other bugs or security issues found. Credential handling (mode-600 check, unsetting vars before sbatch, secrets-only-in-driver pattern) is solid. No shell=True subprocess calls. The manifest write/replace pattern correctly uses an atomic rename.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated Claude review completed. Review run: https://github.com/lexmount/lexmount-browser-lab/actions/runs/29324114760. Please check Claude's inline feedback and summary comment for details.

@waple0820
waple0820 marked this pull request as ready for review July 14, 2026 10:22
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @waple0820's task in 2m 21s —— View job


PR Review

  • Gather context
  • Check diff for new changes since last review
  • Review key files for bugs and security issues
  • Post findings

Previous findings — fixed in 44b3532

Both issues from the prior review are resolved:

  • Regex (smoke_lexmount_cdp.py:143): r"\bERR_[A-Z_]+\b" — word-boundary metacharacters now correct.
  • Heredoc injection (slurm_entrypoint.sh:31-42): printf '%q' shell-quotes LEXBROWSER_RESUME_FROM before embedding it in run.env. Correct.
  • Path validation (run_nvidia.sh:64-67): ^/[A-Za-z0-9][A-Za-z0-9._/-]*$ guard added for --resume. Correct.

New finding: NCCL_IB_HCA can break --export string parsing (run_nvidia.sh:127)

# run_nvidia.sh:127 — current
export_vars="...,NCCL_SOCKET_IFNAME=${NCCL_SOCKET_IFNAME:-},NCCL_IB_HCA=${NCCL_IB_HCA:-},..."
sbatch_args=(... --export="$export_vars")

NCCL_IB_HCA commonly takes values with embedded commas on multi-HCA nodes (e.g. mlx5_0:1,mlx5_1:1). Slurm's --export uses commas as its delimiter, so that value splits into two tokens: NCCL_IB_HCA=mlx5_0:1 and mlx5_1:1. Depending on the Slurm version, this either causes a submission error or silently passes malformed NCCL configuration into the job, which can produce NCCL hangs or severely degraded multi-node bandwidth.

NCCL_SOCKET_IFNAME has the same risk (e.g. eth0,eth1).

Suggested fix — use --export=ALL and instead unset unwanted variables explicitly, or pass NCCL overrides through a run.env entry written in slurm_entrypoint.sh (where printf '%q' already protects the resume path), rather than through the --export CSV.

Fix this →


No other bugs or security issues found.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated Claude review completed. Review run: https://github.com/lexmount/lexmount-browser-lab/actions/runs/29327019875. Please check Claude's inline feedback and summary comment for details.

@waple0820
waple0820 merged commit 354635f into main Jul 14, 2026
1 check passed
@waple0820
waple0820 deleted the agent/nvidia-training-main branch July 14, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant