Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds optional oracle-based evaluation: two offline solvers (LiquidOracle, ContiguousOracle) are implemented and wired into the environment, metrics tracker, CLI, and training iteration plumbing to record and report oracle lower-bound costs per episode and across timelines. Changes
Sequence Diagram(s)sequenceDiagram
actor Agent as TrainingLoop
participant Env as ComputeClusterEnv
participant Liquid as LiquidOracle
participant Contig as ContiguousOracle
participant Metrics as MetricsTracker
Agent->>Env: reset(enable_oracle=True)
Env->>Liquid: reset()
Env->>Contig: reset()
loop each timestep
Agent->>Env: step(action)
Env->>Liquid: record(price, new_jobs)
Env->>Contig: record(price, new_jobs)
Env->>Agent: observation, reward
end
rect rgba(100,150,200,0.5)
Note over Env,Metrics: episode completion
Env->>Liquid: solve()
Liquid-->>Env: oracle_cost
Env->>Contig: solve()
Contig-->>Env: oracle_contiguous_cost
Env->>Metrics: record_episode_completion(..., oracle costs)
Metrics-->>Agent: episode_data (oracle metrics, gaps)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ 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. Review rate limit: 0/1 reviews remaining, refill in 11 minutes and 30 seconds.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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/metrics_tracker.py (1)
185-205:⚠️ Potential issue | 🟠 Major
savings_vs_oracleis backwards for a metric with that name.Line 185 computes
agent_cost - oracle_cost, but the other savings metrics usebaseline_cost - agent_cost. Publishing that value assavings_vs_oracleon Line 204 will invert the meaning for any downstream reporting or plotting that treats positive values as “better.” Either flip the subtraction or rename it to something likegap_to_oracle/above_oracle_cost. As per coding guidelines, "Metrics tracker module (src/metrics_tracker.py) must track and report performance metrics".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/metrics_tracker.py` around lines 185 - 205, The variable savings_vs_oracle is computed backwards (currently self.episode_total_cost - self.episode_oracle_cost) causing sign inversion versus the other savings metrics; change the calculation in metrics_tracker.py so savings_vs_oracle = self.episode_oracle_cost - self.episode_total_cost (so positive means the agent saved cost versus the oracle), and ensure the value placed into episode_data['savings_vs_oracle'] uses that corrected symbol (savings_vs_oracle) — alternatively, if you prefer semantics, rename the symbol to gap_to_oracle and update its usage consistently (but keep the subtraction direction oracle_cost - episode_total_cost).src/environment.py (1)
365-381:⚠️ Potential issue | 🟠 MajorRecord only the workload that actually survives admission.
Line 365 feeds every generated job into the oracles before
add_new_jobs()decides whether some arrivals are rejected (backlog_droppedon Lines 379-381). In overflow episodes that makes the oracle benchmark price work the agent/baseline never had a chance to run, which inflates oracle cost and distorts the capture-rate comparison. Please move oracle recording until after admission and pass the accepted workload, or explicitly account for queue-full rejections in the oracle input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` around lines 365 - 381, The oracles (self.oracle.record and self.contiguous_oracle.record) are being fed all generated arrivals before add_new_jobs decides which jobs are admitted, causing oracle stats to include jobs that were rejected (backlog_dropped); move the oracle.record calls to after add_new_jobs and pass only the accepted workload (new_jobs) or, alternatively, reduce the recorded counts by backlog_dropped so the oracle sees the actual admitted durations/nodes/cores; update the code around add_new_jobs and the subsequent measurements so oracle recording uses new_jobs (or adjusted arrays) and not the original generated lists.
🤖 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/oracles.md`:
- Around line 83-84: Update the documentation so the ContiguousOracle status is
consistent: change the heading or description that currently labels "Oracle 2 —
Job-Contiguous Greedy (Proposed)" to match the implementation-status table's
"shipped" state (or alternatively update the status table to "proposed" if that
is the intended state); ensure both the section title for "Oracle 2 —
Job-Contiguous Greedy" and the mentions of ContiguousOracle in the later
paragraph(s) (around the ContiguousOracle references) use the same status word
so readers see a single, consistent state.
In `@train.py`:
- Around line 395-414: The new oracle-summary prints use f-strings with no
placeholders and Unicode minus signs, which breaks Ruff; change the two header
prints (the ones that print "\n Liquid Oracle..." and "\n Contiguous
Oracle...") to plain string literals (remove the leading f) and replace all
Unicode minus characters (the '−' in the parenthetical labels like "baseline_off
− oracle_liq", "baseline_off − oracle_jcg", "agent − oracle_jcg", "oracle_jcg −
oracle_liq") with the ASCII hyphen-minus '-' so the messages around
total_oracle_liquid_cost, total_oracle_contiguous_cost and continuity_cost
conform to lint rules.
---
Outside diff comments:
In `@src/environment.py`:
- Around line 365-381: The oracles (self.oracle.record and
self.contiguous_oracle.record) are being fed all generated arrivals before
add_new_jobs decides which jobs are admitted, causing oracle stats to include
jobs that were rejected (backlog_dropped); move the oracle.record calls to after
add_new_jobs and pass only the accepted workload (new_jobs) or, alternatively,
reduce the recorded counts by backlog_dropped so the oracle sees the actual
admitted durations/nodes/cores; update the code around add_new_jobs and the
subsequent measurements so oracle recording uses new_jobs (or adjusted arrays)
and not the original generated lists.
In `@src/metrics_tracker.py`:
- Around line 185-205: The variable savings_vs_oracle is computed backwards
(currently self.episode_total_cost - self.episode_oracle_cost) causing sign
inversion versus the other savings metrics; change the calculation in
metrics_tracker.py so savings_vs_oracle = self.episode_oracle_cost -
self.episode_total_cost (so positive means the agent saved cost versus the
oracle), and ensure the value placed into episode_data['savings_vs_oracle'] uses
that corrected symbol (savings_vs_oracle) — alternatively, if you prefer
semantics, rename the symbol to gap_to_oracle and update its usage consistently
(but keep the subtraction direction oracle_cost - episode_total_cost).
🪄 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: 4126e53e-193a-43dc-8f8a-328f0cf649e2
📒 Files selected for processing (6)
src/environment.pysrc/metrics_tracker.pysrc/oracle.pysrc/oracles.mdtrain.pytrain_iter.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/metrics_tracker.py (1)
185-185: ⚡ Quick winDisambiguate
savings_vs_oraclenow that two oracle baselines exist.Line 185 computes savings vs the liquid oracle only, but the payload also includes contiguous-oracle cost. Consider emitting explicit fields for both to avoid accidental misuse.
Suggested patch
- savings_vs_oracle: float = self.episode_total_cost - self.episode_oracle_cost + savings_vs_oracle_liquid: float = self.episode_total_cost - self.episode_oracle_cost + savings_vs_oracle_contiguous: float = ( + self.episode_total_cost - self.episode_oracle_contiguous_cost + ) @@ - 'savings_vs_oracle': savings_vs_oracle, + 'savings_vs_oracle_liquid': savings_vs_oracle_liquid, + 'savings_vs_oracle_contiguous': savings_vs_oracle_contiguous,Also applies to: 204-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/metrics_tracker.py` at line 185, The current single field savings_vs_oracle is ambiguous because there are two oracle baselines; change the metrics emission to compute and emit two explicit fields — e.g., savings_vs_liquid_oracle = episode_total_cost - episode_oracle_cost and savings_vs_contiguous_oracle = episode_total_cost - episode_contiguous_oracle_cost — instead of overwriting/using savings_vs_oracle; update all places that currently set or read savings_vs_oracle (search for the variable name and the emission logic in MetricsTracker/emit_metrics) to use the two new fields so both baselines are unambiguous.
🤖 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/oracle.py`:
- Around line 49-60: The record method currently zips new_jobs_durations,
new_jobs_nodes, and new_jobs_cores which silently truncates if their lengths
differ; add an explicit guard at the start of record (and likewise in
ContiguousOracle.record) that checks len(new_jobs_durations) ==
len(new_jobs_nodes) == len(new_jobs_cores) and raise a ValueError (including the
three lengths in the message) if not equal so mismatched input is detected
rather than silently undercounted.
In `@src/oracles.md`:
- Around line 142-148: Add explicit language identifiers to the two fenced code
blocks that currently lack them: change the block containing the cost names
("baseline_cost ... oracle_jcg_cost") to use ```text and likewise change the
block containing the formula line "Agent Capture Rate = (baseline_off − agent) /
(baseline_off − oracle_jcg)" to use ```text so markdownlint MD040 is satisfied;
locate these blocks by searching for the cost list and the "Agent Capture Rate"
formula in src/oracles.md and prepend "text" to their opening ``` fences.
- Around line 143-148: The documented metric keys in src/oracles.md are out of
sync with the actual emitted metric names from metrics_tracker (they use
baseline_cost_off, oracle_cost, oracle_contiguous_cost); update the listed keys
so they exactly match the emitted names: change baseline_off_cost to
baseline_cost_off, oracle_liq_cost to oracle_cost, and oracle_jcg_cost to
oracle_contiguous_cost (leave baseline_cost and agent_cost as-is if they already
match).
In `@train_iter.py`:
- Line 450: The help text for the CLI flag defined by
parser.add_argument("--oracle", ...) is misleading: update the help string in
train_iter.py to state that this flag enables both the liquid and contiguous
oracles (not only liquid). Locate the parser.add_argument call for "--oracle"
and replace the help value with a concise description such as "Enable both
liquid and contiguous oracles in the environment" (or similar wording) so it
accurately reflects the actual behavior.
---
Nitpick comments:
In `@src/metrics_tracker.py`:
- Line 185: The current single field savings_vs_oracle is ambiguous because
there are two oracle baselines; change the metrics emission to compute and emit
two explicit fields — e.g., savings_vs_liquid_oracle = episode_total_cost -
episode_oracle_cost and savings_vs_contiguous_oracle = episode_total_cost -
episode_contiguous_oracle_cost — instead of overwriting/using savings_vs_oracle;
update all places that currently set or read savings_vs_oracle (search for the
variable name and the emission logic in MetricsTracker/emit_metrics) to use the
two new fields so both baselines are unambiguous.
🪄 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: b655e37d-3f14-4007-b9a7-ee02510a7f31
📒 Files selected for processing (6)
src/environment.pysrc/metrics_tracker.pysrc/oracle.pysrc/oracles.mdtrain.pytrain_iter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- train.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
1 participant
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.