Parallel: Added evalulation logic#29
Merged
rbx merged 2 commits intoFairRootGroup:masterfrom Mar 3, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds workload generator CLI support and savings-evaluation flags. Introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 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: 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 `@train_iter.py`:
- Around line 133-134: The code in build_command appends ["--evaluate-savings",
"--eval-months", str(eval_months)] when evaluate_savings is True, which will
pass the string "None" if eval_months is None; modify build_command (or the
default for eval_months) so that when evaluate_savings is True and eval_months
is None it uses a safe default (e.g., 6) or skips adding --eval-months until a
valid int is provided; specifically update the logic around
evaluate_savings/eval_months in build_command to either set eval_months = 6 when
None or only append "--eval-months" if eval_months is not None so train.py never
receives "None".
Comment on lines
+133
to
+134
| if evaluate_savings: | ||
| command += ["--evaluate-savings", "--eval-months", str(eval_months)] |
Contributor
There was a problem hiding this comment.
Potential bug: eval_months=None will pass "None" string to train.py.
When evaluate_savings=True but eval_months is not explicitly provided (remains None from the default on line 110), str(None) produces the literal string "None", which will likely cause train.py to fail when parsing the integer argument.
While the CLI path via main() always provides a default of 6, direct calls to build_command with evaluate_savings=True will hit this issue.
Proposed fix: add a guard or default
if evaluate_savings:
+ if eval_months is None:
+ eval_months = 6
command += ["--evaluate-savings", "--eval-months", str(eval_months)]Alternatively, change the default on line 110:
- eval_months=None,
+ eval_months=6,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@train_iter.py` around lines 133 - 134, The code in build_command appends
["--evaluate-savings", "--eval-months", str(eval_months)] when evaluate_savings
is True, which will pass the string "None" if eval_months is None; modify
build_command (or the default for eval_months) so that when evaluate_savings is
True and eval_months is None it uses a safe default (e.g., 6) or skips adding
--eval-months until a valid int is provided; specifically update the logic
around evaluate_savings/eval_months in build_command to either set eval_months =
6 when None or only append "--eval-months" if eval_months is not None so
train.py never receives "None".
enlorenz
force-pushed
the
wip/integration
branch
from
5a6185a to
58a4354
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
train_iter.py (1)
109-110: Default value inconsistency betweenbuild_commandand CLI.The
eval_monthsdefault here is0, but the CLI default on line 220 is6. Ifbuild_commandis called directly withevaluate_savings=Truewithout specifyingeval_months, it will evaluate for 0 months, which is likely not meaningful.Consider aligning defaults:
Proposed fix: align default to 6
seed=None, evaluate_savings=False, - eval_months=0, + eval_months=6, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@train_iter.py` around lines 109 - 110, The build_command invocation sets eval_months=0 while the CLI default is 6, causing inconsistent behavior when evaluate_savings=True; update the default in build_command (the eval_months parameter used where evaluate_savings is passed) to 6 to match the CLI, or modify build_command to default eval_months to None and fallback to the CLI's default (6) when evaluate_savings is True; ensure you adjust the default for the eval_months argument referenced in build_command and any callers that rely on evaluate_savings to maintain consistent evaluation length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@train_iter.py`:
- Around line 109-110: The build_command invocation sets eval_months=0 while the
CLI default is 6, causing inconsistent behavior when evaluate_savings=True;
update the default in build_command (the eval_months parameter used where
evaluate_savings is passed) to 6 to match the CLI, or modify build_command to
default eval_months to None and fallback to the CLI's default (6) when
evaluate_savings is True; ensure you adjust the default for the eval_months
argument referenced in build_command and any callers that rely on
evaluate_savings to maintain consistent evaluation length.
rbx
approved these changes
Mar 3, 2026
rbx
reviewed
Mar 3, 2026
enlorenz
force-pushed
the
wip/integration
branch
from
0ef4edc to
6372ad4
Compare
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 `@train_iter.py`:
- Around line 110-111: The eval_months parameter currently defaults to 0 and can
be forwarded into train.py unchanged; update validation in build_command (and
any callers that forward eval_months) to check that eval_months is a positive
integer and, if evaluate_savings is True and eval_months is missing/<=0,
substitute a safe default (e.g., 1 or a named constant DEFAULT_EVAL_MONTHS)
before constructing the command; ensure the validated/normalized value is the
one passed to train invocation and update references in functions/build_command
where eval_months is forwarded so non-CLI callers never send invalid month
values.
Comment on lines
+110
to
+111
| evaluate_savings=False, | ||
| eval_months=0, |
Contributor
There was a problem hiding this comment.
Validate eval_months and use a safe default in build_command.
Line 110 defaults eval_months to 0, and Line 136 forwards it as-is when evaluation is enabled. This allows invalid month values (e.g., 0/negative) to leak into train.py for non-CLI callers.
🔧 Proposed fix
def build_command(
@@
- eval_months=0,
+ eval_months=6,
@@
- if evaluate_savings:
+ if evaluate_savings:
+ if eval_months < 1:
+ raise ValueError("eval_months must be >= 1 when evaluate_savings=True")
command += ["--evaluate-savings", "--eval-months", str(eval_months)]
@@
def main():
@@
if args.parallel < 1:
parser.error("--parallel must be at least 1")
+ if args.evaluate_savings and args.eval_months < 1:
+ parser.error("--eval-months must be at least 1 when --evaluate-savings is set")Also applies to: 135-136, 225-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@train_iter.py` around lines 110 - 111, The eval_months parameter currently
defaults to 0 and can be forwarded into train.py unchanged; update validation in
build_command (and any callers that forward eval_months) to check that
eval_months is a positive integer and, if evaluate_savings is True and
eval_months is missing/<=0, substitute a safe default (e.g., 1 or a named
constant DEFAULT_EVAL_MONTHS) before constructing the command; ensure the
validated/normalized value is the one passed to train invocation and update
references in functions/build_command where eval_months is forwarded so non-CLI
callers never send invalid month values.
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.