Test cores#37
Merged
Merged
Conversation
added 2 commits
March 20, 2026 13:50
…ries for different seeds. Othterwise it would rewrite in the same model directory.
…, not when it is launched
Contributor
📝 WalkthroughWalkthroughThe diff adds array-based power accounting and moves job wait-time metric updates to job completion. It extends APIs to pass metrics/baseline flags, updates environment/baseline to use node/cores arrays for cost/power, and adds CLI seed-path/sweep options with session-dir changes and related test coverage for completion metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as Train/Env Loop
participant Scheduler as Job Manager
participant Cluster as Nodes State
participant Metrics as MetricsTracker
participant Reward as RewardCalculator
participant Baseline as BaselineStep
Orchestrator->>Scheduler: assign_jobs_to_available_nodes(job_queue, Cluster.nodes, Cluster.cores)
Scheduler->>Cluster: mark nodes used, add running_jobs {wait_time:int(job_age)}
Orchestrator->>Scheduler: process_ongoing_jobs(Cluster.nodes, Cluster.cores, running_jobs, metrics, is_baseline)
Scheduler->>Cluster: detect finished jobs -> return completed_job_ids
Scheduler->>Metrics: if metrics and jobs finished -> increment completed counters & aggregate wait_time (baseline vs non-baseline)
Orchestrator->>Reward: calculate(..., nodes=Cluster.nodes, cores_available=Cluster.cores)
Reward->>Reward: compute power via _power_consumption_from_state_mwh(nodes, cores_available, include_idle_nodes)
Reward->>Orchestrator: return cost, power, reward
Orchestrator->>Baseline: baseline_step calls process_ongoing_jobs(..., is_baseline=True) and uses Reward outputs for baseline cost/power
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
analyze_seed_occupancy.py (1)
902-902: Minor style: missing space beforehelpkeyword argument.For consistency with other argument definitions in this file, add a space before
help=.🧹 Suggested fix
- parser.add_argument("--seed-path", default="",help="Path if models are saved by seed (forwarded to train.py)") + parser.add_argument("--seed-path", default="", help="Path if models are saved by seed (forwarded to train.py)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@analyze_seed_occupancy.py` at line 902, The parser.add_argument call for "--seed-path" is missing a space before the help keyword; update the line with the function call (parser.add_argument("--seed-path", default="", help="Path if models are saved by seed (forwarded to train.py)")) so it matches the file's argument formatting; locate the parser.add_argument("--seed-path", default="",help="...") occurrence and insert the space before help= to maintain consistent style.src/reward_calculation.py (1)
67-96: Note: Semantic difference between scalar and array power computation paths.The scalar path (lines 85-88) uses a discrete model where nodes are either "idle" (
COST_IDLE_MW) or "used" (COST_USED_MW), while the array path (lines 90-96) uses utilization-based interpolation where partially-utilized nodes consume proportional power.For a node with 50% core utilization:
- Scalar: Counts as "used" → full
COST_USED_MW- Array:
COST_IDLE_MW + 0.5 * (COST_USED_MW - COST_IDLE_MW)This is likely intentional for the transition to finer-grained power modeling, but ensure all callers use the appropriate path consistently. The issues flagged in
environment.pyandbaseline.pyshow mixing of both paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reward_calculation.py` around lines 67 - 96, power_consumption_mwh currently has two different semantics for scalar vs array inputs (scalar branch uses discrete COST_IDLE_MW/COST_USED_MW while array branch uses utilization interpolation via _power_consumption_from_state_mwh), which causes inconsistent results when callers in environment.py and baseline.py mix both styles; fix by choosing one consistent behavior: either (A) update callers (environment.py, baseline.py) to always pass node-state arrays and cores-available arrays so power_consumption_mwh always follows the interpolation path, or (B) change power_consumption_mwh to canonicalize scalar inputs into the array form before computing (e.g., expand num_used_nodes and num_idle_nodes into boolean/node-util arrays and call _power_consumption_from_state_mwh) so both code paths yield the same utilization-based result—locate the code in power_consumption_mwh and modify the scalar-handling branch or update the callers accordingly.
🤖 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/baseline.py`:
- Around line 140-148: The baseline code currently overwrites the array-based
power/cost with a scalar computation: restore consistent array-based accounting
by using power_consumption_mwh(num_used_nodes, num_idle_nodes) and
power_consumption_mwh(num_used_nodes, 0) for baseline_power_mwh and
baseline_power_off_mwh (and derive baseline_cost/baseline_cost_off from those)
instead of the scalar num_used_cores/CORES_PER_NODE calls; remove or revert the
scalar assignments that assign baseline_power_mwh and baseline_power_off_mwh
from num_used_cores and delete the leftover commented-out lines so
baseline_power_mwh, baseline_power_off_mwh, baseline_cost, and baseline_cost_off
are computed only from the array-based power_consumption_mwh results to match
src/environment.py.
In `@src/environment.py`:
- Around line 487-498: The power/cost calculation is inconsistent:
step_power_mwh is computed twice (once as scalar using
num_used_cores/CORES_PER_NODE and again as an array using
power_consumption_mwh(self.state['nodes'], self.cores_available)), but step_cost
is computed from the scalar while metrics are updated from the array. Fix by
computing step_power_mwh exactly once using the array-based call
power_consumption_mwh(self.state['nodes'], self.cores_available) (or convert
that array to the scalar aggregate you intend), then compute step_cost =
step_power_mwh * current_price using that same value, and update
self.metrics.total_cost and self.metrics.episode_total_cost from that same
step_cost; remove the redundant scalar computation and leftover commented-out
lines (references: power_consumption_mwh, CORES_PER_NODE, num_used_cores,
num_idle_nodes, self.state['nodes'], self.cores_available,
self.metrics.total_cost, self.metrics.episode_total_cost).
---
Nitpick comments:
In `@analyze_seed_occupancy.py`:
- Line 902: The parser.add_argument call for "--seed-path" is missing a space
before the help keyword; update the line with the function call
(parser.add_argument("--seed-path", default="", help="Path if models are saved
by seed (forwarded to train.py)")) so it matches the file's argument formatting;
locate the parser.add_argument("--seed-path", default="",help="...") occurrence
and insert the space before help= to maintain consistent style.
In `@src/reward_calculation.py`:
- Around line 67-96: power_consumption_mwh currently has two different semantics
for scalar vs array inputs (scalar branch uses discrete
COST_IDLE_MW/COST_USED_MW while array branch uses utilization interpolation via
_power_consumption_from_state_mwh), which causes inconsistent results when
callers in environment.py and baseline.py mix both styles; fix by choosing one
consistent behavior: either (A) update callers (environment.py, baseline.py) to
always pass node-state arrays and cores-available arrays so
power_consumption_mwh always follows the interpolation path, or (B) change
power_consumption_mwh to canonicalize scalar inputs into the array form before
computing (e.g., expand num_used_nodes and num_idle_nodes into boolean/node-util
arrays and call _power_consumption_from_state_mwh) so both code paths yield the
same utilization-based result—locate the code in power_consumption_mwh and
modify the scalar-handling branch or update the callers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c610fce8-722f-4dc5-ae09-557fb38101e0
📒 Files selected for processing (8)
analyze_seed_occupancy.pysrc/baseline.pysrc/environment.pysrc/job_management.pysrc/reward_calculation.pytest/test_job_completion_metrics.pytrain.pytrain_iter.py
enlorenz
force-pushed
the
test_cores
branch
2 times, most recently
from
ffe59c6 to
0b3dfc7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/baseline.py (1)
140-148:⚠️ Potential issue | 🟠 MajorPower/cost values computed twice; scalar version overwrites array version.
Lines 123-137 compute
baseline_cost,baseline_power_mwh, etc. using the array-based method, then lines 143-147 overwrite these with scalar-based computation usingnum_used_cores/CORES_PER_NODE. This creates:
- Redundant computation - the array-based values are discarded
- Semantic mismatch - scalar path treats fractional nodes (
num_used_cores/CORES_PER_NODE) differently than array path (per-node utilization interpolation)- Inconsistency with environment.py - the agent uses array-based power accounting via
RewardCalculator.calculate(nodes=..., cores_available=...)The commented-out lines 141-142 indicate incomplete refactoring.
Proposed fix: Remove the redundant scalar recomputation
num_used_cores = num_on_nodes * CORES_PER_NODE - np.sum(baseline_cores_available) - - `#baseline_power_mwh` = power_consumption_mwh(num_used_nodes , num_idle_nodes) - `#baseline_power_off_mwh` = power_consumption_mwh(num_used_nodes, 0) - baseline_power_mwh = power_consumption_mwh(num_used_cores/CORES_PER_NODE, num_idle_nodes) - baseline_power_off_mwh = power_consumption_mwh(num_used_cores/CORES_PER_NODE, 0) - - baseline_cost = baseline_power_mwh * current_price - baseline_cost_off = baseline_power_off_mwh * current_price - + # baseline_cost, baseline_cost_off, baseline_power_mwh, baseline_power_off_mwh + # are already computed above using array-based method (lines 123-137)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/baseline.py` around lines 140 - 148, Remove the redundant scalar recomputation that overwrites the array-based baseline values: keep the array-based calls to power_consumption_mwh and baseline_cost/baseline_cost_off (the ones computed earlier, which align with RewardCalculator.calculate and per-node utilization), and delete the subsequent assignments that use num_used_cores/CORES_PER_NODE (the lines assigning baseline_power_mwh and baseline_power_off_mwh using num_used_cores). Ensure only the array-based baseline_power_mwh, baseline_power_off_mwh, baseline_cost, and baseline_cost_off remain so the semantics match the rest of the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/baseline.py`:
- Around line 140-148: Remove the redundant scalar recomputation that overwrites
the array-based baseline values: keep the array-based calls to
power_consumption_mwh and baseline_cost/baseline_cost_off (the ones computed
earlier, which align with RewardCalculator.calculate and per-node utilization),
and delete the subsequent assignments that use num_used_cores/CORES_PER_NODE
(the lines assigning baseline_power_mwh and baseline_power_off_mwh using
num_used_cores). Ensure only the array-based baseline_power_mwh,
baseline_power_off_mwh, baseline_cost, and baseline_cost_off remain so the
semantics match the rest of the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a6788c4-cd30-45fe-98c3-227980e2c83d
📒 Files selected for processing (3)
src/baseline.pysrc/environment.pysrc/reward_calculation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/environment.py
- switch agent and baseline power/cost accounting from binary idle/used nodes to per-node core utilization - keep baseline_off semantics by excluding idle powered-on nodes from power/cost - add regression tests for completion timing and partial-utilization power accounting
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/baseline.py (1)
123-143: Remove redundant cost computation:baseline_costandbaseline_cost_offare computed twice.Lines 123–130 compute costs via
power_cost(), used only for logging. Lines 141–142 then overwrite both variables by multiplyingpower_mwh * current_price. Sincepower_cost()internally callspower_consumption_mwh() * price, the two approaches are mathematically equivalent, making lines 123–130 wasteful duplicate computation.Refactor to compute power first, then derive cost once:
♻️ Proposed refactor
- baseline_cost = power_cost(baseline_state['nodes'], baseline_cores_available, current_price) - env_print(f" > baseline_cost: €{baseline_cost:.4f} | used nodes: {num_used_nodes}, idle nodes: {num_idle_nodes}") - baseline_cost_off = power_cost( - baseline_state['nodes'], - baseline_cores_available, - current_price, - include_idle_nodes=False, - ) - env_print(f" > baseline_cost_off: €{baseline_cost_off:.4f} | used nodes: {num_used_nodes}, idle nodes: 0") baseline_power_mwh = power_consumption_mwh(baseline_state['nodes'], baseline_cores_available) baseline_power_off_mwh = power_consumption_mwh( baseline_state['nodes'], baseline_cores_available, include_idle_nodes=False, ) num_used_cores = num_on_nodes * CORES_PER_NODE - np.sum(baseline_cores_available) baseline_cost = baseline_power_mwh * current_price baseline_cost_off = baseline_power_off_mwh * current_price + env_print(f" > baseline_cost: €{baseline_cost:.4f} | used nodes: {num_used_nodes}, idle nodes: {num_idle_nodes}") + env_print(f" > baseline_cost_off: €{baseline_cost_off:.4f} | used nodes: {num_used_nodes}, idle nodes: 0")Greedy scheduler guideline is satisfied (docstring confirms all nodes stay online, jobs scheduled greedily).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/baseline.py` around lines 123 - 143, The code redundantly calls power_cost() to compute baseline_cost and baseline_cost_off and then overwrites them by multiplying baseline_power_mwh and baseline_power_off_mwh by current_price; remove the initial power_cost() calls and instead compute power once via power_consumption_mwh() (baseline_power_mwh, baseline_power_off_mwh) and derive baseline_cost = baseline_power_mwh * current_price and baseline_cost_off = baseline_power_off_mwh * current_price, updating the env_print() lines to use those derived values (refer to baseline_cost, baseline_cost_off, baseline_power_mwh, baseline_power_off_mwh, power_cost(), power_consumption_mwh(), num_used_nodes, num_idle_nodes).
🤖 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/baseline.py`:
- Around line 123-143: The code redundantly calls power_cost() to compute
baseline_cost and baseline_cost_off and then overwrites them by multiplying
baseline_power_mwh and baseline_power_off_mwh by current_price; remove the
initial power_cost() calls and instead compute power once via
power_consumption_mwh() (baseline_power_mwh, baseline_power_off_mwh) and derive
baseline_cost = baseline_power_mwh * current_price and baseline_cost_off =
baseline_power_off_mwh * current_price, updating the env_print() lines to use
those derived values (refer to baseline_cost, baseline_cost_off,
baseline_power_mwh, baseline_power_off_mwh, power_cost(),
power_consumption_mwh(), num_used_nodes, num_idle_nodes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91314391-7e05-42af-ab59-cddb8c807255
📒 Files selected for processing (3)
src/baseline.pysrc/environment.pysrc/reward_calculation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/environment.py
- src/reward_calculation.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.