ChromaCloudQwenEmbeddingFunction.build_from_config rejects task=None that get_config() emits

[winklemad]

Bug

ChromaCloudQwenEmbeddingFunction treats task as optional everywhere except build_from_config, so a collection whose Qwen embedding function was created with task=None cannot be reloaded.

• The constructor signature is task: Optional[str], and the docstring says "If None or empty, empty instructions will be used".
• The runtime (__call__ / embed_query) handles task=None (if self.task and ...).
• The JSON schema schemas/embedding_functions/chroma-cloud-qwen.json declares "task": {"type": ["string", "null"]}.
• get_config() emits {"task": None, ...} for such an EF, and validate_config() accepts it.

But build_from_config guards with if model is None or task is None: assert False, "Config is missing a required field". So reloading a persisted collection whose Qwen EF used task=None raises AssertionError, which load_collection_configuration_from_json re-wraps as ValueError("Could not build embedding function ...") — the collection becomes unusable.

Reproduction

import os
os.environ["CHROMA_API_KEY"] = "dummy"   # __init__ only needs a non-empty key; no network call
from chromadb.utils.embedding_functions.chroma_cloud_qwen_embedding_function import (
    ChromaCloudQwenEmbeddingFunction as EF, ChromaCloudQwenEmbeddingModel as M)

ef = EF(model=M.QWEN3_EMBEDDING_0p6B, task=None)   # valid per signature/docstring/schema
cfg = ef.get_config()                              # {"task": None, ...}
EF.validate_config(cfg)                            # passes (schema permits null)
EF.build_from_config(cfg)                          # AssertionError: Config is missing a required field

Fix

Only model is actually required; drop task from the guard:

-        if model is None or task is None:
+        if model is None:
             assert False, "Config is missing a required field"

PR incoming, with a real (non-mocked) get_config → build_from_config round-trip regression test (the existing parametrized round-trip test mocks build_from_config, so it never exercised this path).

[zeweihan]

This is a broken round-trip, and the break lands at the worst possible moment — which is the part worth flagging hardest, because the three methods quietly disagree about whether task=None is valid and the disagreement only surfaces on reload.

1. get_config() → build_from_config() is supposed to be identity for any config that validate_config() accepts. All three methods disagree here. get_config() emits {"task": None}. validate_config() accepts it (the JSON schema declares "task": {"type": ["string", "null"]}). The constructor accepts it (Optional[str], docstring "If None or empty, empty instructions will be used"). The runtime handles it (if self.task and ...). Only build_from_config() rejects it, with assert False. So six surfaces agree None is valid and one rejects it — and that one is the deserialize half of the persistence round-trip. The contract "if I can construct it, I can persist and reload it" is violated at the point of least visibility.

2. The failure detonates at reload, not at write — which is a durability asymmetry, not a config bug. Collection creation works, ingestion works, get_config() during persistence works, the vectors land on disk. The bomb fires the next time anyone loads that collection: possibly an hour later in a fresh process, possibly weeks later after an upgrade or an environment move, possibly in a disaster-recovery restore. At that point load_collection_configuration_from_json re-wraps the AssertionError as ValueError("Could not build embedding function ..."), which points the operator at embedding-function construction rather than at "your persisted config is no longer compatible with the loader." Same loud-but-misleading signature as the empty-Cohere-list case: an error fires, but at a layer far from the root cause, so the operator investigates model IDs and API keys instead of asking "why does the loader reject a config the writer produced." The vectors are recoverable on disk but the collection is effectively bricked because the config can't be reconstructed.

3. The assert False is independently wrong as the mechanism, separate from the None-handling. assert statements are stripped under python -O, so under an optimized interpreter this branch raises nothing and build_from_config proceeds past the guard with an unset/incomplete config — turning a loud-ish failure into a silent construction of a broken embedding function. A bare assert is the wrong tool for a config-validity invariant; it should be a typed exception (ValueError / a dedicated ConfigError) that survives optimization and carries the offending field name. The fact that the invariant is enforced only by an assert that disappears under -O is itself a latent bug for anyone running optimized Chroma in production.

4. Fix surface, in order of importance:

• build_from_config should treat task=None the same way the constructor, runtime, and validate_config already do — as "empty instructions." That single change restores the round-trip identity and makes the outlier method consistent with the other five. Since the runtime already gates on if self.task and ..., passing None through is the behavior the rest of the code already expects.
• Replace the assert False guards in build_from_config with typed exceptions regardless of the None fix — config-validity enforcement shouldn't depend on assertion flags.
• Add a property test: build_from_config(EF(...).get_config()) must succeed for every EF constructible via the public constructor, including task=None and task="". The round-trip is the invariant that actually matters here, and a property test would have caught this at the commit that introduced the Optional[str] constructor signature rather than at the first user reload.

The framing in the report is exactly right — the constructor/docstring/schema/runtime all document None as supported, so this isn't "should we allow None," it's "the loader drifted from the contract everything else implements." The minimal correct fix is making the loader agree with the five surfaces that already do.

[winklemad]

Thanks — good calls, both addressed in the latest commit (4d4f33f9):

1. assert False → ValueError. The guard now raises ValueError("Config is missing a required field: 'model'"). You're right that the assert was load-bearing the wrong way: under python -O it's stripped, so on the original code a missing model never hits the guard — it falls through to ChromaCloudQwenEmbeddingModel(None) and surfaces as None is not a valid ChromaCloudQwenEmbeddingModel, an error about the enum rather than the missing field. (The same -O run also makes the task=None bug "disappear," which is its own tell that this branch shouldn't have been an assert.) The raised exception holds in both normal and -O mode.

2. Real round-trip property test, parametrized. test_chroma_cloud_qwen_config_real_build_roundtrip now runs get_config → validate_config → build_from_config for real over task ∈ {None, "", "nl_to_code"} and asserts task/model survive. None was the reported break; "" is the other value the constructor, docstring, and schema all call valid, so it's pinned too. On the pre-fix code the None case fails with the original AssertionError while the other two pass, so the parametrization genuinely covers the regression rather than just re-asserting the fixed behavior.

One scope question before touching more: the assert False-in-build_from_config pattern isn't unique to Qwen — it's in ~21 of the embedding-function modules, some as required-field guards like this one and some as "this code should not be reached" fallthroughs. Happy to sweep them to typed exceptions in a follow-up PR so this one stays focused on the Qwen round-trip, or fold it in here if you'd prefer — your call.