Skip to content

feat(asr): add model vendor metadata across providers - #2265

Merged
wangyoucao577 merged 1 commit into
mainfrom
push-vnokyxswxlxm
Jul 31, 2026
Merged

feat(asr): add model vendor metadata across providers#2265
wangyoucao577 merged 1 commit into
mainfrom
push-vnokyxswxlxm

Conversation

@anexpn

@anexpn anexpn commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Expose configured model names for Aliyun, ByteDance, Google, OpenAI, and Sarvam ASR extensions, with tests and patch version bumps.

@github-actions

Copy link
Copy Markdown

Review: feat(asr): add model vendor metadata across providers

Consistent, well-scoped change — one vendor_metadata() override per extension, version bumps in both manifest.json and pyproject.toml for all five, and the bytedance manifest gets a missing-EOF-newline fix along the way. Reporting the model name is also a safe addition to the metadata payload, unlike the key/api_key fields some existing implementations already emit.

That said, I think there's a real bug shared by four of the five providers.

1. Reading raw params misses defaults, so metadata is empty exactly when the default model is in use

google_asr_python and sarvam_asr_python both declare a top-level model field on their config with a default, and on_init() calls self.config.update(self.config.params) to fold params into those fields:

  • GoogleASRConfig.model defaults to "long"; consumed by get_recognition_config() as self.model.
  • SarvamASRConfig.model defaults to "saarika:v2.5"; consumed by _build_websocket_url() as self.config.model (and _send_initial_config() branches on self.config.model.startswith("saaras")).

The new overrides read self.config.params.get("model") instead. When an operator does not explicitly set model in params, the resolved model is the default and is genuinely sent to the vendor — but vendor_metadata() returns {}. The metadata goes silent in precisely the case where reporting matters most, and the value it reports is not the one guaranteed to be in effect.

Since vendor_metadata() is only called after on_init() (it feeds connection_status_changed), the resolved field is available and is the correct source:

@override
def vendor_metadata(self) -> dict[str, Any]:
    if self.config is None:
        return {}
    return {"model": self.config.model} if self.config.model else {}

bytedance_llm_based_asr has the same shape via a different route. get_request_config() applies "model_name": "bigmodel" as a default, but the override reads self.config.params.get("request", {}) directly and so bypasses it. Worth routing through the accessor — note it raises ValueError when request is absent, so it needs guarding:

try:
    model = self.config.get_request_config().get("model_name")
except ValueError:
    model = None

aliyun_asr is the one I could not confirm. AliyunASRConfig has no model field, and I could not find any code path that consumes a "model" key out of paramsstart_connection() goes through nls.NlsSpeechTranscriber. If Aliyun's SDK does not take a model parameter here, params.get("model") is unreachable in practice and the override is dead code; if it does, a pointer to where would help. The test passes "paraformer-realtime-v2", which suggests intent, but the test only exercises the getter, not that the value reaches the vendor.

2. Test coverage does not reach the failing case

Each new test file covers two paths: explicit model set, and config is None. The missing third case — config present, model not explicitly set — is the one that regresses:

def test_vendor_metadata_uses_default_model():
    ext = GoogleASRExtension("test")
    ext.config = GoogleASRConfig.model_validate({"params": {}})
    ext.config.update(ext.config.params)

    assert ext.vendor_metadata() == {"model": "long"}

That test fails against the current implementation and passes against the suggested fix, which is a good signal it's the right assertion to add. I'd add the equivalent for Sarvam and ByteDance.

Two smaller gaps: none of the tests call config.update(config.params), so they don't reflect the post-on_init() state the method actually runs in; and the ByteDance test change only extends the existing explicit-model assertion rather than adding a defaults case.

3. openai_asr_python: avoid model_dump() for a single lookup

transcription = self.config.params.model_dump().get("input_audio_transcription", {})

Params sets extra="allow", so input_audio_transcription lands in model_extra and can be read without serializing the whole object:

transcription = (self.config.params.model_extra or {}).get("input_audio_transcription")

