fix(utils): key ParallelExecutor straggler dedup on item index, not future - #84
Conversation
…uture The straggler resubmission dedup set was keyed on Future objects, so the resubmitted future nf was never recorded and could itself be resubmitted on a later pass (f -> nf -> nf2 -> ...), violating the 'at most once per item' invariant. Track item indices instead so an item is permanently marked regardless of which future represents it.
Greptile SummaryThis PR corrects straggler deduplication in
Confidence Score: 4/5The production fix appears safe to merge; the only concern is the non-blocking test-suite runtime increase from long real sleeps. Item indices are unique for initial submissions and intentionally preserved by duplicate futures, so the change enforces the intended one-resubmission invariant without altering result alignment. The regression tests accurately cover the behavior but add roughly 15 seconds of avoidable runtime. Files Needing Attention: tests/utils/test_parallelizer.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Original future becomes a straggler] --> B{Item index already resubmitted?}
B -->|Yes| C[Continue waiting]
B -->|No| D[Record item index]
D --> E[Submit one duplicate future]
E --> F[Original and duplicate race]
F --> G[No future for that index can trigger another resubmit]
Reviews (1): Last reviewed commit: "fix(utils): key ParallelExecutor straggl..." | Re-trigger Greptile |
|
|
||
| def f(i): | ||
| with lock: | ||
| counter[i] = counter.get(i, 0) + 1 |
There was a problem hiding this comment.
The three regression tests each use a five-second real sleep, and execute waits for both racing futures to finish. This adds roughly 15 seconds of avoidable wall-clock time to every test run. Please use synchronization or controlled timing to exercise the resubmission path without repeatedly waiting five seconds.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Warning
GitHub issue creation failed
Detail attempted to publish this bug to GitHub, but the issue could not be created. This fix PR was created without that issue, and missing tracker references are shown as
Unknown issue.You can review and merge this PR normally. Please review your tracker integration settings before the next publish run.
Detail bug report: View on Detail
📝 Changes Description
Closes Unknown issue
This MR/PR contains the following changes:
Bug:
ParallelExecutor._execute_parallel(dspy/utils/parallelizer.py) resubmits "straggler" tasks whose runtime exceedstimeoutto race the original. The dedup set enforcing the code's own "at most once per item" invariant was keyed onFutureobjects, not on item indices. When a stragglerffor itemidxwas resubmitted asnf, onlyfwas recorded;nfwas added to the in-flight set but never marked as already-resubmitted. On later straggler passesnfpassed the dedup guard, and oncenfitself ran ≥timeoutit got resubmitted asnf2, producing a chainf → nf → nf2 → …. A slow-but-completing item was invoked up tomin(num_threads, straggler_limit + 1)times (4× under defaults) instead of the intended 2× (1 original + 1 resubmit) — extra redundant LM calls, each with its own API/token cost.Fix: Track item indices (the true per-item key) in the dedup set —
resubmitted→resubmitted_indices,if f not in resubmitted→if idx not in resubmitted_indices,resubmitted.add(f)→resubmitted_indices.add(idx). Onceidxis marked, no future for that item (original or any duplicate) can trigger a further resubmit, enforcing exactly one resubmit per item regardless of how many duplicate futures race.resubmitted_indicesis touched only on the main thread inside the straggler loop, so the check is race-free.Regression tests added (
tests/utils/test_parallelizer.py):test_straggler_resubmits_at_most_once_per_item— the headline invariant: a slow-but-completing item is invoked exactly twice.test_straggler_resubmits_at_most_once_multiple_slow_items— confirms dedup is per-item, not global.test_high_straggler_limit_no_extra_resubmit— the worst-case config (num_threads=8, straggler_limit=10) where the bug previously produced the most extras now caps at 2.✅ Contributor Checklist
fix(utils): ...Testing
tests/utils/test_parallelizer.pypass (14 pre-existing + 3 new), covering worker independence,active_call_idpropagation,max_errorshandling, sequential mode,compare_results, and the new straggler invariant.ruff checkandruff format --checkare clean ondspy/utils/parallelizer.py(the fix file). The repo configures no static type checker.dspy.Parallel(the public wrapper exposingtimeout/straggler_limit): a stubbed workload with one 5s item and three fast items confirmed the slow item is invoked exactly twice, anddspy.Parallel(timeout=0)disables resubmission entirely (each item once). No LM/network access — the straggler logic is pure Python concurrency.tests/utils/test_parallelizer.py(2RUF043warnings at lines 62/147 and 4 format suggestions at lines 124/141/156/182) predate this PR — confirmed by reverting the change and re-running ruff, which produces identical output. They are in untouched sequential-mode tests; not introduced or addressed here.totalwhen a resubmitted future completes after its original. This is inherent to the racing design (including the intended single resubmit) and is neither caused by, nor addressed by, this fix.Authored by Detail: Automatic Fixes — an AI-assisted contribution; the human author reviewed and is submitting it. The bug was reproduced independently before writing the fix.
Automatic Fixes PRs can be configured here.