Skip to content

deduplicate power metrics#46

Merged
rbx merged 1 commit intomasterfrom
deduplicate-metrics
Apr 1, 2026
Merged

deduplicate power metrics#46
rbx merged 1 commit intomasterfrom
deduplicate-metrics

Conversation

@rbx
Copy link
Copy Markdown
Member

@rbx rbx commented Apr 1, 2026

Environment already calculates proportional (core-based, not node-pased) power consumption metrics. No need to duplicate the work in the metrics code.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

📝 Walkthrough

Walkthrough

Removed proportional per-core power/cost computations and related metric fields across callbacks, metrics tracking, and reporting; replaced them with direct episode-level power metrics emitted from the environment and updated logging/printing to use non‑proportional totals and savings.

Changes

Cohort / File(s) Summary
Callback & Logging
src/callbacks.py
Dropped per-core proportional power/cost computation and imports of COST_IDLE_MW, COST_USED_MW, CORES_PER_NODE; log direct episode power metrics from env.metrics (metrics/power_mwh, metrics/bl_power_mwh, metrics/bl_off_power_mwh, metrics/savings_power_vs_baseline_off).
Metrics Computation
src/metrics_tracker.py
Removed proportional-per-core power and cost calculations and all *_prop_* power/cost and proportional savings fields from record_episode_completion and returned episode_data; removed related config imports.
Reporting & Output
src/evaluation_summary.py, train.py
Removed proportional-power fields from formatted summaries and console prints; replaced proportional power/cost reporting and savings computations with totals (*_power_mwh, *_cost) and percent-of-baseline computed from total savings vs baseline cost.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Pr2 drop #44: Touches the same metrics pipeline and the same metric keys (*_prop_* vs totals) across callbacks, metrics_tracker, evaluation_summary, and train.
  • Pr1 reward #40: Previously introduced proportional per-core power/cost computations and metric fields that this PR removes.
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'deduplicate power metrics' directly and concisely summarizes the main change: removing duplicate power metric calculations.
Description check ✅ Passed The description clearly explains the rationale for the changes: the environment already calculates proportional power metrics, so duplication in the metrics code is unnecessary.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/evaluation_summary.py`:
- Around line 67-73: The episode summary currently emits Power and Savings twice
by repeating the same f-string block using episode_data (the lines building
f"Power={float(episode_data['agent_power_consumption_mwh'])... " and
f"Savings=€{float(episode_data['savings_vs_baseline'])..."); remove the
duplicate occurrence so the summary only formats and prints Power and Savings
once, keeping the original formatting and references to episode_data intact
(locate the duplicated f-string block in the episode summary function and delete
the redundant block).

In `@train.py`:
- Line 384: The print call at the static header in train.py uses an unnecessary
f-string (print(f"...")), which triggers Ruff F541; update the statement in the
same scope where that header is printed (the print of "=== COST SAVINGS (TOTAL
OVER EVALUATION) ===") by removing the leading f so it becomes a normal string
literal without any interpolation.
🪄 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

Run ID: 3f2d6ad0-8de5-4e99-82fe-704bacd4ea17

📥 Commits

Reviewing files that changed from the base of the PR and between 30bbf23 and aeb59c3.

📒 Files selected for processing (4)
  • src/callbacks.py
  • src/evaluation_summary.py
  • src/metrics_tracker.py
  • train.py
💤 Files with no reviewable changes (1)
  • src/metrics_tracker.py

@rbx rbx force-pushed the deduplicate-metrics branch from aeb59c3 to 071ad21 Compare April 1, 2026 09:57
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/callbacks.py (1)

1-1: Consider removing unused MAX_QUEUE_SIZE import.

MAX_QUEUE_SIZE is imported but not used—the only reference at line 41 is commented out.

Suggested fix
-from src.config import EPISODE_HOURS, MAX_QUEUE_SIZE, MAX_NODES
+from src.config import EPISODE_HOURS, MAX_NODES
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/callbacks.py` at line 1, The import list in src/callbacks.py includes
MAX_QUEUE_SIZE which is unused (the only reference is commented out); remove
MAX_QUEUE_SIZE from the from src.config import ... statement so the module only
imports the used symbols (EPISODE_HOURS, MAX_NODES) to eliminate the unused
import warning and keep imports minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/callbacks.py`:
- Line 1: The import list in src/callbacks.py includes MAX_QUEUE_SIZE which is
unused (the only reference is commented out); remove MAX_QUEUE_SIZE from the
from src.config import ... statement so the module only imports the used symbols
(EPISODE_HOURS, MAX_NODES) to eliminate the unused import warning and keep
imports minimal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a43a8f26-8a16-43c6-8ef8-494a4ab5c76f

📥 Commits

Reviewing files that changed from the base of the PR and between aeb59c3 and 071ad21.

📒 Files selected for processing (4)
  • src/callbacks.py
  • src/evaluation_summary.py
  • src/metrics_tracker.py
  • train.py
💤 Files with no reviewable changes (1)
  • src/metrics_tracker.py
✅ Files skipped from review due to trivial changes (1)
  • train.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/evaluation_summary.py

@rbx rbx merged commit 3a5fd54 into master Apr 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant