[Documentation] Add FAQ section explaining cores, threads_per_core and max_workers#998
[Documentation] Add FAQ section explaining cores, threads_per_core and max_workers#998jan-janssen wants to merge 2 commits into
Conversation
Users frequently confuse the cores, threads_per_core and max_workers/max_cores parameters since they all control compute resources but on different levels. Add a dedicated FAQ section to the trouble shooting docs clarifying that max_workers/max_cores is the executor-wide core budget, while cores (per call, primarily for mpi4py) and threads_per_core (per call, OpenMP) describe how each function call spends a slice of that budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR updates docs/trouble_shooting.md: it rewords Flux scheduler Python-version guidance (Python 3.13 recommended, limited to ≤3.13) and adds a troubleshooting section explaining ChangesExecutor Resource Configuration Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/trouble_shooting.md`:
- Around line 80-81: Update the sentence to reference threads_per_core so it
matches the formula: change the wording that currently says "the sum of their
requested cores never exceeds `max_cores`" to state that executorlib schedules
tasks so that the sum of each task's requested `cores * threads_per_core` (i.e.,
requested threads) never exceeds `max_cores`, explicitly mentioning the symbols
`cores`, `threads_per_core`, and `max_cores` to avoid ambiguity.
- Around line 67-68: The doc line claiming "when neither is provided executorlib
uses the number of cores available on the machine" is too absolute; update the
wording to scope the fallback to CPU count to executors/modes that enable local
core fallback (see src/executorlib/standalone/inputcheck.py and the
set_local_cores=True path). Mention that when set_local_cores is False the
library raises a ValueError instead, and clarify which executors or modes use
the automatic CPU-count fallback versus those that require explicit
max_cores/max_workers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| limit refers to the number of compute cores. Setting either is optional - when neither is provided executorlib uses the | ||
| number of cores available on the machine. |
There was a problem hiding this comment.
Clarify defaulting behavior when neither max_cores nor max_workers is set.
This sentence is too absolute. The code path in src/executorlib/standalone/inputcheck.py shows fallback to CPU count is conditional (set_local_cores=True); otherwise it raises ValueError. Please scope this statement to the relevant executors/modes.
Suggested wording
- limit refers to the number of compute cores. Setting either is optional - when neither is provided executorlib uses the
- number of cores available on the machine.
+ limit refers to the number of compute cores. Setting either is optional in executors that support local-core fallback;
+ in that case, when neither is provided, executorlib uses the number of cores available on the machine.📝 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.
| limit refers to the number of compute cores. Setting either is optional - when neither is provided executorlib uses the | |
| number of cores available on the machine. | |
| limit refers to the number of compute cores. Setting either is optional in executors that support local-core fallback; | |
| in that case, when neither is provided, executorlib uses the number of cores available on the machine. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/trouble_shooting.md` around lines 67 - 68, The doc line claiming "when
neither is provided executorlib uses the number of cores available on the
machine" is too absolute; update the wording to scope the fallback to CPU count
to executors/modes that enable local core fallback (see
src/executorlib/standalone/inputcheck.py and the set_local_cores=True path).
Mention that when set_local_cores is False the library raises a ValueError
instead, and clarify which executors or modes use the automatic CPU-count
fallback versus those that require explicit max_cores/max_workers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 39 39
Lines 2103 2103
=======================================
Hits 1981 1981
Misses 122 122 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/trouble_shooting.md (1)
67-68:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAddress the prior review comment about conditional CPU fallback.
A previous review correctly noted that the statement "when neither is provided executorlib uses the number of cores available on the machine" is too absolute. The code path shows this fallback is conditional on
set_local_cores=True; when false, aValueErroris raised instead. Please scope this statement to clarify which executors or modes support automatic CPU-count fallback versus requiring explicit configuration.Run the following script to confirm which executor implementations enable the automatic CPU fallback:
#!/bin/bash # Description: Identify executors that set set_local_cores=True vs False # Search for set_local_cores usage patterns rg -nP 'set_local_cores\s*=' --type=py -A2 -B2 # Find executor initialization that may control this behavior ast-grep --pattern $'class $EXECUTOR($$$) { $$$ set_local_cores $$$ }'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/trouble_shooting.md` around lines 67 - 68, Update the docs text to stop asserting an unconditional fallback: clarify that executorlib only uses the machine CPU count when set_local_cores=True; otherwise the executor raises a ValueError and requires explicit CPU configuration. Mention the configuration flag set_local_cores (and the behavior of ValueError) and instruct readers to check specific executor classes that set set_local_cores=True to know which implementations support automatic fallback (e.g., search for set_local_cores usage in executor classes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/trouble_shooting.md`:
- Line 56: Fix the spelling mistake in the sentence "performance computing
installations Python 3.13 is the recommended Python verion." by changing
"verion" to "version" so it reads "...recommended Python version."; update that
line in the troubleshooting document where that exact sentence appears.
---
Duplicate comments:
In `@docs/trouble_shooting.md`:
- Around line 67-68: Update the docs text to stop asserting an unconditional
fallback: clarify that executorlib only uses the machine CPU count when
set_local_cores=True; otherwise the executor raises a ValueError and requires
explicit CPU configuration. Mention the configuration flag set_local_cores (and
the behavior of ValueError) and instruct readers to check specific executor
classes that set set_local_cores=True to know which implementations support
automatic fallback (e.g., search for set_local_cores usage in executor classes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| the [flux](http://flux-framework.org) job scheduler are currently limited to Python 3.12 and below. Consequently for high | ||
| performance computing installations Python 3.12 is the recommended Python verion. | ||
| the [flux](http://flux-framework.org) job scheduler are currently limited to Python 3.13 and below. Consequently for high | ||
| performance computing installations Python 3.13 is the recommended Python verion. |
There was a problem hiding this comment.
Fix spelling error.
"verion" should be "version".
📝 Proposed fix
-performance computing installations Python 3.13 is the recommended Python verion.
+performance computing installations Python 3.13 is the recommended Python version.📝 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.
| performance computing installations Python 3.13 is the recommended Python verion. | |
| performance computing installations Python 3.13 is the recommended Python version. |
🧰 Tools
🪛 LanguageTool
[grammar] ~56-~56: Ensure spelling is correct
Context: ...s Python 3.13 is the recommended Python verion. ## Cores, Threads per Core and Maximum Work...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/trouble_shooting.md` at line 56, Fix the spelling mistake in the
sentence "performance computing installations Python 3.13 is the recommended
Python verion." by changing "verion" to "version" so it reads "...recommended
Python version."; update that line in the troubleshooting document where that
exact sentence appears.
jan-janssen
changed the title
1 participant
What changed
Added a new "Cores, Threads per Core and Maximum Workers" section to
docs/trouble_shooting.md(the page that serves as the project FAQ), just above the existing "Resource Dictionary" section.Why
Users frequently struggle to distinguish the
cores,threads_per_coreandmax_workers/max_coresparameters, since they all control compute resources but operate on different levels. The new section separates them by scope:max_workers/max_cores— executor-level: the total core budget shared across all submitted tasks.max_coresis recommended;max_workersexists only for stdlibExecutorbackwards compatibility.cores— per-function-call (resource_dict): number of MPI ranks for a single function. Explicitly noted that this should primarily be used with mpi4py, and that giving a plain serial functioncores > 1just reserves idle cores without speeding it up.threads_per_core— per-function-call (resource_dict): OpenMP threads per core (OMP_NUM_THREADSetc.), for thread-based parallelism in linked libraries rather than MPI.It closes with the relationship (
cores * threads_per_coreper task, summed across tasks must stay withinmax_cores) and a runnableSingleNodeExecutor(max_cores=4)+resource_dict={"cores": 2}mpi4py example.Notes for reviewers
trouble_shooting.mdbecause there is no dedicated FAQ page in_toc.yml. Happy to split it into a standalonefaq.mdpage if preferred.🤖 Generated with Claude Code
Summary by CodeRabbit