fix(resources): derive DefaultTraining from the contract floor, not an 8Gi literal (backend#2254)

[aptracebloc]

Closes tracebloc/backend#2254

What & why

resources show reports DefaultTraining as the effective per-run ceiling when a release carries no RESOURCE_* env. It was the hard-coded cpu=2,memory=8Gi, which exceeds a default Docker Desktop VM (8 GiB out of ~7.7 GiB allocatable, unschedulable once the 1c/3Gi platform overhead is reserved). So show reported a number that can never schedule — and it was a second hand-typed copy of a literal that also lives in client-runtime and the installers.

This derives it from the embedded envelope_contract.json floor instead (cpu=1,memory=2Gi today). It therefore cannot drift from client-runtime's own DEFAULT_JOB_RESOURCES fallback — both are now the same contract floor, and the two repos' contracts are kept byte-identical by the existing cross-repo drift gate. The floor is by construction the largest default that still fits the smallest host we support.

Paired with the client-runtime half (client-runtime#406); both are backend#2254.

Tests (acceptance criteria)

• TestDefaultTraining_IsTheContractFloor — drift gate (default ⇔ embedded contract floor).
• TestDefaultTraining_FitsADefaultDockerDesktop — FitsNode against the ticket's 11 CPU / 7.7 GiB env. Red before this change (8Gi needs 11Gi after overhead > 7.7Gi; 2Gi needs 5Gi ≤ 7.7Gi).
• TestDefaultTraining_FitsTheSmallestSupportedHost — the tight 5 GiB case.
• TestDefaultTraining_ReddensBelowDefaultPlusOverhead — mutation: reddens below default+overhead.

go build, go vet, golangci-lint (0 issues), and the full go test ./... are green.

Note

DefaultTraining changes from a const to a func (it now reads the contract). Both in-tree call sites are updated; it's an internal/ symbol, so there are no out-of-module consumers.

🤖 Generated with Claude Code

---

Note

Low Risk
Display and fallback parsing only for unset resource env; behavior is contract-driven and heavily tested, with no auth or data-path changes.

Overview
Fixes misleading per-run defaults in tracebloc resources show when a release has no RESOURCE_* env: the reported ceiling drops from the old hand-typed cpu=2,memory=8Gi (often unschedulable on default Docker Desktop) to the embedded contract floor (1 CPU · 2 GiB today), aligned with jobs-manager / client-runtime’s DEFAULT_JOB_RESOURCES.

DefaultTraining becomes DefaultTraining(), built from envelope_contract.json via mustContract().Floor so the CLI cannot drift from the shared contract. ParseTraining uses that fallback for empty or unparseable env; verbose resources show copy now says jobs-manager falls back to that value instead of “chart default.”

Adds regression tests (contract-floor drift gate, FitsNode on Docker Desktop / 5 GiB minimum host, and “must not fit” below default+overhead). Version 0.10.14.

^{Reviewed by Cursor Bugbot for commit 7d36f67. Bugbot is set up for automated code reviews on this repo. Configure here.}

[shujaatTracebloc]

Reviewed at 7d36f671. Commenting rather than approving only because CI is mid-flight on the head you just pushed — the substance is right, and I'll approve on the next pass once the rollup lands green. Nothing below is a blocker.

Both red checks were yours, and you already fixed both

For the record, since I read them at the previous head 525df942 and they're stale:

• version-bump-gate / version-check — VERSION was still 0.10.13, already released, against a PR touching internal/*. Fixed in 7d36f671 (0.10.13 → 0.10.14).
• set-status / closing-ref — at the time it ran, closingIssuesReferences was empty. It is now 1 → tracebloc/backend#2254, so the body edit took. Worth knowing for anyone reading this later: the cross-repo form does create the link, contrary to the usual same-repo-only folklore — I checked the graph rather than assuming, and Closes tracebloc/backend#2254 resolves. The gate's own advice is right that a bare Closes #2254 would have bound to cli.

Both of those re-run on the push, so I expect them green.

The change itself

const → func() is the correct call and your comment gives the correct reason. A const can be copied into a second literal somewhere else and nothing notices; a function that reads mustContract().Floor cannot be shadowed without deleting the derivation. That's the same reasoning Overhead() already encodes, so this is consistent with the package rather than novel.

I checked the load-bearing claim in the description — "cannot drift from client-runtime's DEFAULT_JOB_RESOURCES, the two repos' contracts are kept byte-identical by the existing cross-repo drift gate" — rather than taking it on trust, because the entire argument for deriving rather than hardcoding rests on it. It holds: .github/workflows/envelope-contract-drift.yml pins the upstream ref, mints a scoped token for the private repo, does a plain diff -u of the vendored contract against upstream, and fails closed when it cannot read upstream instead of warning and exiting 0. So the derivation really is anchored, not just conventionally aligned.

On the tests, against the house bar:

• TestDefaultTraining_IsTheContractFloor is derived, not restated — it reads mustContract().Floor and compares against parsed DefaultTraining(), so a floor change in envelope_contract.json flows through instead of reddening a hand-typed cpu=1,memory=2Gi. This is the thing most drift gates get wrong, and it's right here.
• TestDefaultTraining_ReddensBelowDefaultPlusOverhead is the anti-vacuity mutation that makes the other three mean something. Without it, three "it fits" assertions would all still pass if FitsNode were stubbed to true.
• TestDefaultTraining_FitsADefaultDockerDesktop reproduces the ticket's actual environment and is genuinely red before the change. That's an acceptance test, not a restatement.

I reviewed the client-runtime half (#406) last pass; the two gates are the same shape against the same contract, which is what you'd want from a paired change.

One finding — stale comment, non-blocking

internal/cli/resources_set.go:415 still asserts the old value as present fact:

current is the cluster-wide ceiling (chart default 8Gi)

After this PR, when no RESOURCE_* env is set, current resolves through ParseTraining to DefaultTraining() — the contract floor, 2Gi. So the parenthetical now contradicts the symbol this PR just changed.

No runtime impact, it's a comment. I'm raising it because it's precisely the failure mode this PR exists to remove: a second copy of 8Gi stated as fact somewhere the derivation can't reach. The drift gate protects the value; nothing protects a prose claim about the value. The WSL2 field case in the same sentence ("node ~6.7 GiB under a lingering 8Gi ceiling") is still valid as history and worth keeping — it's only the present-tense "chart default 8Gi" that's now wrong.

The two 8Gi hits in internal/helm/upgrade.go (:17, :141) I checked and deliberately did not flag: they illustrate the --set comma footgun, and cpu=1,memory=2Gi still contains a comma, so the illustration survives the change.

Carry-over from the client-runtime half

Not this PR's to fix, flagging so it doesn't fall between the two halves: client/scripts/lib/install-client-helm.sh still writes cpu=2,memory=8Gi explicitly on the |unschedulable path (:582) and the cluster-unreadable path (:589), from _TRAINING_DEFAULT at :49. Those are the two paths where the installer can't size to the machine and falls back to the literal — which is exactly the unschedulable-on-a-small-host case backend#2254 is about. The Go and Python halves are now both on the floor; the bash installer isn't. Worth a third issue on the epic rather than scope creep here.

[shujaatTracebloc]

Green now — approving as I said I would.

All checks pass at 7d36f671, including the two that were red when I commented: version-bump-gate (you bumped VERSION to 0.10.14) and set-status / closing-ref (the link graph resolves to tracebloc/backend#2254). Nothing else changed, so my read stands: the const → func() derivation is anchored to a drift gate I verified is real and fails closed, and the four tests hold the bar — the drift gate reads the contract rather than restating the floor, and the mutation test keeps the three "it fits" assertions from being vacuous.

The one finding I left is non-blocking and yours to take or leave: internal/cli/resources_set.go:415 still says "chart default 8Gi" in prose, which this PR makes false. It's a comment, so I'm not holding an approval over it — but it's the same species of stale copy the PR exists to kill, and the drift gate can't see prose.

Separately, and not for this PR: client/scripts/lib/install-client-helm.sh still writes the 8Gi literal on its two fallback paths (:582 unschedulable, :589 cluster unreadable, from _TRAINING_DEFAULT at :49). With this and #406 merged, the bash installer is the last piece of the fleet still on the old default — worth its own issue under the epic.

[shujaatTracebloc]

Not retracting my approval and not asking for a re-review — this is a note worth having on the PR while it's still open, because it's cheaper to state here than to rediscover later.

A parallel review of the DDP work (tracebloc-engine#740) surfaced a constraint on what "the contract floor" now means, and it's been written up as item 7 of the per-rank audit checklist on backend#2224. Paraphrasing it here so this PR carries it:

Under replication, N ranks live inside one job's envelope. The memory request is per-job while the consumption is per-rank — each rank holds a full replica plus its own gradients and optimizer state. engine#732 divides the CPU quota across ranks; nothing divides memory. So after this change the floor is precisely defined for one rank's worth of work, and the two halves of the same envelope mean different things.

Nothing here is wrong. The floor genuinely is the largest default that fits the smallest host we support, which is what this PR set out to fix, and at world_size == 1 it's exactly right. The gap is that the constraint is now load-bearing and unstated: a future reader sees a number derived from a contract and has no reason to suspect it assumes a single rank.

Concretely, and entirely your call since it's your PR: a sentence in the DefaultTraining doc comment saying the floor is per-rank-blind — sized for one rank, not divided under replication — would put the constraint where the derivation is. That's the same argument the comment already makes for why this is a function rather than a const: the value lives in one place, so the caveat should too.

Deciding whether the envelope is sized per-job or per-rank is a real decision and explicitly out of scope here — it's item 7's own ask, flagged there as arguably a blocker before the DDP default flips. I'm only suggesting you record the assumption, not resolve it.

One correction for anyone following the link: backend#2224 item 7 currently states this PR and its client-runtime half were "both merged today." Both are approved and still open as of this comment.