Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for throttling batch-array concurrency (“max concurrent workers”) across supported schedulers so large PipeRuns can limit how many array tasks run simultaneously.
Changes:
- Introduces
pipe_settings.max_concurrentand persists it inPipeRunmetadata. - Updates submit-script templates to accept an
array_rangeplus optional scheduler-specificextra_directives. - Implements scheduler-specific throttle rendering in
PipeRunand adds tests covering SLURM/PBS/SGE/OGE/HTCondor behavior and persistence.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| arc/settings/submit.py | Parameterizes array size/range and injects optional extra directives for throttling support. |
| arc/settings/settings.py | Adds new max_concurrent pipe setting to configure scheduler throttling. |
| arc/job/pipe/pipe_run.py | Implements throttle calculation/rendering and persists max_concurrent in run.json. |
| arc/job/pipe/pipe_coordinator.py | Wires max_concurrent from settings into PipeRun creation. |
| arc/job/pipe/pipe_run_test.py | Adds unit tests verifying throttle rendering across schedulers and persistence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'enabled': False, # Set to True to enable pipe mode (it is off by default, use it for large compute campaigns). | ||
| 'min_tasks': 10, # Minimum batch size to trigger pipe mode. | ||
| 'max_workers': 100, # Upper bound on array worker slots per PipeRun. | ||
| 'max_concurrent': None, # Max array workers running simultaneously (like PBS `%N`). None = unthrottled. |
There was a problem hiding this comment.
The new setting comment documents only None as disabling throttling, but the implementation/tests treat max_concurrent=0 as disabled as well. Please update this comment to reflect that 0 (and potentially other non-positive values) disables throttling, or alternatively enforce/validate that max_concurrent must be None or a positive integer.
| 'max_concurrent': None, # Max array workers running simultaneously (like PBS `%N`). None = unthrottled. | |
| 'max_concurrent': None, # Max array workers running simultaneously (like PBS `%N`). None or non-positive values disable throttling. |
| throttle = None | ||
| if self.max_concurrent is not None and self.max_concurrent > 0: | ||
| throttle = min(self.max_concurrent, array_size) | ||
| return cpus, memory_mb, array_size, throttle |
There was a problem hiding this comment.
max_concurrent has implicit semantics for non-positive values (e.g., 0 disables throttling) but this isn’t clearly part of the public API contract of PipeRun. Consider normalizing/validating max_concurrent at construction time (e.g., coerce non-positive to None, or raise on negatives) and documenting the supported values in the class docstring to avoid silent configuration mistakes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #873 +/- ##
==========================================
- Coverage 60.47% 60.37% -0.10%
==========================================
Files 102 102
Lines 31102 31132 +30
Branches 8104 8111 +7
==========================================
- Hits 18808 18797 -11
- Misses 9957 9986 +29
- Partials 2337 2349 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
calvinp0
force-pushed
the
pipe_concurrent
branch
from
01f6714 to
37352e3
Compare
Allow user to define how many workers to run in parallel rather than attempting to apply workers to every job in the array Added validation for settings of max concurrent (So only positive int or None)
This pull request adds support for throttling the number of concurrently running array jobs ("max_concurrent") in pipe runs, allowing users to limit how many jobs are active at once on supported schedulers (Slurm, PBS, SGE, HTCondor, and OGE). It introduces a new
max_concurrentparameter, updates submit script templates and metadata handling, and adds comprehensive tests to ensure correct behavior across schedulers and edge cases.Support for concurrent job throttling:
max_concurrentparameter toPipeRun, allowing users to specify the maximum number of array jobs running simultaneously; this is validated and saved in run metadata, and is now loaded from disk for crash recovery. [1] [2] [3] [4] [5]submit_pipe_runfunction and settings now passmax_concurrentthrough toPipeRun. [1] [2]Scheduler template and logic updates:
Testing improvements:
pipe_run_test.pyto verify throttling behavior, correct template rendering for each scheduler, value validation, persistence across reloads, and edge cases such as clamping and unthrottled runs. [1] [2]Internal code improvements:
max_concurrentvalues, ensuring onlyNoneor positive integers are accepted.These changes make job submission more robust and flexible for large-scale compute campaigns.