feat(config): forward num_retries to LiteLLM per call#171
Conversation
Add a num_retries per-call config plumbing that mirrors the existing timeout pattern. resolve_num_retries validates a non-negative int (rejects bools, floats, and strings — no coercion, unlike timeout) and a process-wide stash (set_num_retries/get_num_retries) is populated by _setup_llm_key from both the legacy top-level key and the litellm: block. The LLM call sites in compiler.py forward it to litellm.(a)completion via kwargs.setdefault. Default is None (opt-in via config) — zero behavior change for non-configurers. Refs VectifyAI#164.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@01luyicheng can you help to resolve the conflicts? Thanks |
|
Thanks — the plumbing is clean. But one thing before we take it: Really appreciate the contribution. |
|
Thanks for the review and the clear explanation. You're right that |
Forward
num_retriesto LiteLLM per callCloses #164.
Problem
Issue #164 reports that when a provider returns a transient failure
(Gemini
503 ServiceUnavailableError— "model is currently experiencinghigh demand"), OpenKB does not retry: the step fails and is skipped
(e.g.
5 concept(s) planned but only 4 written … (ServiceUnavailableError)).LiteLLM supports a
num_retriesknob for exactly this, but OpenKB onlyexposed it as a process-wide
litellm.<key>global, which is not appliedper request and is easy to mis-set.
Approach
Mirror the existing
timeoutplumbing (the proven precedent forper-call LLM tuning):
resolve_num_retries(config)— validates a non-negative int at theconfig boundary. Returns
None(use LiteLLM's default) when absent orinvalid. Rejects bools (int subclass), floats, and strings — a retry
count is discrete, so no numeric coercion is applied (the one
deliberate divergence from
resolve_timeout, documented in its docstring).set_num_retries/get_num_retries/_runtime_num_retries— aprocess-wide stash, populated by
cli._setup_llm_keyfrom both thelegacy top-level
num_retries:key and thelitellm:block (the blockvalue wins, matching timeout's precedence).
compiler._llm_call/_llm_call_asyncforward it tolitellm.completion/litellm.acompletionviakwargs.setdefault,so an explicit per-call value is never clobbered.
Default =
None(zero behavior change)The new knob is opt-in. Nothing is forwarded when
num_retriesisunset, so existing configs behave identically and LiteLLM keeps applying
its own default. This deliberately does not change retry behavior for
non-configurers.
How to enable
Decisions a maintainer may want to revisit
timeout's direct call-site use,num_retriesis forwarded atcompiler._llm_call(_async)— the pathnamed in Requesting retry logic when model is overloaded #164 (concept/entity/summary-rewrite). Unlike
timeout,this PR does not add an Agents-SDK analogue of
get_timeout_extra_args(), so the knob does not currently applyto the chat / query / lint / skill-eval / skill-creator paths that run
LLM calls through
openai-agentsModelSettings. A user settingnum_retries: 3will see retries onopenkb add/compilebut noton
openkb chat/query/lint. This is intentional for this PR(keeps it small and focused on the reported issue) but is an asymmetry
vs.
timeout; a follow-up can addget_num_retries_extra_args()oncewe confirm how
ModelSettingsexposes retries.None, this does notauto-fix Requesting retry logic when model is overloaded #164 for users who don't add the key. If the maintainers
prefer to ship retries by default, that's a one-line follow-up — kept
out of this PR to preserve zero behavior change.
Tests
tests/test_config.py— resolve/stash for absent, int,0, negative,bool, float, string, non-int containers.
tests/test_llm_config_passthrough.py—litellm:block routesnum_retriesper-call (not as a module global).tests/test_llm_retries.py(new) — sync + async call sites: forwardsconfigured value, omits when unset, does not override an explicit
per-call kwarg (setdefault semantics).
All 958 tests pass;
ruff check,ruff format --check, andmypy openkbare clean.