resources constrains#1425
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR delivers a significant architectural improvement for compute resource configuration. It introduces a connection-level pool for resources, highly expressive constraints (group, absolute floor/ceilings, and aggregates), and excellent documentation with automated tests validating the examples. The code quality is excellent. Only one minor bug related to reading inUse state for the free tier was found.
Comments:
• [WARNING][bug] When resolving inUse for a resource in bumpGroupToFloor, it currently always defaults to the paid env.resources array, even when isFree is true. This could cause the sort heuristic to read from the wrong tier, potentially leading to suboptimal resource allocation for free jobs. Consider resolving envResources conditionally.
protected bumpGroupToFloor(
resources: ComputeResourceRequest[],
env: ComputeEnvironment,
ids: string[],
deficit: number,
isFree: boolean
): void {
let remaining = deficit
+ const envResources = isFree ? (env.free?.resources ?? []) : (env.resources ?? [])
const candidates = ids
.map((id) => {
const current = this.getResourceRequest(resources, id) ?? 0
const { max } = this.getMaxMinResource(id, env, isFree)
- const inUse = this.getResource(env.resources, id)?.inUse ?? 0
+ const inUse = this.getResource(envResources, id)?.inUse ?? 0
return { id, current, headroom: max - current, inUse }
})• [INFO][style] Since constraint.id is typed as ComputeResourceType | undefined (which structurally includes any in your type definitions), TypeScript implicitly allows assigning it to string[] or passing it to getMaxMinResource without complaining. Adding an explicit cast makes the contract clearer, especially since Zod already guarantees its presence when isGroup is false.
- const targetIds = isGroup
- ? this.getGroupResourceIds(env, constraint.type!, isFree)
- : [constraint.id]
+ const targetIds = isGroup
+ ? this.getGroupResourceIds(env, constraint.type!, isFree)
+ : [constraint.id as string]• [INFO][other] The documentation here is truly exceptional. Consolidating the CPU/GPU/pricing setup, thoroughly explaining the constraint behavior, and backing up the "Full example" with an actual CI validation test (in compute.test.ts) is a fantastic engineering practice. Excellent work!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/@types/C2D/C2D.ts (1)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReword "absolute aggregate" to avoid confusion with the
aggregatefield.Lines 39-40 use "absolute aggregate" to describe
perUnit:falsesemantics, but this same interface also has an unrelatedaggregate:truefield two lines below (line 42). The similar wording risks readers conflating the two distinct concepts.✏️ Suggested wording
- min?: number // min units of the constrained resource; per unit of parent when `perUnit` (default), absolute aggregate when `perUnit:false` - max?: number // max units of the constrained resource; per unit of parent when `perUnit` (default), absolute aggregate when `perUnit:false` + min?: number // min units of the constrained resource; per unit of parent when `perUnit` (default), absolute total when `perUnit:false` + max?: number // max units of the constrained resource; per unit of parent when `perUnit` (default), absolute total when `perUnit:false`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/`@types/C2D/C2D.ts around lines 39 - 40, Reword the comments on the min and max properties in the C2D interface to describe perUnit:false as a total or overall amount, avoiding the term “aggregate” so it cannot be confused with the separate aggregate field.src/components/c2d/compute_engine_base.ts (1)
342-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the group/single-id target resolution into a small helper.
The
isGroup ? ... : ...branch is repeated four times back-to-back to computetargetIds,targetLabel,targetMax, andcurrentAmount. Extracting this into a single helper (e.g. returning{ targetIds, targetLabel, targetMax, currentAmount }) would reduce duplication and make future changes to group-resolution logic single-sourced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/c2d/compute_engine_base.ts` around lines 342 - 358, The repeated group-versus-single target resolution in the constraint-processing method should be centralized. Extract the logic currently used for targetIds, targetLabel, targetMax, and currentAmount into a small helper returning those values, then consume the helper’s result while preserving the existing behavior and dynamic currentAmount lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/compute.md`:
- Around line 137-138: Update the GPU identity rules in the compute
documentation to be platform-neutral: require a unique resource id for every
physical GPU, but scope exactly-one DeviceID to NVIDIA deviceRequests and
acknowledge platform-specific mappings such as AMD and Intel
init.advanced.Devices.
- Line 72: Add the text language identifier to the fenced Markdown blocks for
the tree and cost-output examples in the documentation, including both
occurrences, so each fence uses a recognized language tag instead of an
unqualified fence.
---
Nitpick comments:
In `@src/`@types/C2D/C2D.ts:
- Around line 39-40: Reword the comments on the min and max properties in the
C2D interface to describe perUnit:false as a total or overall amount, avoiding
the term “aggregate” so it cannot be confused with the separate aggregate field.
In `@src/components/c2d/compute_engine_base.ts`:
- Around line 342-358: The repeated group-versus-single target resolution in the
constraint-processing method should be centralized. Extract the logic currently
used for targetIds, targetLabel, targetMax, and currentAmount into a small
helper returning those values, then consume the helper’s result while preserving
the existing behavior and dynamic currentAmount lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fad70ace-f734-42c8-9201-bb85deeb8f16
📒 Files selected for processing (11)
CLAUDE.mdREADME.mddocs/GPU.mddocs/compute-pricing.mddocs/compute.mddocs/env.mddocs/services.mdsrc/@types/C2D/C2D.tssrc/components/c2d/compute_engine_base.tssrc/test/unit/compute.test.tssrc/utils/config/schemas.ts
💤 Files with no reviewable changes (2)
- docs/compute-pricing.md
- docs/GPU.md
|
|
||
| ## Configuration layout | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add languages to the fenced Markdown blocks.
The tree and cost-output fences need identifiers to satisfy Markdown linting.
Proposed fix
-```
+```textAlso applies to: 920-920
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/compute.md` at line 72, Add the text language identifier to the fenced
Markdown blocks for the tree and cost-output examples in the documentation,
including both occurrences, so each fence uses a recognized language tag instead
of an unqualified fence.
Source: Linters/SAST tools
| - Each physical GPU is its own resource with a unique `id` and exactly **one** `DeviceID`. | ||
| - `kind: "discrete"` (non-fungible): only one job at a time can use the device. This is the |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the GPU identity rule platform-neutral.
This says every physical GPU must have exactly one DeviceID, but the AMD and Intel examples use init.advanced.Devices instead. Restrict DeviceID to NVIDIA deviceRequests, or describe the requirement as a unique resource ID plus platform-specific device mapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/compute.md` around lines 137 - 138, Update the GPU identity rules in the
compute documentation to be platform-neutral: require a unique resource id for
every physical GPU, but scope exactly-one DeviceID to NVIDIA deviceRequests and
acknowledge platform-specific mappings such as AMD and Intel
init.advanced.Devices.
|
fix: validate constraint ceilings after all floor bumps settle Resource constraint checking was single-pass: each parent's min and max Example: gpu0 {id:'cpu', max:6} and gpu1 {id:'cpu', min:8}, both checkResourceConstraints is now two-phase: satisfy all direct floors, Adds unit tests covering the contradictory-constraint scenario in both |
2 participants
Fixes #1419
Compute resource constraints: group-by-
typematching, absolute floor mode, and aggregate summingSummary
Adds three backward-compatible fields to C2D
ResourceConstraintso operators can expressresource rules that the previous model couldn't:
type— target a group of resources (e.g. everytype:"gpu"resource) and aggregatetheir requested amounts, instead of naming a single exact
id.perUnit— choose between the existing ratio semantics (required = parentAmount × value,the default) and a new absolute floor/ceiling (
perUnit:false) that is enforced only whenthe parent resource is requested.
aggregate— sum a single-idconstraint's per-parent contribution across everyrequested parent that carries a matching aggregate constraint, into one shared target.
Together these let a compute environment enforce, in config alone, rules such as:
The change is additive: every existing constraint (exact
id, ratio) behaves exactly asbefore, down to the error-message strings.
Motivation
The cross-resource constraint engine matched resources strictly by exact
idand onlysupported a per-parent-unit ratio, evaluated per parent independently. That left three
things impossible to express:
{id:"gpu0"}).Listing several ids means AND (need all of them), not OR (any one).
max:1wall — each physical GPU is its own discrete resource withmax:1, so aratio constraint (
requiredMin = cpuAmount × 1) throwsCannot satisfythe moment a jobrequests more than 1 CPU.
companion constraint (e.g. cpu per GPU) is evaluated per GPU and does not sum, so "2 GPUs
⇒ 2–8 CPUs" wasn't expressible without collapsing the GPUs into a single counted resource
(which loses per-device pinning).
What changed
Types & schema
src/@types/C2D/C2D.ts—ResourceConstraintnow has optionaltype,perUnit, andaggregate;idis optional (exactly one ofid|type).src/utils/config/schemas.ts—ResourceConstraintSchemavalidates the new fields andadds three refinements: at least one and mutual exclusivity of
id/type, andaggregaterequires a singleid(rejected on atypegroup).perUnitis intentionallyoptional()with no.default()— a default would injectperUnit:trueinto every parsed constraint and break existing deep-equal expectations.Enforcement (
src/components/c2d/compute_engine_base.ts)checkResourceConstraintsbranches onidvstype:perUnit(defaulttrue) → ratio (parentAmount × value), unchanged.perUnit:false→ absolute floor/ceiling (value), enforced only when the parent isrequested.
type) constraints aggregate the requested amounts across all matching resourcesthe environment exposes, and use the sum of member maxes as the "cannot satisfy" ceiling.
bumpGroupToFloordistributes thedeficit across the group's members, preferring the ones with the most availability (lowest
inUse), never exceeding any member's ownmax. Since each physical GPU is a single-DeviceIDresource, exactly one GPU gets attached.
inUseis populated onenv.resourcesbeforeconstraint-checking runs (via
getComputeEnvironments), so this preference is meaningful; theauthoritative gate remains
checkIfResourcesAreAvailable.a new
applyAggregateConstraintspass that sums each aggregate constraint's contribution(
perUnit ? parentAmount × value : value) by targetidacross all requested parents, thenraises the target to the summed floor (capped by the target's own max) and rejects on the
summed ceiling. It runs after the independent-constraint pass and only raises the floor, so it
composes cleanly with group/floor and companion constraints.
getGroupResourceIds,getGroupRequestedAmount,getGroupMax,bumpGroupToFloor,applyAggregateConstraints.id/ ratio path (and itsCannot satisfy …/Too much …error strings) isunchanged.
Docs
docs/compute.md— a single source of truth for C2D configuration: node-wideresources → GPU setup (NVIDIA/AMD/Intel, multi-GPU, shareable devices, migration) → env
refs → resource constraints (companion ratio, group +
perUnit:falsefloor, the counted-vs-per-device
min/maxnuance, andaggregatesumming — each with worked examples) →availability & tracking → pricing → free tier → verify → checklist → a full, schema-validated
example config (two environments, GPUs, constraints, multi-chain pricing, free tiers).
docs/GPU.mdanddocs/compute-pricing.md(merged intodocs/compute.md);all references repointed in
CLAUDE.md,README.md,docs/env.md, anddocs/services.md.docs/services.mdnow notes that services share the same compute environments andresource pool as compute jobs, linking to the new guide.
Constraint semantics at a glance
idtypetype, aggregating their requested amounts. Mutually exclusive withid.min/maxminauto-bumps up;maxis enforced and never auto-reduces.perUnit(defaulttrue)parentAmount × value.false→ absolute floor/ceiling.aggregateidonly. Sums this constraint's per-parent contribution with matching aggregate constraints across all requested parents into one shared target.Backward compatibility
id, ratio, no new fields) parse and behaveidentically.
perUnit/aggregateare never injected into parsed output, so serialized/comparedconstraints are byte-identical to before.
Operator configuration (the features in action)
Group + floor — declare the GPU pool at the connection level; put the group+floor
constraint on the environment's
cpuref (not the pool — a pool-level constraint isinherited by every env, and a GPU-less env would then reject all CPU jobs):
type:"gpu"matches any GPU the env exposes (GPU1/GPU2).perUnit:falsemakes the floorabsolute. If a job selects a CPU with no GPU requested, the engine auto-assigns the freer GPU.
Aggregate — per-device GPUs that jointly scale a companion resource:
{ "id": "GPU1", "constraints": [{ "id": "cpu", "min": 1, "max": 4, "aggregate": true }] }, { "id": "GPU2", "constraints": [{ "id": "cpu", "min": 1, "max": 4, "aggregate": true }] }Renting both →
cpuin [2, 8] (min 1+1, max 4+4); renting one →cpuin [1, 4].Constraint recipes
Common patterns the new fields enable (all documented in
docs/compute.mdand covered by unittests; behavior/error strings verified against the engine):
{ "id": "ram", "min": 8, "max": 16 }on a GPUram:20→Too much ram … Max allowed: 16{ "type": "gpu", "min": 1 }on thecpurefcpu:2auto-assigns 2 GPUs;cpu:3with 2 exposed →Cannot satisfy … requires at least 3 gpu resources, but max is 2{ "type": "gpu", "max": 2, "perUnit": false }on thecpurefToo much gpu resources … Max allowed: 2, requested: 3{ "id": "ram", "min": 8, "aggregate": true }on each GPU[{id:"ram",min:8},{id:"cpu",min:2},{id:"disk",min:20}]on a GPU{ "id": "ram", "min": 32, "perUnit": false }on a GPURecipe 2 is the ratio counterpart of the group + floor example above:
perUnitomitted means"one GPU per CPU" (scales with CPU count), whereas
perUnit:falsemeans "at least one GPUtotal" (independent of CPU count).
Notes / not included
type) andaggregateare separate axes:typegroups the target of oneconstraint;
aggregatesums one-idconstraints across parents.aggregatedeliberatelydoes not combine with a
typetarget (rejected by schema).typeonly (e.g."gpu"). Matching bykind(discrete) would be atrivial follow-up using the same helpers if needed.
Files changed
src/@types/C2D/C2D.ts,src/utils/config/schemas.ts,src/components/c2d/compute_engine_base.ts— feature.src/test/unit/compute.test.ts— tests.docs/compute.md(new),docs/GPU.md+docs/compute-pricing.md(removed),docs/env.md,docs/services.md,README.md,CLAUDE.md— docs & references.Summary by CodeRabbit
New Features
Documentation
compute.md.