Provide the log_obj_size option in every Executor #716
Conversation
WalkthroughA new boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor (Flux/Slurm)
participant InputCheck
User->>Executor: Instantiate with log_obj_size
Executor->>InputCheck: check_log_obj_size(log_obj_size)
InputCheck-->>Executor: Raise ValueError if log_obj_size is True
Executor-->>User: Instance created or error raised
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
==========================================
+ Coverage 96.89% 96.90% +0.01%
==========================================
Files 29 29
Lines 1320 1325 +5
==========================================
+ Hits 1279 1284 +5
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
executorlib/executor/flux.py (1)
105-105: Add log_obj_size validation to FluxJobExecutorThe
FluxClusterExecutorcallscheck_log_obj_size(log_obj_size=…)in its__init__(around line 345), butFluxJobExecutornever performs that check. To keep behavior consistent, invoke the validator as the first statement inFluxJobExecutor.__init__.• File: executorlib/executor/flux.py
Method:FluxJobExecutor.__init__
Location: immediately after the signatureProposed diff:
@@ executorlib/executor/flux.py class FluxJobExecutor(BaseExecutor): def __init__( …, plot_dependency_graph_filename: Optional[str] = None, log_obj_size: bool = False, ): + check_log_obj_size(log_obj_size=log_obj_size) super().__init__(…) …Since
check_log_obj_sizeis already imported at the top of this file, no additional imports are needed. This ensureslog_obj_size=Trueis properly validated at instantiation.executorlib/executor/slurm.py (1)
282-282: Add missing validation to SlurmJobExecutor.The
SlurmJobExecutorclass has thelog_obj_sizeparameter but doesn't callcheck_log_obj_sizefor validation, unlikeSlurmClusterExecutor. This creates inconsistency where some executors validate the parameter while others don't.Add the validation call in
SlurmJobExecutor.__init__:resource_dict.update( {k: v for k, v in default_resource_dict.items() if k not in resource_dict} ) + check_log_obj_size(log_obj_size=log_obj_size) if not disable_dependencies:
🧹 Nitpick comments (2)
executorlib/standalone/inputcheck.py (2)
196-204: Fix type hint inconsistency.The function accepts
Optional[str]but immediately checks forNoneand raises an error. The type hint should bestrinstead ofOptional[str]to accurately reflect thatNonevalues are not accepted.-def check_file_exists(file_name: Optional[str]): +def check_file_exists(file_name: str):
212-213: Fix missing space in error message.There's a missing space between the period and "Please" in the error message.
- "log_obj_size is not supported for the executorlib.SlurmClusterExecutor and executorlib.FluxClusterExecutor." - "Please use log_obj_size=False instead of log_obj_size=True." + "log_obj_size is not supported for the executorlib.SlurmClusterExecutor and executorlib.FluxClusterExecutor. " + "Please use log_obj_size=False instead of log_obj_size=True."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
executorlib/executor/flux.py(5 hunks)executorlib/executor/slurm.py(5 hunks)executorlib/standalone/inputcheck.py(1 hunks)tests/test_standalone_inputcheck.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_standalone_inputcheck.py (1)
executorlib/standalone/inputcheck.py (1)
check_log_obj_size(206-214)
executorlib/executor/flux.py (1)
executorlib/standalone/inputcheck.py (1)
check_log_obj_size(206-214)
executorlib/executor/slurm.py (1)
executorlib/standalone/inputcheck.py (1)
check_log_obj_size(206-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_win
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: notebooks_integration
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: minimal
🔇 Additional comments (4)
tests/test_standalone_inputcheck.py (1)
124-126: LGTM! Test implementation is correct.The test properly verifies that
check_log_obj_sizeraises aValueErrorwhenlog_obj_size=True, following the same pattern as other validation tests in the file.executorlib/executor/flux.py (2)
105-105: LGTM! Parameter addition is correct.The
log_obj_sizeparameter is properly added with appropriate type hint and default value.
345-345: LGTM! Validation is properly implemented.The validation call correctly uses the
check_log_obj_sizefunction to enforce that this feature is currently disabled for theFluxClusterExecutor.executorlib/executor/slurm.py (1)
157-157: LGTM! Validation is properly implemented for SlurmClusterExecutor.The validation call correctly enforces that the
log_obj_sizefeature is disabled forSlurmClusterExecutor.
Summary by CodeRabbit