[DO NOT SUBMIT] Benchmark upgrades for ray train#64294
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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 introduces a new config-driven benchmark harness for Ray Train workloads, featuring launchers for both Ray Train and torchrun, a DeepSpeed framework adapter, and causal-LM dataloaders. The feedback focuses on improving the robustness of the harness. Key recommendations include properly tracking and utilizing the resolved precision during hardware-based fallbacks to ensure accurate MFU calculations, preventing potential runtime errors (such as UnboundLocalError and FileNotFoundError) in the training loop and launchers, and making the CLI configuration overrides parsing more resilient to nested key conflicts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Signed-off-by: Lehui Liu <lehui@anyscale.com>
Address code-review issues in the Ray Train benchmark harness: - torchrun_ray: drop final dist.barrier() and tear down in finally, so a failing rank no longer deadlocks healthy ranks (ray.get would hang). - RayTrainContext.get_checkpoint_dir: use to_directory() instead of as_directory().__enter__() with no __exit__ (temp-dir leak). - FrameworkAdapter.run typed -> Dict[str, Any]; the return value is the torchrun_ray metrics path, so silence-on-None can't break future adapters. - deepspeed adapter: track last_loss + require num_steps >= 1 (no NameError on a 0-step run); require data.seq_len (no None * int TypeError). - _first_attr: use `is not None` so a legitimate top_k/num_experts of 0 isn't skipped (silent MFU error). - free_port: set SO_REUSEADDR and document the inherent TOCTOU window. - ray_launcher: replace private ray.train.v2._internal.util.date_str with a stdlib datetime timestamp. - runner.write_results: reuse shared_storage_root() instead of copy-paste. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| if precision == "bf16": | ||
| if torch.cuda.is_available() and not torch.cuda.is_bf16_supported(): | ||
| logger.warning("bf16 unsupported on this GPU; falling back to fp16.") | ||
| return {"fp16": {"enabled": True}} |
There was a problem hiding this comment.
MFU peak ignores precision fallback
Medium Severity
When bf16 is configured but the GPU lacks bf16 support, _precision_config trains in fp16 while get_gpu_peak_flops still uses cfg.model.precision (bf16). Peak FLOPs lookup then returns None, so train/mfu is omitted even though training and peak tables support fp16 on that hardware.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bdeb0d5. Configure here.
Signed-off-by: Lehui Liu <lehui@anyscale.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit fa77eae. Configure here.
| logger.error(f"Cell {name} failed: {type(e).__name__}: {e}") | ||
| failed.append(name) | ||
| if not args.continue_on_error: | ||
| raise |
There was a problem hiding this comment.
Continue on error never disables
Low Severity
--continue-on-error uses action="store_true" with default=True, so args.continue_on_error is always true and the branch that re-raises after a failed cell never runs.
Reviewed by Cursor Bugbot for commit fa77eae. Configure here.
| cfg, topology, world_size, master_addr, master_port, env | ||
| ) | ||
|
|
||
| actors, pg = create_gpu_actor_group(_TorchDistWorker, cfg.scaling.num_workers) |
There was a problem hiding this comment.
Torchrun ignores scaling config
Medium Severity
The torchrun_ray launcher always calls create_gpu_actor_group with only num_workers, so scaling.resources_per_worker and scaling.accelerator_type from the experiment YAML are ignored unlike the Ray Train launcher.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fa77eae. Configure here.
Rebased additions from the pre-rework branch onto the updated PR ray-project#64294 harness: - metrics: HFU (hardware FLOPs incl. activation-recompute / peak) next to MFU; GPU memory capacity next to the allocator peaks; L40S peak corrected to the dense 181.05 TFLOPS (362 is the 2:4-sparsity figure). - deepspeed adapter: catches CUDA OOM and reports an oom=true result row (for batch/seq sweeps) instead of crashing; echoes launcher, dataloader, DP/TP/PP/CP degrees, ZeRO offload flags, and global batch tokens. - collect.py: new "benchmark" view — the 30-column report-row schema (identity, MFU/HFU, throughput, parallelism degrees, memory, OOM). - qwen3_32b_deepspeed experiment + release test, migrated to the new config schema. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
No description provided.