fix(dspy): Unbatchify rejects batch_fn output length mismatch - #80
Open
detail-app[bot] wants to merge 1 commit into
Open
Conversation
Greptile SummaryThis PR prevents
Confidence Score: 5/5The PR appears safe to merge and correctly rejects mismatched batch output without partially resolving callers. The length check runs before any future is resolved, and the existing exception path delivers the mismatch error to every pending future while allowing the worker to continue. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Collect queued inputs and futures] --> B[Invoke batch_fn]
B --> C{Output count matches future count?}
C -->|Yes| D[Resolve each future with its output]
C -->|No| E[Raise ValueError]
E --> F[Set exception on all batch futures]
F --> G[Worker continues processing later batches]
Reviews (1): Last reviewed commit: "fix(dspy): Unbatchify rejects batch_fn o..." | Re-trigger Greptile |
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
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.
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
This MR/PR contains the following changes:
Unbatchify(dspy/utils/unbatchify.py) routesbatch_fn's returned outputs back to pending caller futures withzip(outputs, futures, strict=False). Whenbatch_fnreturns a list whose length differs from the number of submitted inputs, the zip silently truncates to the shorter side: outputs shift onto the wrong callers (callers receive each other's results), and any trailing future with no corresponding output is left forever unset. Because__call__waits onfuture.result()with no timeout, the orphaned caller hangs indefinitely with no error raised.close()cannot rescue it once the(input, future)pair has been dequeued into the worker.len(outputs) == len(futures)before calling anyset_result, raisingValueErrorso the existingexceptpath safely propagates the exception to every still-pending future. Because noset_resultruns before the check passes, mis-pairing is impossible, no future is orphaned, the worker thread survives, and all callers receive a clear error.strict=True: A one-characterzip(..., strict=True)change is strictly worse —strict=Trueraises only after yielding every short-side pair, soset_resulthas already run on the leading futures; the existingexceptblock then callsset_exceptionon already-FINISHEDfutures, raisingInvalidStateErrorinside the handler, which escapes_workerand crashes the worker thread (converting a per-batch failure into a permanent per-object failure). Validating the length before iterating avoids this entirely.Closes Unknown issue
✅ Contributor Checklist
None. This is a latent hardening fix: the only in-tree
batch_fnpassed toUnbatchify(Embeddings._batch_forward→_rerank_and_predict) always returns exactlylen(queries)tuples by construction, so no current code path is affected. The fix closes a dormant trap for futurebatch_fnimplementations that could return a mismatched-length list.Testing:
tests/utils/test_unbatchify.pycovering: shortbatch_fnoutput rejected, longbatch_fnoutput rejected, end-to-end__call__raises and does not hang on a short output, and worker thread survives a mismatched batch to service subsequent calls.tests/utils/test_unbatchify.pytests (batch-size trigger, timeout trigger, max_wait_time re-arming) andtests/retrievers/test_embeddings.py(the only in-tree consumer) pass unchanged — no regression.Authored by Detail.
Automatic Fixes PRs can be configured here.