Repository navigation
feat(resources): typed nodes on base resources - #13
Merged
Merged
Conversation
`[[resources.components]]` has a typed `nodes`, base `[resources]` did not — so in a het recipe the trainer component could declare its node count directly while the fleet, declared in `[resources]`, had to reach for `sbatch_extra = ["--nodes=N"]`. The `sbatch_extra` docstring listed `--nodes` among the things the typed schema doesn't cover, which is the asymmetry rather than a justification for it. A multi-node fleet is the normal shape for RL runs here, not an edge case. Unset emits no directive, so single-node recipes render byte-identical scripts to before. Verified against the scheduler: a fleet of 2 nodes plus a 1-node GPU trainer allocated as one het job, `srun --het-group=0` reaching both fleet nodes (run_01a0157a86e872819d79e8b05888e496). Two unit tests cover the emission and that base and component counts stay independent.
CI's first step is `cargo fmt --all -- --check`, and it failed on the two one-line `assert!`s in `base_and_component_nodes_are_independent`: their argument lists are 62 chars, just past rustfmt's 60-char `fn_call_width`, so rustfmt wants them vertical. Formatted — every other assert in `het_tests` already has that shape. fmt runs before clippy, so nothing after it had executed in CI and both clippy steps were unverified; `cargo clippy --no-deps -- -D warnings` and its `--features ui` variant both pass clean, so no `#[allow]` was needed anywhere. `Resources::nodes` goes from four doc lines to one, matching its counterpart `HetComponent::nodes` (one line) rather than the escape-hatch fields around it. The `sbatch_extra` asymmetry it recounted is history and lives in the PR description; the doc keeps what a reader of the struct needs — the default, and that unset emits no directive at all, which is what makes existing single-node recipes render byte-identical scripts. Same trim on the two-line narration in `base_resources_nodes_is_typed`, where the module's own in-test comments are one-liners. Also rewraps the `sbatch_extra` paragraph, left ragged when `--nodes` came out of its list. No behaviour change; 41 tests pass under `--all-features`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Resources` carries a container-level `#[serde(default)]`, and no sibling Option field repeats it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
[[resources.components]]has a typednodesfield; base[resources]didn't. In a het recipe that means the trainer component can declare its node count directly, while the fleet — declared in[resources]— has to reach for the escape hatch:Resources::sbatch_extra's docstring listed--nodesamong the directives "the typed schema doesn't cover", which reads as justification but is really just the asymmetry. A multi-node fleet is the normal shape for RL runs on this cluster, not an edge case —run_combined_allocation.pyexpects group 0 to be the fleet and group 1 the trainer, and the fleet is usually the wider one.Unset emits no
--nodesdirective, so existing single-node recipes render byte-identical scripts.Verification
Unit tests: emission from the typed field, no directive when unset, and base/component counts independent (
--nodes=2for the fleet alongside--nodes=1for the trainer, exactly two occurrences).cargo test --all-features: 41 passed, 0 failed.Against the real scheduler — a 2-node fleet plus a 1-node GPU trainer as one het job,
srun --het-group=0reaching both fleet nodes:(
run_01a0157a86e872819d79e8b05888e496. The same recipe was first proven viasbatch_extrainrun_01a0157327187af0bca60a52ffe2346e, then switched to the typed field — same allocation, same result.)