Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 132b512497
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 2. Enable `data.concurrent_compute` and give each simultaneous candidate enough | ||
| loader workers. As a starting invariant, request at least | ||
| `executor.max_concurrent * data.loading.num_workers` CPUs for the allocation, | ||
| plus modest controller overhead. More workers help only until storage or the |
There was a problem hiding this comment.
Account for every distributed loader process
When adaptive data parallelism assigns world_size > 1, every rank creates its own DataLoader with data.loading.num_workers workers, so this formula underestimates CPU demand by the rank multiplicity. For example, two concurrent four-GPU candidates require roughly 8 * num_workers CPUs rather than 2 * num_workers; following the documented invariant can therefore make the utilization smoke CPU-starved. Base the recommendation on the sum of the concurrently planned world sizes, not merely the candidate count.
Useful? React with 👍 / 👎.
| may report every physical GPU in a shared allocation. Candidate progress records | ||
| contain synchronized model-batch seconds and total training seconds; their ratio | ||
| is a reproducible upper bound on compute duty over the recorded training epoch. |
There was a problem hiding this comment.
Normalize recorded GPU seconds before computing duty
For an adaptive run with multiple ranks, the recorded progress/gpu_seconds accumulates batch_seconds * world_size, while epoch_train_seconds is wall-clock time, so their direct ratio can approach world_size rather than 1 and is not a compute-duty bound. Moreover, gpu_seconds is cumulative across epochs while the training duration is per epoch. The guidance should require a per-epoch delta divided by world_size * epoch_train_seconds before using this value for a utilization threshold.
Useful? React with 👍 / 👎.
| Clusters that expose the container runtime through environment modules should | ||
| set `executor.apptainer_module` to the exact loadable module name (for example, | ||
| `singularity-ce/4.3.3` on Torch). The resolved value is exported to every worker |
There was a problem hiding this comment.
Limit apptainer_module guidance to the Slurm executor
This unqualified recommendation fails for the slurm_allocation workflow described immediately above (and for local): their executor config models have no apptainer_module field and reject the extra key during validation. Only the separately submitting slurm executor supports and exports this setting; allocation users must load the runtime module in their batch script, so the text should explicitly scope the field to that executor.
Useful? React with 👍 / 👎.
Summary
This is intentionally stacked on #868 so it can describe the adaptive resource configuration introduced there.
Validation
uvx pre-commit run --files docs/search.md