This deep-copies every param — including the plaintext api_key — into a throwaway dict on each call. Nothing leaks (the value is discarded, and encrypting_serializer is when_used="json" so model_dump() wouldn't have masked it anyway), but it's avoidable work in a method invoked on every connection-status transition, and it keeps the secret out of an intermediate structure.

4. Minor: five copies of the same shape

vendor_metadata() is now implemented in 7+ extensions with substantially identical if self.config is None: return {} / filter-empty-values logic. A small helper on AsyncASRBaseExtension — something like _model_metadata(model) returning {"model": model} if model else {} — would keep the next provider consistent by default. Not blocking; worth considering while the pattern is still spreading.

Summary

The structure and versioning are right, and the ByteDance test update shows the mechanism works. The blocking item is the params-vs-resolved-field mismatch in Google, Sarvam, and ByteDance, plus a decision on whether Aliyun's lookup is reachable at all. Adding the default-model test case for each provider should keep this from recurring.

@anexpn
anexpn force-pushed the push-vnokyxswxlxm branch from 6b95a9a to d625d6f Compare July 30, 2026 07:47
diyuyi-agora
diyuyi-agora previously approved these changes Jul 30, 2026
@github-actions

Copy link
Copy Markdown

Review: feat(asr): add model vendor metadata across providers

Consistent, well-scoped change: five providers gain a model key in vendor_metadata(), each with a paired test and a patch version bump in both manifest.json and pyproject.toml. The if self.config is None: return {} guard and the truthiness filter match the existing convention in deepgram_asr_python, soniox_asr_python, tencent_asr_python, and azure_asr_python, so the new code reads like the code already there. Findings are ordered by how much they affect correctness.

1. Google and Sarvam read params, but the model lives on a top-level field

Both extensions load config and then immediately flatten params onto the model:

self.config = GoogleASRConfig.model_validate_json(config_json)
self.config.update(self.config.params)   # copies params["model"] -> self.config.model

GoogleASRConfig.model defaults to "long" and SarvamASRConfig.model to "saarika:v2.5", and both are what the request path actually uses:

  • google_asr_python/config.py:129 uses "model": self.model in get_recognition_config()
  • sarvam_asr_python/extension.py:120 uses "model": self.config.model in _build_websocket_url()

The new vendor_metadata() reads self.config.params.get("model") instead. That works only when the operator happens to spell model inside params. Both shipped property.json files do, which is why the tests pass, but the tests construct config via model_validate and never call .update(), so they do not exercise the real path. A deployment that sets model at the top level (valid for both configs, and the field the client honors) reports no model at all, and the effective default (long / saarika:v2.5) is never reported when params omits the key.

Reading the resolved field is both simpler and accurate:

def vendor_metadata(self) -> dict[str, Any]:
    if self.config is None:
        return {}
    return {"model": self.config.model} if self.config.model else {}

This mirrors tencent_asr_python, which reads self.request_params.engine_model_type, the resolved value, not the raw params dict. Worth adding a test that calls config.update(config.params) first, and one where model is set top-level only, to pin the behavior.

2. Aliyun: params is never populated, and the config class is malformed

vendor_metadata() reads self.config.params.get("model"), but in aliyun_asr:

  • property.json has no params key at all, only appkey, akid, aksecret, api_url
  • params and black_list_params are declared but never read anywhere in the extension. start_connection() calls self.client.start(...) with hardcoded aformat and enable_* arguments and never passes a model.
  • NlsSpeechTranscriber takes no model parameter; the model is implied by the appkey

So this returns {} in every real configuration, and would report a model that has no effect on the request even if someone set one. The new test passes only because it injects params.model by hand. Consider dropping the Aliyun override, or, if the goal is a uniform telemetry shape, wiring params["model"] into the actual start_connection() call first so the reported value is real.

Separately, and pre-existing but directly adjacent: AliyunASRConfig is declared as both @DataClass and BaseModel, with field(default_factory=dict) instead of the Pydantic Field. That stacking is unsupported and makes the default value of params unreliable, which is exactly the attribute this PR now calls .get() on. If params resolves to a dataclasses.Field object rather than a dict, .get() raises AttributeError inside error-reporting code. Worth converting to a plain BaseModel with Field(default_factory=dict) while you are here, since the new code depends on it.

3. OpenAI: model_dump() on every call is avoidable, and couples to the encryption serializer

transcription = self.config.params.model_dump().get("input_audio_transcription", {})

Params is a Pydantic model with extra="allow", so input_audio_transcription is reachable directly. Two concerns:

  • model_dump() serializes the entire params object, including api_key, to pull one nested string. vendor_metadata() is called on connection events and error paths, so this is needless work that also materializes secrets in memory.
  • Params registers _encrypt_serializer = encrypting_serializer("api_key", "organization", "project"). Field serializers run during model_dump(). Today that only touches those three names, so model is unaffected, but it ties model-name extraction to the encryption config, and a future addition to that list could silently start returning an encrypted string.

Direct attribute access avoids both:

transcription = getattr(self.config.params, "input_audio_transcription", None) or {}
model = transcription.get("model") if isinstance(transcription, dict) else None

input_audio_transcription arrives from property.json as a plain dict (see openai_asr_client/client.py:356, which passes a dict literal), so the isinstance check already present is the right instinct: keep it.

4. ByteDance: reads raw params["request"], bypassing the default

The extension uses self.config.params.get("request", {}) then request.get("model_name"). But config.py:74 establishes model_name: "bigmodel" as a default inside get_request_config(), which is what volcengine_asr_client.py:176 and :603 actually send. When an operator supplies a request block without model_name, the request goes out as bigmodel while metadata reports nothing.

The sibling fields in the same dict already use accessors (get_api_url(), get_app_key(), and so on). Following that pattern keeps the reported value equal to the sent value:

try:
    request = self.config.get_request_config()
except ValueError:
    request = {}
fields = {..., "model": request.get("model_name")}

The try matters: get_request_config() raises ValueError when params.request is absent, and vendor_metadata() is called from error paths where raising would mask the original error.

Also note config.py:162 mutates config.params["request"] during to_json(). Not triggered by this PR, but it means the content of params["request"] can differ depending on whether config logging ran first, another reason to go through the accessor.

5. Test collection is worth verifying locally

The five test_vendor_metadata.py files use relative imports (from ..extension import ...) and are collected by tests/bin/start, which runs pytest -s tests/. Two things to check:

  • aliyun_asr/tests/bin/start sets PYTHONPATH without a leading dot entry, unlike bytedance_llm_based_asr/tests/bin/start which begins with .:. Since the new Aliyun test relies on a relative import resolving the parent package, please confirm it is actually collected rather than erroring on import. The ByteDance test predates this PR and its start script has the dot entry, consistent with it working there.
  • from ..extension import AliyunASRExtension executes import nls at module load. If the Aliyun SDK is absent in the unit-test environment, that is a collection error rather than a skip. The other four import lighter modules, and Google, Sarvam, and OpenAI correctly import config from ..config where it lives.

Both are environment questions I could not verify here. A local tests/bin/start run per extension would confirm all ten new tests are collected and green.

Minor

  • AliyunASRExtension.vendor() is not decorated with @OverRide while the new vendor_metadata() is: a small inconsistency within one class.
  • Test coverage is limited to the happy path plus the config is None case. The more valuable cases for this feature are the ones above: model absent from params, model set top-level only, and post-update() state. Those are where the current implementations diverge from what gets sent.
  • Fixing the missing trailing newline in bytedance_llm_based_asr/manifest.json is a nice touch.
  • Commit message and branch naming follow the conventional-commits rules in AGENTS.md.

Security

No new secret exposure. Each override returns only a model name, and the truthiness filter keeps empty values out. The OpenAI model_dump() point above is the only place worth tightening, and it is about avoiding unnecessary handling of api_key rather than an actual leak.

Summary

The pattern and structure are right. The core issue is that four of the five implementations read the raw params dict while the request path reads a resolved value, either a default or a flattened field, so metadata can disagree with what was actually sent, and for Aliyun it likely reports nothing in any real config. Reading the same value the client sends fixes all four and simplifies the code. The tests pass because they construct config in a way that skips the resolution step, so they would not catch this today.

Expose configured model names for Aliyun, ByteDance, Google, OpenAI, 
and Sarvam ASR extensions, with tests and patch version bumps.
@wangyoucao577
wangyoucao577 merged commit 1868957 into main Jul 31, 2026
34 checks passed
@wangyoucao577
wangyoucao577 deleted the push-vnokyxswxlxm branch July 31, 2026 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants