feat(provider): include provider id and model name in retry warning logs | 在重试警告日志中包含提供商 ID 和模型名称 - #9670
Open
SweetenedSuzuka wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The repeated pattern
str(self.provider_config.get("id", ""))across provider sources suggests extracting a small helper (e.g.,get_provider_id_str(self.provider_config)) to reduce duplication and keep the logging metadata consistent. - For OpenAI retry calls,
modelfalls back to""while Anthropic usesself.get_model()and Gemini uses the localmodelvariable; consider aligning OpenAI to use a meaningful fallback (e.g.,self.get_model()) so retry logs always reflect the effective model when available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated pattern `str(self.provider_config.get("id", ""))` across provider sources suggests extracting a small helper (e.g., `get_provider_id_str(self.provider_config)`) to reduce duplication and keep the logging metadata consistent.
- For OpenAI retry calls, `model` falls back to `""` while Anthropic uses `self.get_model()` and Gemini uses the local `model` variable; consider aligning OpenAI to use a meaningful fallback (e.g., `self.get_model()`) so retry logs always reflect the effective model when available.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/anthropic_source.py" line_range="523-524" />
<code_context>
**payloads, stream=False, extra_body=extra_body
),
max_attempts=request_max_retries,
+ provider_id=str(self.provider_config.get("id", "")),
+ model=payloads.get("model", self.get_model()),
)
except httpx.RequestError as e:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid `str(..., "")` for provider_id to prevent turning `None` into the literal string `'None'`.
If `provider_config['id']` is ever `None`, this will log `provider=None`, which is different from an absent ID and may be misleading. If you only need the ID when present, pass `self.provider_config.get("id")` and let `_log_retry` handle `None`, preserving non-string IDs as well.
Suggested implementation:
```python
max_attempts=request_max_retries,
provider_id=self.provider_config.get("id"),
model=payloads.get("model", self.get_model()),
```
```python
"Anthropic",
lambda: self.client.messages.stream(**payloads, extra_body=extra_body),
max_attempts=request_max_retries,
provider_id=self.provider_config.get("id"),
model=payloads.get("model", self.get_model()),
```
</issue_to_address>
### Comment 2
<location path="tests/test_request_retry.py" line_range="32-41" />
<code_context>
assert calls == 2
+
+
+@pytest.mark.asyncio
+async def test_retry_log_includes_provider_id_and_model(monkeypatch, caplog):
+ monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MIN_S", 0)
+ monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MAX_S", 0)
+
+ async def request():
+ raise httpx.ConnectError("temporary connection failure")
+
+ with caplog.at_level(logging.WARNING, logger="astrbot"):
+ with pytest.raises(httpx.ConnectError):
+ await retry_provider_request(
+ "OpenAI",
+ request,
+ max_attempts=2,
+ provider_id="my-openai-instance",
+ model="gpt-4o",
+ )
+
+ assert "[OpenAI]" in caplog.text
+ assert "provider=my-openai-instance" in caplog.text
+ assert "model=gpt-4o" in caplog.text
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for cases where only provider_id or only model is supplied to verify log formatting edge cases
Current tests only cover when both values are present or both omitted, but `_log_retry` also handles cases where only `provider_id` or only `model` is set. Please add tests (or parametrize the existing one) for these two scenarios to verify the identity string formatting (parentheses/comma placement) remains correct and doesn’t produce stray commas or empty parentheses.
Suggested implementation:
```python
assert "[OpenAI]" in caplog.text
assert "provider=my-openai-instance" in caplog.text
assert "model=gpt-4o" in caplog.text
@pytest.mark.asyncio
async def test_retry_log_includes_only_provider_id(monkeypatch, caplog):
monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MIN_S", 0)
monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MAX_S", 0)
async def request():
raise httpx.ConnectError("temporary connection failure")
with caplog.at_level(logging.WARNING, logger="astrbot"):
with pytest.raises(httpx.ConnectError):
await retry_provider_request(
"OpenAI",
request,
max_attempts=2,
provider_id="my-openai-instance",
)
# Provider name should be present
assert "[OpenAI]" in caplog.text
# Provider ID should be present
assert "provider=my-openai-instance" in caplog.text
# Model should not be present when not provided
assert "model=" not in caplog.text
# Identity formatting should not produce empty parentheses or stray commas
assert "()" not in caplog.text
assert "(," not in caplog.text
@pytest.mark.asyncio
async def test_retry_log_includes_only_model(monkeypatch, caplog):
monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MIN_S", 0)
monkeypatch.setattr(request_retry, "REQUEST_RETRY_WAIT_MAX_S", 0)
async def request():
raise httpx.ConnectError("temporary connection failure")
with caplog.at_level(logging.WARNING, logger="astrbot"):
with pytest.raises(httpx.ConnectError):
await retry_provider_request(
"OpenAI",
request,
max_attempts=2,
model="gpt-4o",
)
# Provider name should be present
assert "[OpenAI]" in caplog.text
# Model should be present
assert "model=gpt-4o" in caplog.text
# Provider ID should not be present when not provided
assert "provider=" not in caplog.text
# Identity formatting should not produce empty parentheses or stray commas
assert "()" not in caplog.text
assert "(," not in caplog.text
```
I assumed the signature of `retry_provider_request` allows omitting `provider_id` and `model` (i.e. they are optional keyword arguments). If the actual signature differs (e.g. positional-only or different parameter names), adjust the calls in the two new tests to match the real parameters.
If `_log_retry` can emit other parenthesized sections or commas in the warning message unrelated to the provider identity, you may want to tighten the assertions to match the exact identity substring instead of checking for `"()"` or `"(,"` anywhere in `caplog.text`. That would require inspecting the actual log format and updating the tests to assert on the specific expected identity string.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
摘要 / Summary
修复 #9453:重试日志只显示静态的适配器类型标签(如
[OpenAI])。当发生 429 / 超时 / 临时网络错误并触发重试时,无法判断是哪个已配置的提供商实例、哪个模型在失败。本 PR 让重试警告日志带上提供商实例 id 与当前请求的模型名。Fixes #9453: the provider request retry warning only showed the static adapter type label (e.g.
[OpenAI]), so when a 429 / timeout / transient connection error triggered a retry, you couldn't tell which configured provider instance or which model was failing. This PR adds the provider instance id and the current request model to the retry warning log.改动 / Changes
retry_provider_request/retry_provider_request_context新增可选关键字参数provider_id/model,由_log_retry统一渲染,例如:[OpenAI] (provider=inst-1, model=gpt-4o) Request failed with retryable error; retrying (2/5): ...各聊天提供商(openai、openai responses、gemini、anthropic、ssycloud)在重试调用点传入提供商实例 id 与实际请求的模型名;
get_models等模型列表抓取只传提供商 id(无请求模型可言)。完全向后兼容:未提供
provider_id/model时日志保持原格式。retry_provider_request/retry_provider_request_contextnow accept optionalprovider_id/modelkwargs, rendered uniformly by_log_retry, e.g.:[OpenAI] (provider=inst-1, model=gpt-4o) Request failed with retryable error; retrying (2/5): ...Chat providers (openai, openai responses, gemini, anthropic, ssycloud) pass the provider instance id and the actual request model at the retry call sites;
get_modelscatalog fetches pass only the provider id (no request model exists).Fully backward compatible: without
provider_id/model, the log keeps its original format.测试 / Tests
tests/test_request_retry.py:新增基于 caplog 的用例,验证新日志含provider=.../model=...,以及缺省时保持原格式。相关提供商测试:passed
tests/test_request_retry.py: added caplog-based cases verifying the newprovider=.../model=...details and the unchanged format when not supplied.Related provider suites: passed
Summary by Sourcery
Include provider instance id and model name in provider request retry warning logs for better visibility into failing configurations and models.
Enhancements:
Tests: