Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRemoves the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
🧹 Nitpick comments (3)
test/test_jobs_replay_modes.py (2)
76-89: Consider cleaning up temporary files.Each test function creates a temporary file with
delete=Falsebut never removes it. While this works for CI (ephemeral environments), it leaves artifacts on local runs.♻️ Optional: Add cleanup using try/finally or context manager
+import os + def test_raw_replay_job_counts(): with tempfile.NamedTemporaryFile(mode="w", suffix=".log", delete=False) as f: f.write(LOG_CONTENT) path = f.name - sampler = _make_sampler(path, template=False) - - count1, durations1, nodes1, cores1 = _call_generate(sampler, hour=1, exact_replay=True) - assert count1 == 3, f"bin 1 raw: expected 3 jobs, got {count1}" - - count2, durations2, nodes2, cores2 = _call_generate(sampler, hour=2, exact_replay=True) - assert count2 == 2, f"bin 2 raw: expected 2 jobs, got {count2}" - - print(f"PASS: raw replay job counts ({count1}, {count2})") + try: + sampler = _make_sampler(path, template=False) + + count1, _, _, _ = _call_generate(sampler, hour=1, exact_replay=True) + assert count1 == 3, f"bin 1 raw: expected 3 jobs, got {count1}" + + count2, _, _, _ = _call_generate(sampler, hour=2, exact_replay=True) + assert count2 == 2, f"bin 2 raw: expected 2 jobs, got {count2}" + + print(f"PASS: raw replay job counts ({count1}, {count2})") + finally: + os.unlink(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_jobs_replay_modes.py` around lines 76 - 89, The test_raw_replay_job_counts test creates a temp file with tempfile.NamedTemporaryFile(delete=False) but never removes it; modify the test to ensure the temporary file is cleaned up by wrapping the use in a try/finally (or switch to delete=True and use the file inside the with-block) and call os.remove(path) in the finally block if the file still exists; locate the tempfile usage in test_raw_replay_job_counts (references: tempfile.NamedTemporaryFile, LOG_CONTENT, _make_sampler, _call_generate) and add the cleanup so local runs do not leave artifacts.
83-88: Unused variables flagged by static analysis.The unpacked variables
durations1,nodes1,cores1, etc. are not used in this test. Using underscores for unused values improves clarity.♻️ Optional: Prefix unused variables with underscore
- count1, durations1, nodes1, cores1 = _call_generate(sampler, hour=1, exact_replay=True) + count1, _durations1, _nodes1, _cores1 = _call_generate(sampler, hour=1, exact_replay=True) assert count1 == 3, f"bin 1 raw: expected 3 jobs, got {count1}" - count2, durations2, nodes2, cores2 = _call_generate(sampler, hour=2, exact_replay=True) + count2, _durations2, _nodes2, _cores2 = _call_generate(sampler, hour=2, exact_replay=True) assert count2 == 2, f"bin 2 raw: expected 2 jobs, got {count2}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_jobs_replay_modes.py` around lines 83 - 88, Test unpacks from _call_generate into variables (durations1, nodes1, cores1, durations2, nodes2, cores2) that are never used; change those unused names to start with an underscore (e.g., _durations1, _nodes1, _cores1, _durations2, _nodes2, _cores2) or use single underscores to indicate intentionally unused values when calling _call_generate so static analysis warnings are removed while leaving the assertions on count1 and count2 unchanged.src/workload_generator.py (1)
67-74: Simplified raw exact replay logic looks correct.The implementation correctly:
- Converts duration from minutes to hours with ceiling (minimum 1 hour)
- Clamps
nnodesandcores_per_nodeto configured boundsOne minor cleanup from static analysis: the inner
int()cast on line 68 is redundant sincejob['duration_minutes']is already an integer fromparse_jobs().♻️ Optional: Remove redundant int() cast
for job in raw_jobs: - duration_hours = max(1, int(math.ceil(int(job['duration_minutes']) / 60))) + duration_hours = max(1, int(math.ceil(job['duration_minutes'] / 60))) nnodes = min(max(int(job['nnodes']), MIN_NODES_PER_JOB), MAX_NODES_PER_JOB)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workload_generator.py` around lines 67 - 74, The duration conversion uses an unnecessary int() cast around job['duration_minutes']; update the calculation of duration_hours to remove the redundant int() so it reads duration_hours = max(1, int(math.ceil(job['duration_minutes'] / 60))). Locate the loop iterating over raw_jobs (variables raw_jobs, duration_hours, job['duration_minutes']) and adjust that expression; no other logic changes needed and keep parse_jobs() as the producer of integer minutes.
🤖 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/workload_generator.py`:
- Around line 67-74: The duration conversion uses an unnecessary int() cast
around job['duration_minutes']; update the calculation of duration_hours to
remove the redundant int() so it reads duration_hours = max(1,
int(math.ceil(job['duration_minutes'] / 60))). Locate the loop iterating over
raw_jobs (variables raw_jobs, duration_hours, job['duration_minutes']) and
adjust that expression; no other logic changes needed and keep parse_jobs() as
the producer of integer minutes.
In `@test/test_jobs_replay_modes.py`:
- Around line 76-89: The test_raw_replay_job_counts test creates a temp file
with tempfile.NamedTemporaryFile(delete=False) but never removes it; modify the
test to ensure the temporary file is cleaned up by wrapping the use in a
try/finally (or switch to delete=True and use the file inside the with-block)
and call os.remove(path) in the finally block if the file still exists; locate
the tempfile usage in test_raw_replay_job_counts (references:
tempfile.NamedTemporaryFile, LOG_CONTENT, _make_sampler, _call_generate) and add
the cleanup so local runs do not leave artifacts.
- Around line 83-88: Test unpacks from _call_generate into variables
(durations1, nodes1, cores1, durations2, nodes2, cores2) that are never used;
change those unused names to start with an underscore (e.g., _durations1,
_nodes1, _cores1, _durations2, _nodes2, _cores2) or use single underscores to
indicate intentionally unused values when calling _call_generate so static
analysis warnings are removed while leaving the assertions on count1 and count2
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f328ffb9-320b-42dd-bb45-51c3544b0bd7
📒 Files selected for processing (7)
.github/workflows/tests.ymlsrc/environment.pysrc/workload_generator.pytest/run_all.pytest/test_jobs_replay_modes.pytrain.pytrain_iter.py
rbx
force-pushed
the
deduplicate-sampler
branch
from
01f3157 to
74e9937
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/workload_generator.py`:
- Line 68: The duration_hours calculation uses an unnecessary int() around
math.ceil; update the expression in workload_generator.py (the duration_hours
assignment) to remove the redundant outer int() so it becomes max(1,
math.ceil(int(job['duration_minutes']) / 60)); keep the inner
int(job['duration_minutes']) conversion if needed to handle string inputs.
In `@test/test_jobs_replay_modes.py`:
- Around line 77-80: The tests create temporary files with
tempfile.NamedTemporaryFile(mode="w", suffix=".log", delete=False) and bind the
filename to path but never remove them; update each test that follows this
pattern (the instances that set path and write LOG_CONTENT) to unlink the file
after the test (e.g., import os and call os.unlink(path) in a finally block or
use pytest tmp_path/TemporaryDirectory), or revert to using delete=True/using
the file object directly so the file is removed automatically; ensure cleanup is
added for the occurrences that set path (the blocks around the
NamedTemporaryFile usages at the noted locations) so no temp files remain on
disk.
- Line 100: The tuple unpacking from _call_generate currently assigns unused
variables (e.g., count, durations, nodes, cores) causing RUF059; update those
call sites so unused elements are replaced with _ placeholders (for example use
count, _, _, _ = _call_generate(...) or _, durations, _, _ = _call_generate(...)
depending on which values are actually used) — locate every call to
_call_generate where not all four returned values are consumed (including the
occurrences noted around the replay-mode tests) and adjust the left-hand tuple
to use _ for any ignored positions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc9a1b4b-1c2c-4ed0-ae5b-ffe7652f4221
📒 Files selected for processing (7)
.github/workflows/tests.ymlsrc/environment.pysrc/workload_generator.pytest/run_all.pytest/test_jobs_replay_modes.pytrain.pytrain_iter.py
✅ Files skipped from review due to trivial changes (1)
- test/run_all.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/tests.yml
- src/environment.py
- train.py
rbx
force-pushed
the
deduplicate-sampler
branch
from
74e9937 to
a6e297d
Compare
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.