Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions sieval/core/models/dialects/openai_chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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],
)


Expand Down
14 changes: 8 additions & 6 deletions sieval/core/models/dialects/openai_completions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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],
)


Expand Down Expand Up @@ -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"
Expand Down
31 changes: 26 additions & 5 deletions sieval/core/models/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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"),
Expand Down
14 changes: 10 additions & 4 deletions tests/unit/core/models/dialects/test_openai_chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
24 changes: 23 additions & 1 deletion tests/unit/core/models/dialects/test_openai_completions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand Down
48 changes: 47 additions & 1 deletion tests/unit/core/models/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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):
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/core/models/test_model_derivation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Loading