Use per-future observed CPU/GPU metrics in runtime persistence#15
Conversation
| try: | ||
| return float(value) | ||
| except (TypeError, ValueError): | ||
| return float(fallback) |
There was a problem hiding this comment.
@copilot Do not need _safe_float() function, remove it
There was a problem hiding this comment.
Done in 6546ad9. I removed _safe_float() from ventis/controller/utils/sqlalchemy.py and inlined the float parsing/fallback logic in send_data(...) while preserving observed-metric precedence.
📝 WalkthroughWalkthroughChangesRuntime resource persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LocalController
participant Redis
participant send_data
participant SQLAlchemy
LocalController->>LocalController: measure wall and thread CPU time
LocalController->>Redis: store finished_at and cpu_resource
send_data->>Redis: read runtime metrics
send_data->>SQLAlchemy: upsert observed CPU and configured GPU
SQLAlchemy-->>send_data: persist runtime_information
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| POLICY_RULES_KEY = "policy:rules" | ||
|
|
||
|
|
||
| def _read_gpu_memory_for_pid(pid): |
There was a problem hiding this comment.
remove this function and all GPU logic, just keep CPU logic and make GPU pull from the .yaml file still and be set to 0
There was a problem hiding this comment.
Addressed in 1365697: removed the GPU observation function and related GPU runtime logic from local_controller.py; runtime GPU now comes from configured resources.
| "execution_time": execution_time, | ||
| "cpu_resource": cpu_percent, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
@copilot Remove traces of execution_time from this. Only should hset cpu_resource
There was a problem hiding this comment.
Addressed in fffdd36: removed execution_time from the runtime hash metric write; this block now only hsets cpu_resource.
| "execution_time": end - start, | ||
| "cpu_resource": float(res.get("cpu", 0)), | ||
| "gpu_resource": float(res.get("gpu", 0)), | ||
| "execution_time": execution_time, |
There was a problem hiding this comment.
@copilot Remove line 80 and just move end-start to this line, no need to define variable earlier, the initial logic was good
There was a problem hiding this comment.
Updated in 2c29205: removed the earlier execution_time variable and now bind execution_time directly as end - start in the upsert payload.
|
Added CPU Usage Metric to the DB ingestion. |
![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
🤖 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 `@ventis/controller/local_controller.py`:
- Around line 467-474: Update the completion writes in the pull_data completion
path to publish finished_at and cpu_resource atomically via one Redis
hset_multiple call. Ensure ingestion cannot observe a populated future hash with
finished_at present but without the calculated CPU metric, while preserving the
existing wall-duration and CPU-percentage calculations.
In `@ventis/controller/utils/sqlalchemy.py`:
- Around line 80-81: Normalize cpu_resource and gpu_resource immediately after
conversion and before the UPSERT, ensuring both observed CPU and configured GPU
values cannot persist as negative metrics. Use the existing resource-processing
flow to clamp them to zero or reject invalid values consistently, while
preserving valid non-negative values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 916d75b1-461f-46ff-ab9a-a770386902ce
📒 Files selected for processing (3)
tests/test_runtime_sqlalchemy.pyventis/controller/local_controller.pyventis/controller/utils/sqlalchemy.py
| wall_end = time.time() | ||
| self.redis.hset(f"future:{future_id}", "finished_at", wall_end) | ||
|
|
||
| wall_duration = max(wall_end - wall_start, 0.0) | ||
| cpu_seconds = max(time.thread_time() - thread_cpu_start, 0.0) | ||
| cpu_percent = (cpu_seconds / wall_duration * 100.0) if wall_duration else 0.0 | ||
|
|
||
| self.redis.hset(f"future:{future_id}", "cpu_resource", cpu_percent) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Publish completion metrics atomically.
pull_data() scans any populated future:* hash, while send_data() falls back to configured CPU when cpu_resource is absent. Lines 468 and 474 use separate Redis commands, so ingestion can observe finished_at without the observed CPU and persist the static fallback.
Use one hset_multiple completion write, or make ingestion ignore hashes until finished_at is present and retry partial records.
Proposed fix
- self.redis.hset(f"future:{future_id}", "finished_at", wall_end)
-
wall_duration = max(wall_end - wall_start, 0.0)
cpu_seconds = max(time.thread_time() - thread_cpu_start, 0.0)
cpu_percent = (cpu_seconds / wall_duration * 100.0) if wall_duration else 0.0
- self.redis.hset(f"future:{future_id}", "cpu_resource", cpu_percent)
+ self.redis.hset_multiple(
+ f"future:{future_id}",
+ {"finished_at": wall_end, "cpu_resource": cpu_percent},
+ )🤖 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 `@ventis/controller/local_controller.py` around lines 467 - 474, Update the
completion writes in the pull_data completion path to publish finished_at and
cpu_resource atomically via one Redis hset_multiple call. Ensure ingestion
cannot observe a populated future hash with finished_at present but without the
calculated CPU metric, while preserving the existing wall-duration and
CPU-percentage calculations.
| cpu_resource = float(raw.get("cpu_resource") or res.get("cpu", 0)) | ||
| gpu_resource = float(res.get("gpu", 0)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize resource values before persistence.
float() accepts negative observed CPU and configured GPU values, allowing malformed Redis/configuration data to write negative resource metrics. Clamp or reject both values before the UPSERT.
Proposed fix
- cpu_resource = float(raw.get("cpu_resource") or res.get("cpu", 0))
- gpu_resource = float(res.get("gpu", 0))
+ cpu_resource = max(
+ float(raw.get("cpu_resource") or res.get("cpu", 0)), 0.0
+ )
+ gpu_resource = max(float(res.get("gpu", 0)), 0.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpu_resource = float(raw.get("cpu_resource") or res.get("cpu", 0)) | |
| gpu_resource = float(res.get("gpu", 0)) | |
| cpu_resource = max( | |
| float(raw.get("cpu_resource") or res.get("cpu", 0)), 0.0 | |
| ) | |
| gpu_resource = max(float(res.get("gpu", 0)), 0.0) |
🤖 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 `@ventis/controller/utils/sqlalchemy.py` around lines 80 - 81, Normalize
cpu_resource and gpu_resource immediately after conversion and before the
UPSERT, ensuring both observed CPU and configured GPU values cannot persist as
negative metrics. Use the existing resource-processing flow to clamp them to
zero or reject invalid values consistently, while preserving valid non-negative
values.
CPU/GPU values written to
runtime_informationwere coming from static YAML allocations, so persisted metrics did not reflect actual per-future usage. This change records observed metrics at execution time and makes the SQL upsert path prefer observed future data over config fallbacks.Runtime metric capture in LocalController
LocalController._execute_locally(...)now records per-future:execution_time(wall-clock duration)cpu_resource(thread CPU utilization percentage)gpu_resource(GPU memory used by process vianvidia-smi, with safe0.0fallback when unavailable)future:<id>Redis hash for downstream ingestion.Observed-metric precedence in SQL upsert
ventis/controller/utils/sqlalchemy.py::send_data(...)now readsexecution_time,cpu_resource, andgpu_resourcefrom each future row when present.resources_by_agentvalues remain as fallback defaults only.Focused coverage for precedence behavior
tests/test_runtime_sqlalchemy.pyvalidating that observed Redis metrics override static config allocations.Summary by CodeRabbit
New Features
Tests