feat: add DashScope embedding provider with multimodal model support#9137
Open
leafliber wants to merge 2 commits into
Open
feat: add DashScope embedding provider with multimodal model support#9137leafliber wants to merge 2 commits into
leafliber wants to merge 2 commits into
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The use of the global
dashscope.base_http_api_urlinside_callcan cause race conditions when multiple DashScope providers or requests run concurrently; consider passing the base URL via SDK options (if supported) or encapsulating calls to avoid mutating global state per request. - The logic for parsing
embedding_dimensionsand emitting the same warning is duplicated betweenget_embeddingsandget_dim; extracting this into a small helper (e.g.,_get_dimension()) would reduce repetition and keep the behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of the global `dashscope.base_http_api_url` inside `_call` can cause race conditions when multiple DashScope providers or requests run concurrently; consider passing the base URL via SDK options (if supported) or encapsulating calls to avoid mutating global state per request.
- The logic for parsing `embedding_dimensions` and emitting the same warning is duplicated between `get_embeddings` and `get_dim`; extracting this into a small helper (e.g., `_get_dimension()`) would reduce repetition and keep the behavior consistent.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/dashscope_embedding_source.py" line_range="86" />
<code_context>
+ # module-level global. Set it inside the worker thread right before the
+ # call so this request targets the configured endpoint.
+ def _call():
+ dashscope.base_http_api_url = self.base_url
+ if is_multimodal:
+ return MultiModalEmbedding.call(
</code_context>
<issue_to_address>
**issue (bug_risk):** Setting a module-level dashscope base URL per call may cause cross-request interference in concurrent usage.
Because `dashscope.base_http_api_url` is a module-level global, concurrent calls using different `base_url` values can race and overwrite each other, causing requests to be sent to the wrong endpoint under load.
Prefer a per-client or per-request base URL if the SDK supports it. Otherwise, at minimum document that only one base URL is effectively supported, or add a guard to detect and prevent conflicting configurations.
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/dashscope_embedding_source.py" line_range="57" />
<code_context>
+ embeddings = await self.get_embeddings([text])
+ return embeddings[0] if embeddings else []
+
+ async def get_embeddings(self, text: list[str]) -> list[list[float]]:
+ """Get the embedding vectors for a batch of texts via the dashscope SDK.
+
</code_context>
<issue_to_address>
**suggestion:** Consider returning early for an empty input list to avoid an unnecessary SDK call.
If `text` is empty, we still call the dashscope SDK, which may error if it requires at least one input and is unnecessary work even when it succeeds. Adding an early return like:
```python
if not text:
return []
```
would avoid this call and make the behavior for empty input explicit.
Suggested implementation:
```python
async def get_embeddings(self, text: list[str]) -> list[list[float]]:
if not text:
return []
```
This edit assumes the existing implementation of `get_embeddings` (the SDK call and processing of the response) remains unchanged and directly follows the function signature. No further changes are required as long as the rest of the function continues to return `list[list[float]]` for non-empty inputs.
</issue_to_address>
### Comment 3
<location path="tests/test_dashscope_embedding_source.py" line_range="81" />
<code_context>
+
+@pytest.mark.asyncio
+async def test_text_model_routes_to_text_embedding(monkeypatch):
+ provider = _make_provider({"embedding_dimensions": 1024})
+ captured: dict = {}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider tests around invalid or non-positive `embedding_dimensions` for `get_embeddings`
`test_text_model_routes_to_text_embedding` only covers the case where a valid positive `embedding_dimensions` is passed through as `dimension`. `get_embeddings`, however, is designed to ignore invalid or non-positive values and log a warning. Please add tests that exercise this behaviour end-to-end, e.g.:
- A provider with `{"embedding_dimensions": 0}` where the fake `TextEmbedding.call` / `MultiModalEmbedding.call` is invoked without a `dimension` key in `kwargs`.
- Optionally, a provider with `{"embedding_dimensions": "abc"}` to confirm that non-int values are also omitted from the outbound request.
This will ensure the invalid-dimension handling in `get_embeddings` is covered, not just the `get_dim()` branch.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_text_model_routes_to_text_embedding(monkeypatch):
@pytest.mark.asyncio
async def test_text_model_ignores_zero_embedding_dimension(monkeypatch):
provider = _make_provider({"embedding_dimensions": 0})
captured: dict = {}
async def fake_call(*args, **kwargs):
captured["kwargs"] = kwargs
return _FakeResponse()
# Patch the underlying dashscope TextEmbedding used by the provider
monkeypatch.setattr(dashscope.TextEmbedding, "call", fake_call)
await provider.get_embeddings("hello world")
# get_embeddings should omit invalid/non-positive dimensions
assert "dimension" not in captured["kwargs"]
@pytest.mark.asyncio
async def test_text_model_ignores_non_int_embedding_dimension(monkeypatch):
provider = _make_provider({"embedding_dimensions": "abc"})
captured: dict = {}
async def fake_call(*args, **kwargs):
captured["kwargs"] = kwargs
return _FakeResponse()
monkeypatch.setattr(dashscope.TextEmbedding, "call", fake_call)
await provider.get_embeddings("hello world")
# get_embeddings should omit non-int dimensions
assert "dimension" not in captured["kwargs"]
```
1. Ensure `_make_provider` constructs a provider that uses `dashscope.TextEmbedding.call` for text models; if it uses a different class/path (e.g. `dashscope.TextEmbedding.text_embedding` or a wrapper), update the `monkeypatch.setattr` targets in both new tests accordingly.
2. If `get_embeddings` is synchronous in your implementation, drop `pytest.mark.asyncio`, remove `async` from the tests, and adjust `fake_call` and the `await` calls to be synchronous.
3. If you want to verify the warning log as well, add a `caplog` fixture to these tests and assert that the expected warning about invalid `embedding_dimensions` is emitted when calling `provider.get_embeddings`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_text_model_routes_to_text_embedding(monkeypatch): | ||
| provider = _make_provider({"embedding_dimensions": 1024}) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider tests around invalid or non-positive embedding_dimensions for get_embeddings
test_text_model_routes_to_text_embedding only covers the case where a valid positive embedding_dimensions is passed through as dimension. get_embeddings, however, is designed to ignore invalid or non-positive values and log a warning. Please add tests that exercise this behaviour end-to-end, e.g.:
- A provider with
{"embedding_dimensions": 0}where the fakeTextEmbedding.call/MultiModalEmbedding.callis invoked without adimensionkey inkwargs. - Optionally, a provider with
{"embedding_dimensions": "abc"}to confirm that non-int values are also omitted from the outbound request.
This will ensure the invalid-dimension handling in get_embeddings is covered, not just the get_dim() branch.
Suggested implementation:
@pytest.mark.asyncio
async def test_text_model_routes_to_text_embedding(monkeypatch):
@pytest.mark.asyncio
async def test_text_model_ignores_zero_embedding_dimension(monkeypatch):
provider = _make_provider({"embedding_dimensions": 0})
captured: dict = {}
async def fake_call(*args, **kwargs):
captured["kwargs"] = kwargs
return _FakeResponse()
# Patch the underlying dashscope TextEmbedding used by the provider
monkeypatch.setattr(dashscope.TextEmbedding, "call", fake_call)
await provider.get_embeddings("hello world")
# get_embeddings should omit invalid/non-positive dimensions
assert "dimension" not in captured["kwargs"]
@pytest.mark.asyncio
async def test_text_model_ignores_non_int_embedding_dimension(monkeypatch):
provider = _make_provider({"embedding_dimensions": "abc"})
captured: dict = {}
async def fake_call(*args, **kwargs):
captured["kwargs"] = kwargs
return _FakeResponse()
monkeypatch.setattr(dashscope.TextEmbedding, "call", fake_call)
await provider.get_embeddings("hello world")
# get_embeddings should omit non-int dimensions
assert "dimension" not in captured["kwargs"]- Ensure
_make_providerconstructs a provider that usesdashscope.TextEmbedding.callfor text models; if it uses a different class/path (e.g.dashscope.TextEmbedding.text_embeddingor a wrapper), update themonkeypatch.setattrtargets in both new tests accordingly. - If
get_embeddingsis synchronous in your implementation, droppytest.mark.asyncio, removeasyncfrom the tests, and adjustfake_calland theawaitcalls to be synchronous. - If you want to verify the warning log as well, add a
caplogfixture to these tests and assert that the expected warning about invalidembedding_dimensionsis emitted when callingprovider.get_embeddings.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new embedding provider adapter for Aliyun DashScope (DashScopeEmbeddingProvider), supporting both text and multimodal embedding models via the native SDK. It also adds default configurations, dynamic provider loading, localization files, and comprehensive unit tests. The reviewer feedback highlights a critical concurrency issue where concurrent calls to asyncio.to_thread could cause race conditions on the global dashscope.base_http_api_url variable, suggesting the use of a threading lock to synchronize access. Additionally, the reviewer recommends refactoring the parsing of embedding_dimensions into the provider's initialization to avoid redundant logic and duplicate warning logs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1
to
+3
| import asyncio | ||
| import os | ||
| from http import HTTPStatus |
Contributor
There was a problem hiding this comment.
为了支持多线程并发请求时的线程同步，我们需要导入 threading 模块。
Suggested change
| import asyncio | |
| import os | |
| from http import HTTPStatus | |
| import asyncio | |
| import os | |
| import threading | |
| from http import HTTPStatus |
Comment on lines
+14
to
+16
| _DEFAULT_API_BASE = "https://dashscope.aliyuncs.com/api/v1" | ||
| _DEFAULT_MODEL = "text-embedding-v4" | ||
|
|
Contributor
There was a problem hiding this comment.
定义一个模块级别的锁 _lock，用于保护对全局变量 dashscope.base_http_api_url 的并发修改。
Suggested change
| _DEFAULT_API_BASE = "https://dashscope.aliyuncs.com/api/v1" | |
| _DEFAULT_MODEL = "text-embedding-v4" | |
| _DEFAULT_API_BASE = "https://dashscope.aliyuncs.com/api/v1" | |
| _DEFAULT_MODEL = "text-embedding-v4" | |
| _lock = threading.Lock() |
Comment on lines
+85
to
+99
| def _call(): | ||
| dashscope.base_http_api_url = self.base_url | ||
| if is_multimodal: | ||
| return MultiModalEmbedding.call( | ||
| model=self.model, | ||
| input=[{"text": t} for t in text], | ||
| api_key=self.api_key, | ||
| **kwargs, | ||
| ) | ||
| return TextEmbedding.call( | ||
| model=self.model, | ||
| input=text, | ||
| api_key=self.api_key, | ||
| **kwargs, | ||
| ) |
Contributor
There was a problem hiding this comment.
由于 dashscope.base_http_api_url 是一个全局模块级变量，而 _call 是通过 asyncio.to_thread 在单独的线程中并发执行的，因此并发请求可能会相互覆盖该全局变量，导致请求被发送到错误的 Endpoint。建议使用 _lock 锁来确保设置全局变量和执行 API 调用这两个步骤的原子性。
Suggested change
| def _call(): | |
| dashscope.base_http_api_url = self.base_url | |
| if is_multimodal: | |
| return MultiModalEmbedding.call( | |
| model=self.model, | |
| input=[{"text": t} for t in text], | |
| api_key=self.api_key, | |
| **kwargs, | |
| ) | |
| return TextEmbedding.call( | |
| model=self.model, | |
| input=text, | |
| api_key=self.api_key, | |
| **kwargs, | |
| ) | |
| def _call(): | |
| with _lock: | |
| dashscope.base_http_api_url = self.base_url | |
| if is_multimodal: | |
| return MultiModalEmbedding.call( | |
| model=self.model, | |
| input=[{"text": t} for t in text], | |
| api_key=self.api_key, | |
| **kwargs, | |
| ) | |
| return TextEmbedding.call( | |
| model=self.model, | |
| input=text, | |
| api_key=self.api_key, | |
| **kwargs, | |
| ) |
References
- In a single-threaded asyncio event loop, synchronous functions are executed atomically and safe from race conditions. However, since
_callis executed in a separate thread viaasyncio.to_thread, it is not safe from race conditions when modifying shared global state likedashscope.base_http_api_url.
Comment on lines
+32
to
+50
| def __init__(self, provider_config: dict, provider_settings: dict) -> None: | ||
| super().__init__(provider_config, provider_settings) | ||
| self.provider_config = provider_config | ||
|
|
||
| self.api_key = provider_config.get("embedding_api_key") or os.getenv( | ||
| "DASHSCOPE_API_KEY", "" | ||
| ) | ||
| if not self.api_key: | ||
| raise ValueError("阿里云百炼(DashScope) Embedding API Key 不能为空。") | ||
|
|
||
| self.base_url = provider_config.get("embedding_api_base", _DEFAULT_API_BASE) | ||
| self.model = provider_config.get("embedding_model", _DEFAULT_MODEL) | ||
|
|
||
| provider_id = provider_config.get("id", "unknown_id") | ||
| logger.info( | ||
| f"[DashScope Embedding] {provider_id} Initialized via native SDK, " | ||
| f"base_url={self.base_url}, model={self.model}" | ||
| ) | ||
| self.set_model(self.model) |
Contributor
There was a problem hiding this comment.
为了避免在 get_embeddings 和 get_dim 中重复解析 embedding_dimensions 并打印警告日志，建议在 __init__ 初始化时解析一次并将其存储在 self.dimensions 中。
def __init__(self, provider_config: dict, provider_settings: dict) -> None:
super().__init__(provider_config, provider_settings)
self.provider_config = provider_config
self.api_key = provider_config.get("embedding_api_key") or os.getenv(
"DASHSCOPE_API_KEY", ""
)
if not self.api_key:
raise ValueError("阿里云百炼(DashScope) Embedding API Key 不能为空。")
self.base_url = provider_config.get("embedding_api_base", _DEFAULT_API_BASE)
self.model = provider_config.get("embedding_model", _DEFAULT_MODEL)
self.dimensions = None
if "embedding_dimensions" in provider_config:
try:
dimensions = int(provider_config["embedding_dimensions"])
if dimensions > 0:
self.dimensions = dimensions
except (ValueError, TypeError):
logger.warning(
f"embedding_dimensions in embedding configs is not a valid integer: "
f"'{provider_config['embedding_dimensions']}', ignored."
)
provider_id = provider_config.get("id", "unknown_id")
logger.info(
f"[DashScope Embedding] {provider_id} Initialized via native SDK, "
f"base_url={self.base_url}, model={self.model}"
)
self.set_model(self.model)
Comment on lines
+70
to
+80
| kwargs: dict = {} | ||
| if "embedding_dimensions" in self.provider_config: | ||
| try: | ||
| dimensions = int(self.provider_config["embedding_dimensions"]) | ||
| if dimensions > 0: | ||
| kwargs["dimension"] = dimensions | ||
| except (ValueError, TypeError): | ||
| logger.warning( | ||
| f"embedding_dimensions in embedding configs is not a valid integer: " | ||
| f"'{self.provider_config['embedding_dimensions']}', ignored." | ||
| ) |
Contributor
There was a problem hiding this comment.
使用在 __init__ 中已经解析好的 self.dimensions，避免重复解析和日志打印。
kwargs: dict = {}
if self.dimensions is not None:
kwargs["dimension"] = self.dimensions
Comment on lines
+128
to
+138
| def get_dim(self) -> int: | ||
| """Get the configured embedding dimension.""" | ||
| if "embedding_dimensions" in self.provider_config: | ||
| try: | ||
| return int(self.provider_config["embedding_dimensions"]) | ||
| except (ValueError, TypeError): | ||
| logger.warning( | ||
| f"embedding_dimensions in embedding configs is not a valid integer: " | ||
| f"'{self.provider_config['embedding_dimensions']}', ignored." | ||
| ) | ||
| return 0 |
Contributor
There was a problem hiding this comment.
直接返回在 __init__ 中已经解析好的 self.dimensions，避免重复解析和日志打印。
def get_dim(self) -> int:
"""Get the configured embedding dimension."""
return self.dimensions or 0
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
1 participant
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.
Modifications / 改动点
新增阿里云百炼 (DashScope) Embedding 提供商适配器，通过 dashscope SDK 原生协议调用，支持文本和多模态两类模型
Fixes: #9127
Screenshots or Test Results / 运行截图或测试结果
============================= test session starts ==============================
collected 15 items
tests/test_dashscope_embedding_source.py::test_requires_api_key PASSED [ 6%]
tests/test_dashscope_embedding_source.py::test_env_var_fallback PASSED [ 13%]
tests/test_dashscope_embedding_source.py::test_api_key_takes_precedence_over_env PASSED [ 20%]
tests/test_dashscope_embedding_source.py::test_defaults PASSED [ 26%]
tests/test_dashscope_embedding_source.py::test_user_values_preserved PASSED [ 33%]
tests/test_dashscope_embedding_source.py::test_text_model_routes_to_text_embedding PASSED [ 40%]
tests/test_dashscope_embedding_source.py::test_multimodal_model_routes_to_multimodal_embedding PASSED [ 46%]
tests/test_dashscope_embedding_source.py::test_tongyi_vision_model_routes_to_multimodal PASSED [ 53%]
tests/test_dashscope_embedding_source.py::test_get_embedding_single PASSED [ 60%]
tests/test_dashscope_embedding_source.py::test_error_surfaces_status_code_and_request_id PASSED [ 66%]
tests/test_dashscope_embedding_source.py::test_multimodal_error_url_uses_multimodal_path PASSED [ 73%]
tests/test_dashscope_embedding_source.py::test_no_embeddings_raises PASSED [ 80%]
tests/test_dashscope_embedding_source.py::test_get_dim_returns_configured PASSED [ 86%]
tests/test_dashscope_embedding_source.py::test_get_dim_returns_zero_when_not_set PASSED [ 93%]
tests/test_dashscope_embedding_source.py::test_get_dim_returns_zero_when_invalid PASSED [100%]
======================== 15 passed, 1 warning in 1.56s =========================
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add a DashScope (Aliyun Bailian) embedding provider using the native DashScope SDK with support for both text and multimodal embedding models.
New Features:
Tests: