Retry transient embedding API failures#476
Conversation
|
|
📝 WalkthroughWalkthrough
ChangesEmbedding retry handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_embedding_api_retry.py (1)
1-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood assertions, but coverage gaps remain in
_is_transient_embedding_error's classification surface.All four tests assert concrete outcomes (call counts, exact sleep values, returned data) rather than just "doesn't crash" — solid. However, the classifier being tested (
_is_transient_embedding_error) has three distinct branches and this suite only exercises two of them:
- The string-match fallback branch (
"429"/"rate limit"/"rate-limit"instr(exc)for exceptions that aren'tHTTPError/URLError/TimeoutError/ConnectionError/OSError) is never exercised by any test here.- Bare
ConnectionError/genericOSErrorraised directly (not wrapped inURLError) aren't tested, even though the classifier treats them as transient.- The HTTP-transient boundary is only tested at 429/503 — the edges of
500 <= exc.code < 600(500 and 599) aren't tested.- Non-transient coverage is only at 400 — no test for a non-transient status just outside the transient range (e.g. 600) or another common 4xx (e.g. 404).
Given this suite exists specifically to validate the retry/backoff and classification contract, closing these gaps would meaningfully strengthen confidence in
_is_transient_embedding_error's edge behavior.As per path instructions, "Flag missing or weak test scenarios" and "Verify that new tests actually assert something meaningful" for files under
tests/**.🤖 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 `@tests/test_embedding_api_retry.py` around lines 1 - 81, Extend the tests covering embeddings._is_transient_embedding_error to exercise its string-match fallback with a non-specialized exception, direct ConnectionError and OSError cases, HTTP status boundaries 500 and 599, and non-transient statuses 404 and 600. Keep assertions explicit about classification or resulting retry behavior, reusing the existing retry test setup and avoiding tests that only verify the call does not raise.Source: Path instructions
🤖 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 `@mnemosyne/core/embeddings.py`:
- Around line 237-244: Update _is_transient_embedding_error to reuse the
existing _is_rate_limit_error helper for its fallback classification instead of
duplicating message matching, preserving the helper’s handling of “too many
requests” and other rate-limit cases while leaving HTTP and transport-error
handling unchanged.
---
Outside diff comments:
In `@tests/test_embedding_api_retry.py`:
- Around line 1-81: Extend the tests covering
embeddings._is_transient_embedding_error to exercise its string-match fallback
with a non-specialized exception, direct ConnectionError and OSError cases, HTTP
status boundaries 500 and 599, and non-transient statuses 404 and 600. Keep
assertions explicit about classification or resulting retry behavior, reusing
the existing retry test setup and avoiding tests that only verify the call does
not raise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1615d9e6-444a-488e-9731-fd9bec35d9a3
📒 Files selected for processing (2)
mnemosyne/core/embeddings.pytests/test_embedding_api_retry.py
| def _is_transient_embedding_error(exc: Exception) -> bool: | ||
| """Return whether an API embedding request is safe to retry.""" | ||
| if isinstance(exc, urllib.error.HTTPError): | ||
| return exc.code == 429 or 500 <= exc.code < 600 | ||
| if isinstance(exc, (urllib.error.URLError, TimeoutError, ConnectionError, OSError)): | ||
| return True | ||
| message = str(exc).lower() | ||
| return "429" in message or "rate limit" in message or "rate-limit" in message |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Duplicates and diverges from the existing _is_rate_limit_error helper.
The fallback branch reimplements the rate-limit string matching that _is_rate_limit_error (Line 221) already provides, but drops the "too many requests" case that the existing helper checks. This creates two slightly different classifiers for the same concept — an embedding API error whose message is "Too many requests" (no "429" or "rate limit" phrase) won't be retried here even though _is_rate_limit_error would flag it as transient.
♻️ Proposed fix: reuse the existing helper
def _is_transient_embedding_error(exc: Exception) -> bool:
"""Return whether an API embedding request is safe to retry."""
if isinstance(exc, urllib.error.HTTPError):
return exc.code == 429 or 500 <= exc.code < 600
if isinstance(exc, (urllib.error.URLError, TimeoutError, ConnectionError, OSError)):
return True
- message = str(exc).lower()
- return "429" in message or "rate limit" in message or "rate-limit" in message
+ return _is_rate_limit_error(exc)📝 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.
| def _is_transient_embedding_error(exc: Exception) -> bool: | |
| """Return whether an API embedding request is safe to retry.""" | |
| if isinstance(exc, urllib.error.HTTPError): | |
| return exc.code == 429 or 500 <= exc.code < 600 | |
| if isinstance(exc, (urllib.error.URLError, TimeoutError, ConnectionError, OSError)): | |
| return True | |
| message = str(exc).lower() | |
| return "429" in message or "rate limit" in message or "rate-limit" in message | |
| def _is_transient_embedding_error(exc: Exception) -> bool: | |
| """Return whether an API embedding request is safe to retry.""" | |
| if isinstance(exc, urllib.error.HTTPError): | |
| return exc.code == 429 or 500 <= exc.code < 600 | |
| if isinstance(exc, (urllib.error.URLError, TimeoutError, ConnectionError, OSError)): | |
| return True | |
| return _is_rate_limit_error(exc) |
🤖 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 `@mnemosyne/core/embeddings.py` around lines 237 - 244, Update
_is_transient_embedding_error to reuse the existing _is_rate_limit_error helper
for its fallback classification instead of duplicating message matching,
preserving the helper’s handling of “too many requests” and other rate-limit
cases while leaving HTTP and transport-error handling unchanged.
|
Merged in 6b4de5f. @mia-fourier thanks for the retry tests. The retry logic already shipped in #478 (raitoxlol), but your focused test coverage is a valuable addition. Your second contribution after #206 (embedding API URL independence, back in May). Solid follow-up. Edit: @mia-fourier — I originally called this your "first contribution" which is wrong. It's your second. Your first was #206. My bad, not tracking contributor history properly. |
|
@AxDSan It's actually my second, but thanks for the recognition. I have a few other ideas I'm thinking about posting as issues fro feature requests to provide fine grained access control over memory. Will post more when I have it fleshed out, but I think it's going to be a good one. Think "Private", "Confidential", "Public" there's a bit more I'm putting together a spec and design document, but it's on the back burner while a few other things I am working on get released. |
3 participants
Description
This change adds bounded retry handling for transient embedding endpoint failures in
_embed_api.Related Issue
Closes #475
Type of Change
How Has This Been Tested?
git diff --checkThe manual check used the cached quantized
BAAI/bge-small-en-v1.5ONNX model behind a temporary loopback OpenAI-compatible endpoint. The endpoint returned HTTP 503 for the first request and a real 384-dimensional embedding for the retry.The current upstream
mainfull suite has an unrelated baseline failure after 85 passes intests/test_beam.py::TestMnemosyneIntegration::test_legacy_and_beam_dual_write:Mnemosynehas nobeamattribute and__del__references an undefinedsession_id. This branch does not modifymnemosyne/core/memory.py.Checklist
mnemosyne/__init__.py(maintainer release task)CHANGELOG.mdupdated (maintainer release task)Summary
Adds bounded retries for transient embedding API failures, including network/timeouts, HTTP 429, and HTTP 5xx responses. Retries use exponential backoff with jitter and stop after three attempts, while non-transient errors such as HTTP 400 fail immediately.
Architectural impact
This is the right call for resilience: bounded, jittered retries address transient provider failures without masking permanent client errors or changing Mnemosyne’s core architecture.