Uh oh!
There was an error while loading. Please reload this page.
Update docstrings to improve consistency - #764
Conversation
WalkthroughThis update standardizes and enhances the documentation for the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
executorlib/standalone/interactive/spawner.py (1)
63-77: Renamingoversubscribesilently breaks external callers – provide a compatibility shim
openmpi_oversubscribeis a clearer name, but removing the oldoversubscribeparameter is a breaking change for anyone instantiatingSubprocessSpawnerdirectly or passing the key through factory code.Add a backwards-compatibility alias that raises a
DeprecationWarningfor one release cycle.def __init__( self, cwd: Optional[str] = None, cores: int = 1, - openmpi_oversubscribe: bool = False,+ openmpi_oversubscribe: bool = False, threads_per_core: int = 1, + # Deprecated – keep until vX.Y+ oversubscribe: Optional[bool] = None, ): @@ - super().__init__(- cwd=cwd,- cores=cores,- openmpi_oversubscribe=openmpi_oversubscribe,- )+ if oversubscribe is not None:+ import warnings+ warnings.warn(+ "`oversubscribe` is deprecated; use `openmpi_oversubscribe`",+ DeprecationWarning,+ stacklevel=2,+ )+ openmpi_oversubscribe = oversubscribe++ super().__init__(cwd=cwd, cores=cores, openmpi_oversubscribe=openmpi_oversubscribe)This prevents hard runtime failures while still nudging users toward the new name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/executor/base.py(1 hunks)executorlib/standalone/interactive/spawner.py(1 hunks)executorlib/standalone/serialize.py(1 hunks)executorlib/task_scheduler/base.py(1 hunks)executorlib/task_scheduler/interactive/blockallocation.py(1 hunks)
🔇 Additional comments (3)
executorlib/executor/base.py (1)
71-80: Docstring enrichment aligns with resource_dict spec – looks goodThe expanded description is clear and consistent with the other modules.
executorlib/standalone/serialize.py (1)
45-54: Docstring update is clear and consistentThe new key list mirrors the executor and task-scheduler docs, removing ambiguity about GPU and oversubscribe semantics.
executorlib/task_scheduler/interactive/blockallocation.py (1)
116-125: Docstring harmonisation acknowledgedThe parameter list now matches the rest of the codebase – no further action needed.
| resource_dict (dict): A dictionary of resources required by the task. With the following keys: | ||
| - cores (int): number of MPI cores to be used for each function call | ||
| - threads_per_core (int): number of OpenMP threads to be used for each function call | ||
| - gpus_per_core (int): number of GPUs per worker - defaults to 0 | ||
| - cwd (str/None): current working directory where the parallel python task is executed | ||
| - openmpi_oversubscribe (bool): adds the `--oversubscribe` command line flag (OpenMPI and | ||
| SLURM only) - default False | ||
| - slurm_cmd_args (list): Additional command line arguments for the srun call (SLURM only) | ||
| - error_log_file (str): Name of the error log file to use for storing exceptions raised | ||
| by the Python functions submitted to the Executor. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure check_resource_dict recognises gpus_per_core
The docstring now documents gpus_per_core, but executorlib.standalone.inputcheck.check_resource_dict historically expected gpus_per_worker. Verify the helper was updated accordingly to avoid false validation errors.
🏁 Script executed:
#!/bin/bash# Search for gpus_per_worker vs gpus_per_core in the input-check helper
fd inputcheck | xargs rg -n 'gpus_per_(worker|core)'Length of output: 754
Update inputcheck to support gpus_per_core
The executorlib.standalone.inputcheck module still defines and tests:
check_gpus_per_worker(gpus_per_worker: int)
File: executorlib/standalone/inputcheck.py (lines 30–38)- Calls in tests: tests/test_standalone_inputcheck.py (lines 5, 31–33)
These must be renamed to check_gpus_per_core(gpus_per_core: int) (and all related call sites in check_resource_dict) to match the updated docstring in executorlib/task_scheduler/base.py and avoid validation errors.
Tagging as
🤖 Prompt for AI Agents
In executorlib/standalone/inputcheck.py around lines 30 to 38 and
tests/test_standalone_inputcheck.py around lines 5 and 31 to 33, rename the
function check_gpus_per_worker to check_gpus_per_core and update all related
call sites in the check_resource_dict function accordingly. This change aligns
the validation function and its usage with the updated resource_dict key
gpus_per_core in executorlib/task_scheduler/base.py, preventing validation
errors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #764 +/- ##
=======================================
Coverage 97.47% 97.47% =======================================
Files 32 32 Lines 1423 1423 =======================================
Hits 1387 1387 Misses 36 36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Documentation
gpus_per_workertogpus_per_core,oversubscribetoopenmpi_oversubscribe).slurm_cmd_argsanderror_log_file.Refactor
oversubscribetoopenmpi_oversubscribe) in one component.