fix(core): accept gpu: 0 in resources - #198
Conversation
…r writes by default
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughResource validation now allows zero for GPU values and rejects negative GPU values. The numeric checker supports inclusive floors. Tests cover GPU zero and invalid resource values. ChangesResource validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The resource validation preserves positive CPU and memory requirements and permits GPU zero. No issue identified here needs to be resolved before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Zero GPU now means no GPU across the checked validation and runtime paths. The change does not appear to grant GPU access or add a deployment entrypoint, but deployment access and production rollout conditions were not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@userAugustos Basically if GPU is 0 it becomes None? |
|
@Saaketh0 kinda (?) before we had the validation with This doesn't change runtime, because existing code already supported
and then no the only thing was that we (I) added on the manifest PR this check of bigger than 0 gpu, which is wrong basically the whole code already supports "0 gpus" only the validation files changed |
The picker, the manifest schema and the deploy config check now all treat gpu as a non-negative integer; the picker also rejects negative values at input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Repository rule violations found
Repository rule violations found
Every
canyonos deployon main currently fails at config validation:#184 added the pre-deploy resource picker, which fills
gpu: 0for any agent that does not set it and writes the config back. #178 added the manifest schema, which requiresgpu > 0. The two landed independently and contradict each other, so a config that does not already declare a positive gpu is rejected on every deploy. Behind the schema,cli._load_confighas an older check with the same> 0rule (Agent SplitAgent resourcegpumust be a positive number.), so both are changed.The runtimes already read a falsy gpu as "no GPU" (
bool(resources.get("gpu"))in the global controller,if resources.get("gpu")before--gpusin the Local runtime), sogpu: 0is a meaningful value, not a mistake. This makes both checks accept it: the gpu bound becomes inclusive (>= 0), while cpu and memory keep their exclusive> 0.Repro:
canyonos deployon any project whose agents do not setresources.gpu(e.g.examples/portfolio), with the picker left at its defaults.Verification:
uv run --package canyonos-core pytest -q packages/core/tests/test_manifest_schema.py packages/core/tests/test_agent_declaration_schema.py packages/core/tests/test_cli.py— 139 passed.Summary by CodeRabbit