Merged
Conversation
- remove old plot_rewards - remove legacy cumulative savings plotter - rename plot() to plot_episode and move it to plotter.py - delete the now empty plot.py
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRemoves legacy plotting module and flags, introduces per-episode plotting in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_plotter.py (1)
295-306:⚠️ Potential issue | 🔴 Critical
output_dirfixture is undefined and tests will fail.The
test_saves_fileandtest_with_plot_flags_disabledmethods expect anoutput_dirpytest fixture that does not exist in the repository. These tests will fail when run with pytest. Thetest_no_savemethod demonstrates the available pattern: usetempfile.TemporaryDirectory()inline instead of relying on a fixture.Either define the
output_dirfixture intest/conftest.pyor refactor these methods to use inlinetempfile.TemporaryDirectory()for consistency withtest_no_save.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_plotter.py` around lines 295 - 306, Tests reference a non-existent pytest fixture output_dir causing failures; update TestPlotEpisode.test_saves_file (and test_with_plot_flags_disabled if present) to use tempfile.TemporaryDirectory() inline like test_no_save: import tempfile, create a TemporaryDirectory() context, set env.plots_dir to f"{tmpdir}/" (ensuring trailing slash), call plot_episode(env, ... , save=True, show=False, suffix=1) and then inspect files in that temporary directory; alternatively add a simple output_dir fixture to conftest.py that yields a TemporaryDirectory path, but prefer the inline TemporaryDirectory approach for consistency with test_no_save and to keep tests self-contained.
🧹 Nitpick comments (1)
src/plotter.py (1)
172-175: Minor inconsistency: Useos.path.join()for path construction.
plot_dashboard(line 329) andplot_cumulative_savings(line 414) useos.path.join()for path construction, butplot_episodeuses direct string concatenation. This assumesplots_dirhas a trailing slash.Suggested fix for consistency
if save: timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") - plt.savefig(f"{env.plots_dir}{prefix}_{suffix:09d}_{timestamp}.png") - print(f"Figure saved as: {env.plots_dir}{prefix}_{suffix:09d}_{timestamp}.png\nExpecting next save after {env.next_plot_save + env.steps_per_iteration}") + fname = f"{prefix}_{suffix:09d}_{timestamp}.png" + save_path = os.path.join(env.plots_dir, fname) + plt.savefig(save_path) + print(f"Figure saved as: {save_path}\nExpecting next save after {env.next_plot_save + env.steps_per_iteration}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plotter.py` around lines 172 - 175, The save path in the plot_episode save block uses string concatenation and assumes a trailing slash; change the code in the save branch (the plt.savefig and subsequent print) to build the filename with os.path.join(env.plots_dir, f"{prefix}_{suffix:09d}_{timestamp}.png") and reuse that joined path for both plt.savefig and the print message so it matches how plot_dashboard and plot_cumulative_savings construct paths and avoids relying on a trailing slash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/test_plotter.py`:
- Around line 295-306: Tests reference a non-existent pytest fixture output_dir
causing failures; update TestPlotEpisode.test_saves_file (and
test_with_plot_flags_disabled if present) to use tempfile.TemporaryDirectory()
inline like test_no_save: import tempfile, create a TemporaryDirectory()
context, set env.plots_dir to f"{tmpdir}/" (ensuring trailing slash), call
plot_episode(env, ... , save=True, show=False, suffix=1) and then inspect files
in that temporary directory; alternatively add a simple output_dir fixture to
conftest.py that yields a TemporaryDirectory path, but prefer the inline
TemporaryDirectory approach for consistency with test_no_save and to keep tests
self-contained.
---
Nitpick comments:
In `@src/plotter.py`:
- Around line 172-175: The save path in the plot_episode save block uses string
concatenation and assumes a trailing slash; change the code in the save branch
(the plt.savefig and subsequent print) to build the filename with
os.path.join(env.plots_dir, f"{prefix}_{suffix:09d}_{timestamp}.png") and reuse
that joined path for both plt.savefig and the print message so it matches how
plot_dashboard and plot_cumulative_savings construct paths and avoids relying on
a trailing slash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bed9f9c-f521-41f3-8966-ffa823de7845
📒 Files selected for processing (7)
.github/workflows/tests.ymlsrc/environment.pysrc/plot.pysrc/plot_config.pysrc/plotter.pytest/test_plotter.pytrain.py
💤 Files with no reviewable changes (2)
- src/plot_config.py
- src/plot.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/plotter.py (2)
89-116: Potential shape mismatch when metrics lists are shorter thannum_hours.The
hoursarray hasnum_hourselements, butenv.metrics.episode_*lists may have fewer entries if the episode hasn't completed all hours yet or if metrics collection started later. Matplotlib will raise aValueErrorwhen plotting arrays of different lengths.Consider using
_as_series()(already defined in this file) to pad/truncate metrics to the expected length, similar to howplot_dashboardhandles this:♻️ Proposed fix using _as_series for safe array handling
def plot_episode(env: ComputeClusterEnv, num_hours: int, max_nodes: int, save: bool = True, show: bool = True, suffix: int = 0) -> None: hours = np.arange(num_hours) fig, ax1 = plt.subplots(figsize=(12, 6)) color = 'tab:blue' ax1.set_xlabel('Hours') ax1.set_ylabel('Electricity Price (€/MWh)', color=color) if env.plot_config.plot_price: - ax1.plot(hours, env.metrics.episode_price_stats, color=color, label='Electricity Price (€/MWh)') + ax1.plot(hours, _as_series(env.metrics.episode_price_stats, num_hours), color=color, label='Electricity Price (€/MWh)') ax1.tick_params(axis='y', labelcolor=color) ax2 = ax1.twinx() ax2.set_ylabel('Count / Rewards', color='tab:orange') if env.plot_config.plot_online_nodes: - ax2.plot(hours, env.metrics.episode_on_nodes, color='orange', label='Online Nodes') + ax2.plot(hours, _as_series(env.metrics.episode_on_nodes, num_hours), color='orange', label='Online Nodes') if env.plot_config.plot_used_nodes: - ax2.plot(hours, env.metrics.episode_used_nodes, color='green', label='Used Nodes') + ax2.plot(hours, _as_series(env.metrics.episode_used_nodes, num_hours), color='green', label='Used Nodes') if env.plot_config.plot_job_queue: - ax2.plot(hours, env.metrics.episode_job_queue_sizes, color='red', label='Job Queue Size') + ax2.plot(hours, _as_series(env.metrics.episode_job_queue_sizes, num_hours), color='red', label='Job Queue Size') if env.plot_config.plot_eff_reward: - ax2.plot(hours, env.metrics.episode_eff_rewards, color='brown', linestyle='--', label='Efficiency Rewards') + ax2.plot(hours, _as_series(env.metrics.episode_eff_rewards, num_hours), color='brown', linestyle='--', label='Efficiency Rewards') if env.plot_config.plot_price_reward: - ax2.plot(hours, env.metrics.episode_price_rewards, color='blue', linestyle='--', label='Price Rewards') + ax2.plot(hours, _as_series(env.metrics.episode_price_rewards, num_hours), color='blue', linestyle='--', label='Price Rewards') if env.plot_config.plot_idle_penalty: - ax2.plot(hours, env.metrics.episode_idle_penalties, color='green', linestyle='--', label='Idle Penalties') + ax2.plot(hours, _as_series(env.metrics.episode_idle_penalties, num_hours), color='green', linestyle='--', label='Idle Penalties') if env.plot_config.plot_job_age_penalty: - ax2.plot(hours, env.metrics.episode_job_age_penalties, color='yellow', linestyle='--', label='Job Age Penalties') + ax2.plot(hours, _as_series(env.metrics.episode_job_age_penalties, num_hours), color='yellow', linestyle='--', label='Job Age Penalties')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plotter.py` around lines 89 - 116, The plotting code uses hours = np.arange(num_hours) but directly plots env.metrics.episode_* lists which can be shorter, causing ValueError; update each plot call in the block (e.g., plotting episode_price_stats, episode_on_nodes, episode_used_nodes, episode_job_queue_sizes, episode_eff_rewards, episode_price_rewards, episode_idle_penalties, episode_job_age_penalties) to pass their values through the existing _as_series(obj, name, length) helper (the same approach used by plot_dashboard) so each metric is padded/truncated to num_hours before plotting; ensure you call _as_series(env.metrics, 'episode_price_stats', num_hours) etc. and use the returned sequence for the respective ax.plot calls.
172-175: Inconsistent path construction—useos.path.joinfor robustness.
plot_dashboard(line 329) andplot_cumulative_savings(line 414) useos.path.join(), but this function uses string concatenation. This creates an inconsistency and could fail ifplots_dirdoesn't end with a trailing slash.♻️ Proposed fix for consistent path handling
if save: timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") - plt.savefig(f"{env.plots_dir}{prefix}_ep_{suffix:09d}_{timestamp}.png") - print(f"Figure saved as: {env.plots_dir}{prefix}_ep_{suffix:09d}_{timestamp}.png\nExpecting next save after {env.next_plot_save + env.steps_per_iteration}") + fname = f"{prefix}_ep_{suffix:09d}_{timestamp}.png" + save_path = os.path.join(env.plots_dir, fname) + plt.savefig(save_path) + print(f"Figure saved as: {save_path}\nExpecting next save after {env.next_plot_save + env.steps_per_iteration}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plotter.py` around lines 172 - 175, The save path is built via string concatenation which is fragile; change the plt.savefig call to build the filepath with os.path.join(env.plots_dir, f"{prefix}_ep_{suffix:09d}_{timestamp}.png") (and update the printed message to use that same joined filepath) so it matches plot_dashboard/plot_cumulative_savings behavior; ensure you import os if not already and keep the filename formatting using prefix, suffix, and timestamp as before.
🤖 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/plotter.py`:
- Around line 89-116: The plotting code uses hours = np.arange(num_hours) but
directly plots env.metrics.episode_* lists which can be shorter, causing
ValueError; update each plot call in the block (e.g., plotting
episode_price_stats, episode_on_nodes, episode_used_nodes,
episode_job_queue_sizes, episode_eff_rewards, episode_price_rewards,
episode_idle_penalties, episode_job_age_penalties) to pass their values through
the existing _as_series(obj, name, length) helper (the same approach used by
plot_dashboard) so each metric is padded/truncated to num_hours before plotting;
ensure you call _as_series(env.metrics, 'episode_price_stats', num_hours) etc.
and use the returned sequence for the respective ax.plot calls.
- Around line 172-175: The save path is built via string concatenation which is
fragile; change the plt.savefig call to build the filepath with
os.path.join(env.plots_dir, f"{prefix}_ep_{suffix:09d}_{timestamp}.png") (and
update the printed message to use that same joined filepath) so it matches
plot_dashboard/plot_cumulative_savings behavior; ensure you import os if not
already and keep the filename formatting using prefix, suffix, and timestamp as
before.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/environment.py`:
- Around line 279-287: The reset() method currently leaves timeline state from
the previous episode; call _reset_timeline_state() at the start of reset() (or
explicitly clear backlog_queue and reinitialize state keys nodes, job_queue,
pending_job_count, pending_core_hours, pending_avg_duration, pending_max_nodes,
backlog_size) so each episode begins with a fresh 336-hour timeline; update
reset() to invoke _reset_timeline_state() before metrics.reset_episode_metrics()
and the price handling (or perform equivalent explicit clears) to ensure full
episode isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe173bcc-bedb-4068-847a-28b7a0442d9c
📒 Files selected for processing (5)
analyze_lambda_occupancy.pysrc/environment.pytest/test_sanity_env.pytrain.pytrain_iter.py
💤 Files with no reviewable changes (1)
- analyze_lambda_occupancy.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.
also: