Uh oh!
There was an error while loading. Please reload this page.
[feature] Add option to specify number of nodes - #565
Conversation
Caution Review failedThe pull request is closed. WalkthroughThis update introduces two optional parameters, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as FluxPythonSpawner
participant J as JobspecV1
U->>F: Call bootup(command_lst)
F->>J: Create job spec using _num_nodes & _exclusive via from_command
F->>J: Create job spec using _num_nodes & _exclusive via from_nest_command
J-->>F: Return job specs
sequenceDiagram
participant U as User
participant S as SrunSpawner
participant G as generate_slurm_command
U->>S: Call generate_command()
S->>G: Invoke with _num_nodes & _exclusive
G-->>S: Return command list with flags (-N, --exact)
S-->>U: Return complete SLURM command
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Nitpick comments (3)
executorlib/interactive/slurm.py (2)
35-45: Update docstring to include new parameters.The docstring is missing documentation for the new parameters
num_nodesandexclusive.Add the following lines to the docstring:
gpus_per_core (int, optional): The number of GPUs per core. Defaults to 0. + num_nodes (int, optional): The number of nodes to allocate. Defaults to None.+ exclusive (bool, optional): Whether to request exclusive node access. Defaults to False. openmpi_oversubscribe (bool, optional): Whether to oversubscribe the cores. Defaults to False.
93-105: Update docstring to include new parameters.The docstring is missing documentation for the new parameters
num_nodesandexclusive.Add the following lines to the docstring:
gpus_per_core (int, optional): The number of GPUs per core. Defaults to 0. + num_nodes (int, optional): The number of nodes to allocate. Defaults to None.+ exclusive (bool, optional): Whether to request exclusive node access. Defaults to False. openmpi_oversubscribe (bool, optional): Whether to oversubscribe the cores. Defaults to False.executorlib/interactive/flux.py (1)
24-37: Update docstring to include new parameters.The docstring is missing documentation for the new parameters
num_nodesandexclusive.Add the following lines to the docstring:
gpus_per_core (int, optional): The number of GPUs per base. Defaults to 0. + num_nodes (int, optional): The number of nodes to allocate. Defaults to None.+ exclusive (bool, optional): Whether to request exclusive node access. Defaults to False. openmpi_oversubscribe (bool, optional): Whether to oversubscribe. Defaults to False.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executorlib/interactive/flux.py(3 hunks)executorlib/interactive/slurm.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.10)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_win
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_flux_mpich
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (1)
executorlib/interactive/flux.py (1)
87-103: LGTM!The implementation correctly passes the node count and exclusivity parameters to both job specification methods, maintaining consistency with the SLURM implementation.
| if num_nodes is not None: | ||
| command_prepend_lst += ["-N", str(num_nodes)] | ||
| if threads_per_core > 1: | ||
| command_prepend_lst += ["--cpus-per-task" + str(threads_per_core)] |
There was a problem hiding this comment.
Fix missing = in --cpus-per-task flag.
The --cpus-per-task flag is missing the = separator, which will cause the command to fail.
- command_prepend_lst += ["--cpus-per-task" + str(threads_per_core)]+ command_prepend_lst += ["--cpus-per-task=" + str(threads_per_core)]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command_prepend_lst+= ["--cpus-per-task"+str(threads_per_core)] | |
| command_prepend_lst+= ["--cpus-per-task="+str(threads_per_core)] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #565 +/- ##
==========================================
+ Coverage 95.89% 95.91% +0.02%
==========================================
Files 26 26 Lines 1168 1176 +8 ==========================================
+ Hits 1120 1128 +8
Misses 48 48 ☔ View full report in Codecov by Sentry. |
jan-janssen
commented
Feb 11, 2025
Documentation is still missing. |
Summary by CodeRabbit
New Features
Documentation