Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions mnemosyne/core/embeddings.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@

import json
import os
import random
import ssl
import time
import urllib.error
import urllib.request
from typing import List, Optional
from functools import lru_cache
Expand Down Expand Up @@ -231,6 +234,16 @@ def _is_rate_limit_error(exc: BaseException) -> bool:
return False


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
Comment on lines +237 to +244

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.



def _embed_api(texts: List[str]) -> Optional[np.ndarray]:
"""Embed texts via OpenAI-compatible API (OpenRouter or custom endpoint)."""
global _API_CALL_COUNT
Expand Down Expand Up @@ -269,9 +282,8 @@ def _embed_api(texts: List[str]) -> Optional[np.ndarray]:
_API_CALL_COUNT += 1
return np.array(embeddings, dtype=np.float32)
except Exception as e:
if "429" in str(e) or "rate" in str(e).lower():
import time
time.sleep(2 ** attempt)
if _is_transient_embedding_error(e) and attempt < 2:
time.sleep((0.5 * (2 ** attempt)) + random.uniform(0.0, 0.25))
continue
return None

Expand Down
80 changes: 80 additions & 0 deletions tests/test_embedding_api_retry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
"""Regression coverage for transient embedding endpoint failures."""

import io
import json
import urllib.error
from unittest.mock import patch

import pytest

from mnemosyne.core import embeddings


class Response:
def __init__(self, payload):
self.body = io.BytesIO(json.dumps(payload).encode())

def __enter__(self):
return self

def __exit__(self, *_args):
return False

def read(self):
return self.body.read()


def test_embed_api_retries_transient_network_failures(monkeypatch):
monkeypatch.setenv("MNEMOSYNE_EMBEDDING_API_URL", "http://127.0.0.1:11435/v1")
result = Response({"data": [{"embedding": [0.25, 0.75]}]})
failures = [urllib.error.URLError(OSError(65, "No route to host")), TimeoutError(), result]

with patch("urllib.request.urlopen", side_effect=failures) as request, \
patch("mnemosyne.core.embeddings.random.uniform", return_value=0.1), \
patch("mnemosyne.core.embeddings.time.sleep") as sleep:
vectors = embeddings._embed_api(["retry me"])

assert vectors.tolist() == [[0.25, 0.75]]
assert request.call_count == 3
assert [call.args[0] for call in sleep.call_args_list] == [0.6, 1.1]


@pytest.mark.parametrize("status", [429, 503])
def test_embed_api_retries_transient_http_errors(monkeypatch, status):
monkeypatch.setenv("MNEMOSYNE_EMBEDDING_API_URL", "http://127.0.0.1:11435/v1")
error = urllib.error.HTTPError("http://example", status, "transient", {}, None)
result = Response({"data": [{"embedding": [0.25, 0.75]}]})

with patch("urllib.request.urlopen", side_effect=[error, result]) as request, \
patch("mnemosyne.core.embeddings.random.uniform", return_value=0), \
patch("mnemosyne.core.embeddings.time.sleep") as sleep:
vectors = embeddings._embed_api(["retry me"])

assert vectors.tolist() == [[0.25, 0.75]]
assert request.call_count == 2
sleep.assert_called_once_with(0.5)


def test_embed_api_does_not_retry_nontransient_client_error(monkeypatch):
monkeypatch.setenv("MNEMOSYNE_EMBEDDING_API_URL", "http://127.0.0.1:11435/v1")
error = urllib.error.HTTPError("http://example", 400, "bad request", {}, None)

with patch("urllib.request.urlopen", side_effect=error) as request, \
patch("mnemosyne.core.embeddings.time.sleep") as sleep:
assert embeddings._embed_api(["bad request"]) is None

assert request.call_count == 1
sleep.assert_not_called()


def test_embed_api_stops_after_three_transient_attempts(monkeypatch):
monkeypatch.setenv("MNEMOSYNE_EMBEDDING_API_URL", "http://127.0.0.1:11435/v1")
error = urllib.error.URLError(OSError(65, "No route to host"))

with patch("urllib.request.urlopen", side_effect=error) as request, \
patch("mnemosyne.core.embeddings.random.uniform", return_value=0), \
patch("mnemosyne.core.embeddings.time.sleep") as sleep:
assert embeddings._embed_api(["offline"]) is None

assert request.call_count == 3
assert sleep.call_count == 2
Loading