Embedding retries - #1175
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The embedding retry loop still misses some transport failure cases and fatal-error semantics, and the new timeout/retry validation behavior isn’t covered by tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes create.embeddings/wrangles.openai.embeddings retry behavior for transport failures and introduces a configurable per-attempt request timeout to make embedding generation more resilient.
Changes:
- Add
timeoutparameter to recipecreate.embeddingsand plumb it through towrangles.openai.embeddings. - Rework embedding retry loop to handle timeouts/connection failures and improve retry logging/error messages.
- Add input validation for
retriesandtimeoutinwrangles.openai.embeddings.
Recommended disposition: Request changes
Next steps
- PR assignee: Apply the inline fixes in
wrangles/openai.py(broaden transport exception handling; raise fatal OpenAI errors consistently) and add tests coveringtimeoutpass-through + validation. - Reviewer: Verify the updated retry behavior with mocked transport failures and confirm the new timeout parameter is exercised; then resolve threads and re-review.
File summaries
| File | Description |
|---|---|
wrangles/recipe_wrangles/create.py |
Exposes timeout in the recipe-facing create.embeddings API/schema and forwards it to the OpenAI embeddings backend. |
wrangles/openai.py |
Implements per-attempt timeout, fixes retry semantics for transport errors, and tightens parameter validation/error reporting for embeddings. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ebhills
approved these changes
Sep 9, 2026
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.
Summary
Fixes
create.embeddingsretries that stopped after the first failed HTTP or transport attempt, and adds a configurable per-attempttimeout.The regression was introduced when embeddings adopted the shared OpenAI retry-classification helpers in #1124:
requests.Responseevaluates false for 4xx/5xx responses, so the old truthiness guard discarded the response context and classified retryable failures as permanent. This change checks response presence explicitly and keeps embeddings independent ofextract.aipackaged defaults.What changed
Retry-After.model_not_foundresponses without applying OpenAI-specific fatal handling to Jina.timeoutin bothwrangles.openai.embeddingsand recipecreate.embeddings.retriesand add focused mocked regression coverage using real false-valuedrequests.Responseobjects.main.Compatibility and behavior
retriesmeans additional attempts. Each retry receives the full configured timeout; there is no total batch deadline.Retry-After.Validation
Passed locally with mocked network calls; no live OpenAI or Jina requests were made:
extract.aitransport tests.git diff --check origin/main...HEAD.GitHub CI will remain the authoritative full-suite check for the pushed head.
Risk and rollback
The primary risk is retry classification for less common
requeststransport failures. Tests cover connection, timeout, TLS, proxy, chunked-transfer, content-decoding, permanent URL, HTTP, authentication, and model errors.Rollback by reverting the embeddings retry commits in this PR. No configuration migration or stored-data change is required.