Repository navigation
Stop retry multiplication on ApiModel.retryer replays (OpenAIModel, LiteLLMModel) and make is_rate_limit_error status-first - #3036
Conversation
simin-yuan
changed the title
b2aa4f8 to
d2af421
Compare
On a rate limit, OpenAIModel sends 9 requests: ApiModel.retryer makes 3 attempts and the openai client inside each one keeps its default max_retries=2. This keeps the client's own retries on the first attempt and uses self.client.with_options(max_retries=0) only on the retryer's replays, so a 429 costs 3 + 1 + 1 = 5 requests. 5xx errors, dropped connections and timeouts are unchanged, because the retryer does not retry them and the client's retries are their only retry. Closes huggingface#3034.
Two ways one logical call still became more requests than it should. LiteLLMModel had the same multiplication the OpenAIModel fix addresses: litellm retries internally (DEFAULT_MAX_RETRIES=2) underneath smolagents' retryer, so a single call is up to 9 requests. The retryer's replays now pass num_retries=0; the first attempt keeps the client's own retries, matching the OpenAIModel change in this branch. is_rate_limit_error only looked at str(exception), so a 429 whose body says "quota" was not recognised (the retryer gave up on a retryable error) while a 500 whose body happened to contain "429" was treated as a rate limit and retried nine times. A status code is now authoritative in both directions; the message heuristics only apply when there is no status.
A local server that answers 429 to every request, plus the count of requests one generate() call spends. Run by hand; it is not collected by pytest.
d2af421 to
122151a
Compare
|
AI-assisted, source-only review of One small reproducibility-label issue: line 70 searches Could this inspect |
|
Thanks, you're right, and it was worse than the label. Fixed in a29682f (re-committed as a8778c8 to correct the author email; the code is unchanged). The script now inspects the closure returned by With the fixed script the two sides come out as:
The request counts in the PR description are not affected. The local server logs one line per request, and the count is read after The same commit also fixes ruff errors this file already had (blank lines and line wrapping, no logic change). |
|
Thanks. Source review of |
…nit__ `smolagents.__file__` is `src/smolagents/__init__.py`, and `_completion_create` is defined in `models.py`, so the label was false on this branch as well: 1.26.0 and this branch both recorded `num_retries_patched: false`, which makes the field carry no information. The label now asks the object the fix lives on: it checks that `num_retries` appears among the code constants of the closure returned by `LiteLLMModel._completion_create`. A bare `hasattr` would not tell a method that exists from a method that still retries. Re-run with the fixed label: - this branch: 5 requests, `num_retries_patched: true` - 1.26.0 from PyPI: 9 requests, `num_retries_patched: false` The file was also not ruff-clean (`quality.yml` runs `ruff check examples src tests`), so the blank lines and line wrapping it asks for are applied here too; the reformatting is whitespace only.
a29682f to
a8778c8
Compare
|
Thanks for the re-read. The request counts stay on me to validate — I'll post the measured numbers here once I have them, rather than leave the claim standing on the source argument alone. |
a8778c8 to
fa9758a
Compare
|
Numbers, as promised — and where they come from.
5 = 3 on the first attempt (client retries) + 1 per replay, with Honest scope: these are the numbers recorded when the branch was written; I have not re-run the script today, and the harness in the branch is a rewrite of the throwaway script used for the first run (noted in the PR body). The command is The predicate half is independent of these counts: the status-first tests encode the three message-only cases (429 with no status, 500 whose body says "429", 400 whose body says "rate limit") and fail on the base commit. |
|
Thank you for this pull request. Under our contribution guidelines, a pull request needs to link an issue that a maintainer has assigned to its author before we review it. Assignment is how we decide what gets worked on and by whom, and it keeps the review queue at a size we can actually handle. This PR links #3034, which is not assigned to you, so I am closing the PR for now. If we decide we want this change and assign it to you, comment here and we will reopen this PR as it is: please do not open a new one. Closing it is not a judgement on the code. |
albertvillanova
added
the
needs-assigned-issue
Fixes #3034. Related: #3037.
ApiModel.retryerwraps a callable that already has its own retry loop underneath it. In both OpenAI-backed models that produced N×M requests for a single logical call, and the predicate that decides whether an error is worth retrying answered the wrong question.What this branch does
OpenAIModel(unchanged from the earlier revision): the retryer's replays go through a client copy withmax_retries=0, so the two retry layers no longer multiply.LiteLLMModel(new): the same fix, one layer deeper.litellm.completionretries internally (DEFAULT_MAX_RETRIES = 2), so the retryer's replays now passnum_retries=0while the first attempt keeps the client's own retries. Measured withtests/_manual/measure_retry_requests.py(a local server that answers 429 to every request), onegenerate()on 1.26.0 spends 9 requests / 872.11 s; with this change it spends 5 requests / 424.87 s (3 on the first attempt plus 1 per replay). Same server, same model id, defaultRETRY_WAIT— only the code changed.The measurement harness is rebuilt: the first run happened on a throwaway script that was not kept, so the script in this branch is a rewrite of it. Both numbers above come from that script on the two versions — re-run it to check them.
is_rate_limit_erroris status-first (new): it only inspectedstr(exception), so:LiteLLMModelgave up after 19.1 s whileOpenAIModel(namedOpenAIServerModelin 1.26.0) kept retrying for 590.4 s — the only difference was the wording of the error message, andA status code is now authoritative in both directions; the message heuristics only apply when the exception carries no status at all.
Coverage, explicitly
OpenAIModelLiteLLMModelLiteLLMRouterModelLiteLLMModel._completion_createAzureOpenAIModel,InferenceClientModel,AmazonBedrockModelTests
Four new test functions, 11 test items after parametrization (8 predicate cases plus 3 single tests). Six of the 11 fail against the base commit — the three predicate cases that encode the bug (429 on a plain exception; 500 whose body mentions 429; 400 whose body says "rate limit"), the
litellm.RateLimitErrorcase, and both retry-accounting tests (LiteLLMModelpassesnum_retries=0on replays;generate()actually routes through that path) — and all 11 pass with the change. Collection goes from 123 items on the base commit to 134 on this branch.pytest tests/test_models.py: 120 passed, 6 failed, 7 skipped, 1 error. The six failures and the error areTransformersModel/MLXModeltests that need torch; they fail identically on the base commit (109 passedthere — the 11 extra passes are the new items).AI involvement
This patch was written with an AI assistant, under my direction. I have read the diff, the measurements above were run on this machine against the versions stated, and I will answer review comments.
Why one PR instead of two
Both are the same defect seen from two sides: a retry layer that decides "retry me" from a substring, and retry layers stacked on top of each other. Splitting them would leave the LiteLLM multiplication unfixed in a thread that is specifically about request multiplication. Happy to split if you would rather review them separately.