diff --git a/sieval/core/models/dialects/openai_chat.py b/sieval/core/models/dialects/openai_chat.py index 68848512..d3fd7279 100644 --- a/sieval/core/models/dialects/openai_chat.py +++ b/sieval/core/models/dialects/openai_chat.py @@ -256,9 +256,15 @@ class _LegacyPlan: def _chat_usage_stats(raw: Any) -> UsageStats | None: + """Build usage from the reply's prompt and completion counts. + + ``total_tokens`` is computed, not read: a reported total that does not + decompose is a bookkeeping quirk, and rejecting the reply would discard + tokens already generated and billed. + """ if raw is None: return None - names = ("prompt_tokens", "completion_tokens", "total_tokens") + names = ("prompt_tokens", "completion_tokens") values: list[int] = [] for name in names: value = getattr(raw, name, None) @@ -267,14 +273,10 @@ def _chat_usage_stats(raw: Any) -> UsageStats | None: f"chat usage.{name} must be a non-negative integer" ) values.append(value) - if values[2] != values[0] + values[1]: - raise OutputContractError( - "chat usage.total_tokens must equal prompt_tokens + completion_tokens" - ) return UsageStats( input_tokens=values[0], output_tokens=values[1], - total_tokens=values[2], + total_tokens=values[0] + values[1], ) diff --git a/sieval/core/models/dialects/openai_completions.py b/sieval/core/models/dialects/openai_completions.py index 857cd024..821facf2 100644 --- a/sieval/core/models/dialects/openai_completions.py +++ b/sieval/core/models/dialects/openai_completions.py @@ -280,10 +280,15 @@ def _optional_response_string(value: object, path: str) -> str | None: def _completions_usage_stats(raw: object) -> UsageStats | None: + """Build usage from the reply's prompt and completion counts. + + ``total_tokens`` is computed, not read: a reported total that does not + decompose must not cost the caller a completed reply. + """ if raw is None: return None - names = ("prompt_tokens", "completion_tokens", "total_tokens") + names = ("prompt_tokens", "completion_tokens") values: list[int] = [] for name in names: value = getattr(raw, name, None) @@ -295,7 +300,7 @@ def _completions_usage_stats(raw: object) -> UsageStats | None: return UsageStats( input_tokens=values[0], output_tokens=values[1], - total_tokens=values[2], + total_tokens=values[0] + values[1], ) @@ -722,10 +727,7 @@ def _validate_input_scoring_boundary(self, usage: UsageStats) -> None: raise OutputContractError( "usage completion-token count contradicts echoed logprob positions" ) - if usage.total_tokens != usage.input_tokens + usage.output_tokens: - raise OutputContractError( - "usage total-token count contradicts prompt and completion counts" - ) + # No total-token check: the total is computed from these two counts. if self._top_logprobs and len(self._top_logprobs) != token_count: raise OutputContractError( "input scoring top-logprob positions are inconsistent" diff --git a/sieval/core/models/model.py b/sieval/core/models/model.py index 0090b7db..0463eff5 100644 --- a/sieval/core/models/model.py +++ b/sieval/core/models/model.py @@ -183,6 +183,12 @@ class _LegacyOpenAIBinding: def _named_json_value(value: object, name: str) -> JSONValue: + """Validate and detach a JSON value, naming the offending leaf on failure. + + Sequences are ``list``/``tuple`` only: the result is persisted, and a + ``set`` would serialize in hash order while a generator would serialize + as ``[]`` once consumed. + """ if isinstance(value, float): if not math.isfinite(value): raise ValueError(f"{name} must not contain a non-finite float") @@ -196,11 +202,23 @@ def _named_json_value(value: object, name: str) -> JSONValue: raise TypeError(f"{name} keys must be strings") result[key] = _named_json_value(item, f"{name}.{key}") return result - if isinstance(value, Iterable) and not isinstance(value, str | bytes): + if isinstance(value, list | tuple): return [_named_json_value(item, name) for item in value] raise TypeError(f"{name} must be JSON-compatible, got {type(value).__name__}") +def _checked_builder_defaults(values: Mapping[str, object]) -> dict[str, object]: + """Reject builder defaults that ``meta()`` could not persist. + + ``meta()`` runs once per response, so an unpersistable default would + otherwise raise only after a call had been billed. Values are stored + unconverted -- the request builders need them as given. + """ + for key, value in values.items(): + _named_json_value(value, f"default_params.{key}") + return dict(values) + + def _optional_float(value: object, name: str) -> float | None: if value is None: return None @@ -542,7 +560,7 @@ def _initialize( self._transport = dialect # one-cycle private compatibility alias self._model = runtime_plan.requested_model_id self._api_base = api_base - self._kwargs = dict(builder_defaults) + self._kwargs = _checked_builder_defaults(builder_defaults) self._extra = dict(extra) if extra is not None else None self._limiter = local_limiter self._parent_limiter = parent_limiter @@ -592,7 +610,7 @@ def with_args( raise ValueError("concurrency_limit must be a positive integer") new_model = copy.copy(self) - new_model._kwargs = {**self._kwargs, **kwargs} + new_model._kwargs = _checked_builder_defaults({**self._kwargs, **kwargs}) if extra is not None: new_model._extra = dict(extra) if concurrency_limit is not None: @@ -936,13 +954,16 @@ def _kwargs_to_request( if stop is not None: if isinstance(stop, str): stop_value = (stop,) - elif isinstance(stop, Iterable): + elif isinstance(stop, list | tuple): values = tuple(stop) if not all(isinstance(item, str) for item in values): raise TypeError("stop must contain only strings") stop_value = cast(tuple[str, ...], values) else: - raise TypeError("stop must be a string or iterable of strings") + # ``list``/``tuple`` only, matching ``_named_json_value``: this + # value is echoed into the persisted ``request_params``, and a + # ``set`` would land there in hash order. + raise TypeError("stop must be a string, list, or tuple of strings") sampling = SamplingParams( temperature=_optional_float(temperature_value, "temperature"), top_p=_optional_float(top_p_value, "top_p"), diff --git a/tests/unit/core/models/dialects/test_openai_chat.py b/tests/unit/core/models/dialects/test_openai_chat.py index 03dcd22e..53e0fdba 100644 --- a/tests/unit/core/models/dialects/test_openai_chat.py +++ b/tests/unit/core/models/dialects/test_openai_chat.py @@ -812,7 +812,7 @@ async def test_stream_missing_choice_index_is_a_contract_error(self) -> None: [ _usage(-1, 1, 0), _usage(1, True, 2), - _usage(1, 1, 2.0), + _usage(1, 2.0, 3), ], ) async def test_usage_fields_must_be_non_negative_integers( @@ -824,11 +824,17 @@ async def test_usage_fields_must_be_non_negative_integers( await dialect.arun(Request(input=_chat())) @pytest.mark.anyio - async def test_usage_total_must_equal_input_plus_output(self) -> None: + async def test_reported_total_is_ignored_in_favour_of_the_computed_one( + self, + ) -> None: + """A total that does not decompose must not cost the caller the reply.""" dialect, _ = _dialect(_response(_choice(0, "ok"), usage=_usage(2, 3, 99))) - with pytest.raises(OutputContractError, match="must equal"): - await dialect.arun(Request(input=_chat())) + response = await dialect.arun(Request(input=_chat())) + + assert response.usage == UsageStats( + input_tokens=2, output_tokens=3, total_tokens=5 + ) @pytest.mark.anyio @pytest.mark.parametrize( diff --git a/tests/unit/core/models/dialects/test_openai_completions.py b/tests/unit/core/models/dialects/test_openai_completions.py index 72121009..775b4173 100644 --- a/tests/unit/core/models/dialects/test_openai_completions.py +++ b/tests/unit/core/models/dialects/test_openai_completions.py @@ -430,7 +430,6 @@ async def test_missing_usage_is_an_error_instead_of_a_zero_split(self) -> None: (_usage(0, 2), "positive"), (_usage(3, 0), "exceeds"), (_usage(1, 0), "completion-token"), - (_usage(1, 1, 9), "total-token"), ], ) async def test_inconsistent_usage_is_an_error( @@ -451,6 +450,29 @@ async def test_inconsistent_usage_is_an_error( with pytest.raises(OutputContractError, match=message): await dialect.execute(prepared) + @pytest.mark.anyio + async def test_reported_total_is_ignored_in_favour_of_the_computed_one( + self, + ) -> None: + """A total that does not decompose must not cost the caller the reply.""" + dialect, _ = _dialect( + _response( + _choice(tokens=["p", " out"], token_logprobs=[None, -0.2]), + usage=_usage(1, 1, 9), + ) + ) + req = Request( + input=CompletionInput("p"), + scoring=ScoringParams(input_scoring=True), + ) + _, prepared = _prepare(dialect, req) + + result = await dialect.execute(prepared) + + assert result.usage == UsageStats( + input_tokens=1, output_tokens=1, total_tokens=2 + ) + class TestOutputLifting: @pytest.mark.anyio diff --git a/tests/unit/core/models/test_model.py b/tests/unit/core/models/test_model.py index 03c35ab1..ecf2fe7e 100644 --- a/tests/unit/core/models/test_model.py +++ b/tests/unit/core/models/test_model.py @@ -564,6 +564,11 @@ def test_stop_list_becomes_tuple(self): req = self._model()._build_generate_request("p", stop=["\n\n", "Q:"]) assert req.sampling.stop == ("\n\n", "Q:") + def test_stop_tuple_becomes_tuple(self): + """Bind time stores a tuple default unconverted, so this path sees one.""" + req = self._model()._build_generate_request("p", stop=("\n\n", "Q:")) + assert req.sampling.stop == ("\n\n", "Q:") + def test_top_k_kwarg_is_sampling_top_k(self): """`top_k` is the vLLM/sglang sampling knob, not the logprobs count.""" req = self._model()._build_generate_request("p", top_k=40) @@ -644,6 +649,45 @@ class TestBuilderValidation: def _model(self): return GenModel(model="m", api_key="k") + @pytest.mark.parametrize( + "default", + [{"a", "b"}, (item for item in ["a", "b"])], + ids=["set", "generator"], + ) + def test_binding_rejects_default_params_that_cannot_round_trip(self, default): + """``default_params`` is persisted: a ``set`` reorders, a generator empties.""" + with pytest.raises( + TypeError, match="default_params.stop must be JSON-compatible" + ): + GenModel(model="m", api_key="k", stop=default) + + def test_with_args_rejects_default_params_that_cannot_round_trip(self): + model = GenModel(model="m", api_key="k") + + with pytest.raises( + TypeError, match="default_params.stop must be JSON-compatible" + ): + model.with_args(stop={"a", "b"}) + + def test_meta_keeps_tuple_default_params(self): + model = GenModel(model="m", api_key="k", stop=("a", "b")) + + assert model.meta()["default_params"]["stop"] == ["a", "b"] + + @pytest.mark.parametrize( + "stop", + [{"a", "b"}, (item for item in ["a", "b"])], + ids=["set", "generator"], + ) + def test_call_time_stop_is_refused_on_the_same_terms_as_a_default(self, stop): + """``stop`` lands in the persisted ``request_params``, so it needs an order. + + The builder pops ``stop`` before the JSON check runs, so narrowing that + check alone left this path taking any iterable. + """ + with pytest.raises(TypeError, match="stop must be a string, list, or tuple"): + self._model()._build_generate_request("p", stop=stop) + def test_n_must_be_int(self): with pytest.raises(TypeError, match="n must be an int"): self._model()._build_generate_request("p", n="3") @@ -691,7 +735,7 @@ def test_logprobs_request_forces_logprob_fields(self): ({"temperature": True}, TypeError, "temperature must be a number"), ({"top_k": True}, TypeError, "top_k must be an integer"), ({"stop": ["ok", 1]}, TypeError, "only strings"), - ({"stop": 3}, TypeError, "string or iterable"), + ({"stop": 3}, TypeError, "string, list, or tuple"), ({"return_logprobs": "yes"}, TypeError, "must be a bool"), ({"logprobs": "five"}, TypeError, "bool or integer"), ({"top_logprobs": True}, TypeError, "must be an integer"), @@ -733,6 +777,8 @@ def test_tools_none_and_json_iterables_are_normalized(self): [ ({1: "bad-key"}, "keys must be strings"), (object(), "JSON-compatible"), + ({"a", "b"}, "JSON-compatible"), + ((item for item in ["a"]), "JSON-compatible"), ], ) def test_tool_choice_must_be_json_compatible(self, tool_choice, match): diff --git a/tests/unit/core/models/test_model_derivation.py b/tests/unit/core/models/test_model_derivation.py index 6aafa282..5485104c 100644 --- a/tests/unit/core/models/test_model_derivation.py +++ b/tests/unit/core/models/test_model_derivation.py @@ -230,11 +230,10 @@ def test_model_meta_contains_default_params(self, base_gen): assert m["default_params"]["top_p"] == 0.9 @pytest.mark.parametrize("value", [float("nan"), float("inf"), float("-inf")]) - def test_model_meta_rejects_non_finite_default_params(self, value): - model = StubGenModel(model="bad-param", api_key="fake", custom=value) - + def test_binding_rejects_non_finite_default_params(self, value): + """Rejected at bind time, so no model call can be spent on it first.""" with pytest.raises(ValueError, match="non-finite"): - model.meta() + StubGenModel(model="bad-param", api_key="fake", custom=value) @pytest.mark.parametrize("value", [float("nan"), float("inf"), float("-inf")]) def test_request_builder_rejects_non_finite_sampling_values(self, base_gen, value):