mirror of
https://github.com/lancedb/lancedb.git
synced 2026-10-23 12:12:27 +00:00
fix(python): stop retrying a missing API key or provider package (#4521)
`retry_with_exponential_backoff` retries every exception except an auth
error (`python/python/lancedb/embeddings/utils.py:298-318`). For Cohere,
Gemini, Jina and VoyageAI a missing API key raises from
`api_key_not_found_help`, and a missing provider package raises
`ImportError` everywhere. Neither can succeed on a retry, yet both sleep
through all seven rounds and then surface wrapped in `Exception("Maximum
number of retries...")`.
Before, on `142d032`, a VoyageAI function with no key set (script below,
`time.sleep` recorded, `random.seed(0)`):
```
$ python lance_retry_deterministic.py python/python
raised: Exception('Maximum number of retries (7) exceeded.', ValueError('Please set the VOYAGEAI_API_KEY environment v
attempts: 8 sleeps: [4, 13, 37, 93, 280, 788, 2811]
slept 4026 s before a missing-API-key error surfaced
```
After:
```
$ python lance_retry_deterministic.py python/python
raised: MissingApiKeyError('Please set the VOYAGEAI_API_KEY environment variable.')
attempts: 1 sleeps: []
```
<details><summary>lance_retry_deterministic.py</summary>
```python
# loads the pure-Python embeddings package from the tree, native extension stubbed
import os, random, sys, time, types
TREE = sys.argv[1]
pkg = types.ModuleType("lancedb"); pkg.__path__ = [TREE + "/lancedb"]; sys.modules["lancedb"] = pkg
native = types.ModuleType("lancedb._lancedb"); native.__getattr__ = lambda name: None
sys.modules["lancedb._lancedb"] = native
import pyarrow as pa
from lancedb.embeddings import get_registry
random.seed(0)
os.environ.pop("VOYAGE_API_KEY", None)
slept = []
time.sleep = slept.append
func = get_registry().get("voyageai").create(name="voyage-3")
try:
func.compute_source_embeddings_with_retry(pa.array(["hello"]))
except Exception as e:
print("raised:", repr(e)[:110])
print("attempts:", len(slept) + 1, "sleeps:", [round(s) for s in slept])
print("slept %.0f s before a missing-API-key error surfaced" % sum(slept))
```
</details>
The message names the wrong variable: `voyageai.py:501` reads
`VOYAGE_API_KEY`. I left that for a separate fix.
`api_key_not_found_help` now raises `MissingApiKeyError`, a `ValueError`
subclass, so existing `except ValueError` callers still catch it. That
error and `ImportError` join the 401 exemption from #2995. A plain
`ValueError` is still retried, because `bedrock.py:171` wraps boto3
throttling in one. No issue exists for this. OpenAI is not covered: with
no key, `openai.OpenAI()` raises `OpenAIError`, which has no status
code, so it still retries. I can follow up on that.
### Verification
```
# lance_stub: an empty stand-in for lancedb._lancedb, loaded from the source tree
$ pytest -p lance_stub --noconftest python/tests/test_embeddings.py -k "retry or 401 or deterministic" -q
5 passed, 23 deselected in 0.59s
# utils.py from 142d032, tests kept:
2 failed, 3 passed, 23 deselected in 0.74s
$ pytest -p lance_stub --noconftest python/tests/test_voyageai_embeddings.py -q
18 passed in 0.18s
# not run: the full suite or pyright; the native extension did not build here
```
Baseline on `142d032` has 2 of these tests. The new ones cover a missing
key, a missing package, and a wrapped throttling error that must still
retry. ruff 0.16.10 check and format are clean.
@wjones127 you merged the 401 change. Is a `ValueError` subclass the way
you want to mark this?
Co-authored-by: Xuanwo <github@xuanwo.io>
This commit is contained in:
co-authored by
Xuanwo
parent
d826b6115a
commit
b6d15558ec
@@ -320,6 +320,11 @@ def _is_non_retryable_error(error: Exception) -> bool:
|
||||
if error.status_code in (401, 403):
|
||||
return True
|
||||
|
||||
# A missing API key or a missing optional dependency fails the same way on
|
||||
# every attempt
|
||||
if isinstance(error, (MissingApiKeyError, ImportError)):
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
|
||||
@@ -337,9 +342,15 @@ def url_retrieve(url: str):
|
||||
raise ConnectionError("could not download {} due to {}".format(url, err))
|
||||
|
||||
|
||||
class MissingApiKeyError(ValueError):
|
||||
"""Raised when an embedding provider's API key is not set."""
|
||||
|
||||
|
||||
def api_key_not_found_help(provider):
|
||||
logging.error("Could not find API key for %s", provider)
|
||||
raise ValueError(f"Please set the {provider.upper()}_API_KEY environment variable.")
|
||||
raise MissingApiKeyError(
|
||||
f"Please set the {provider.upper()}_API_KEY environment variable."
|
||||
)
|
||||
|
||||
|
||||
def is_flash_attn_2_available():
|
||||
|
||||
@@ -633,6 +633,51 @@ def test_openai_no_retry_on_401(mock_sleep):
|
||||
assert mock_sleep.call_count == 0
|
||||
|
||||
|
||||
@patch("time.sleep")
|
||||
@pytest.mark.parametrize(
|
||||
"error",
|
||||
[
|
||||
pytest.param("missing_key", id="missing-api-key"),
|
||||
pytest.param(ImportError("No module named 'voyageai'"), id="missing-package"),
|
||||
],
|
||||
)
|
||||
def test_no_retry_on_deterministic_errors(mock_sleep, error):
|
||||
from lancedb.embeddings.utils import (
|
||||
api_key_not_found_help,
|
||||
retry_with_exponential_backoff,
|
||||
)
|
||||
|
||||
def func():
|
||||
if error == "missing_key":
|
||||
api_key_not_found_help("voyageai")
|
||||
raise error
|
||||
|
||||
mock_func = MagicMock(side_effect=func)
|
||||
wrapped_func = retry_with_exponential_backoff(mock_func, max_retries=3)
|
||||
|
||||
with pytest.raises((ValueError, ImportError)):
|
||||
wrapped_func()
|
||||
|
||||
assert mock_func.call_count == 1
|
||||
assert mock_sleep.call_count == 0
|
||||
|
||||
|
||||
@patch("time.sleep")
|
||||
def test_retry_on_wrapped_provider_error(mock_sleep):
|
||||
"""Providers such as Bedrock wrap throttling in a plain ValueError."""
|
||||
from lancedb.embeddings.utils import retry_with_exponential_backoff
|
||||
|
||||
mock_func = MagicMock(
|
||||
side_effect=[
|
||||
ValueError("Error raised by boto3 client: ThrottlingException"),
|
||||
[0.1],
|
||||
]
|
||||
)
|
||||
wrapped_func = retry_with_exponential_backoff(mock_func, max_retries=3)
|
||||
|
||||
assert wrapped_func() == [0.1]
|
||||
assert mock_func.call_count == 2
|
||||
assert mock_sleep.call_count == 1
|
||||
def test_openai_bad_request_partial_batch_recovery():
|
||||
# Regression test for issue #1677
|
||||
|
||||
|
||||
Reference in New Issue
Block a user