Merged
Conversation
Contributor
WalkthroughDocstring formatting adjustments (indentation and line wrapping) were applied in interactive task scheduler modules: blockallocation.py and onetoone.py. No code logic, control flow, or API signatures changed. 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #805 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 32 32
Lines 1468 1468
=======================================
Hits 1435 1435
Misses 33 33 ☔ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
executorlib/task_scheduler/interactive/onetoone.py (1)
179-184: Polish grammar and boolean literal in hostname_localhost docs.Minor clarity nits: add “is”, use “lookup”, proper macOS casing, and Python’s
True.- context of an HPC cluster this essential to be able to communicate to an + context of an HPC cluster this is essential to be able to communicate to an Executor running on a different compute node within the same allocation. And - in principle any computer should be able to resolve that their own hostname - points to the same address as localhost. Still MacOS >= 12 seems to disable - this look up for security reasons. So on MacOS it is required to set this - option to true + in principle any computer should be able to resolve that their own hostname + points to the same address as localhost. Still macOS 12+ seems to disable + this lookup for security reasons. So on macOS it is required to set this + option to Trueexecutorlib/task_scheduler/interactive/blockallocation.py (3)
119-128: Tighten wording and naming in resource_dict bullets.Consistent terminology: “Open MPI”, hyphenate “command-line”, capitalize “Python”.
- - 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 + - 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 (Open MPI + 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.
151-154: Parameter docs: minor consistency/clarity tweaks.Add types, punctuation after True, and avoid underscore in prose.
- wait: If True then shutdown will not return until all running futures have finished executing and the - resources used by the parallel_executors have been reclaimed. - cancel_futures: If True then shutdown will cancel all pending futures. Futures that are completed or running - will not be cancelled. + wait (bool): If True, shutdown will not return until all running futures have finished executing and the + resources used by the parallel executors have been reclaimed. + cancel_futures (bool): If True, cancel all pending futures. Futures that are completed or running + will not be cancelled.
199-208: Mirror the hostname_localhost doc nits here as well.Same minor grammar/casing fixes as in onetoone.py.
- hostname_localhost (boolean): use localhost instead of the hostname to establish the zmq connection. In the - context of an HPC cluster this essential to be able to communicate to an + hostname_localhost (boolean): use localhost instead of the hostname to establish the zmq connection. In the + context of an HPC cluster this is essential to be able to communicate to an Executor running on a different compute node within the same allocation. And - in principle any computer should be able to resolve that their own hostname - points to the same address as localhost. Still MacOS >= 12 seems to disable - this look up for security reasons. So on MacOS it is required to set this - option to true + in principle any computer should be able to resolve that their own hostname + points to the same address as localhost. Still macOS 12+ seems to disable + this lookup for security reasons. So on macOS it is required to set this + option to True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executorlib/task_scheduler/interactive/blockallocation.py(3 hunks)executorlib/task_scheduler/interactive/onetoone.py(1 hunks)
⏰ 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). (13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_old
- GitHub Check: notebooks_integration
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_win
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.
Summary by CodeRabbit
Documentation
Style
Tests
Chores