diff --git a/packages/client/api-report/launchdarkly_ai_server.experimental.evaluations.txt b/packages/client/api-report/launchdarkly_ai_server.experimental.evaluations.txt index abc72168..3ecd56ce 100644 --- a/packages/client/api-report/launchdarkly_ai_server.experimental.evaluations.txt +++ b/packages/client/api-report/launchdarkly_ai_server.experimental.evaluations.txt @@ -19,6 +19,7 @@ EvalRunResult [class, @dataclass] url: str EvalTool [class, @dataclass(frozen=True)] __init__(key: str, implementation: ToolImplementation, schema: dict[str, Any], description: str = '') + __post_init__() -> None description: str = '' implementation: ToolImplementation key: str diff --git a/packages/client/src/launchdarkly_ai_server/evaluations/api.py b/packages/client/src/launchdarkly_ai_server/evaluations/api.py index 6f87dd39..c1e7f5fc 100644 --- a/packages/client/src/launchdarkly_ai_server/evaluations/api.py +++ b/packages/client/src/launchdarkly_ai_server/evaluations/api.py @@ -213,6 +213,20 @@ def require_mapping(value: Any, *, description: str) -> Mapping[str, Any]: return value +def check_string_keys(value: Any) -> None: + """Raise ``TypeError`` on a mapping key that is not a ``str``, recursively.""" + if isinstance(value, Mapping): + for key, item in value.items(): + if not isinstance(key, str): + raise TypeError( + f"keys must be str, not {type(key).__name__} (got {key!r})" + ) + check_string_keys(item) + elif isinstance(value, (list, tuple)): + for item in value: + check_string_keys(item) + + def require_string(data: Mapping[str, Any], key: str, description: str) -> str: """Return the non-empty string at ``key``. Raises ``EvaluationsError``.""" value = data.get(key) diff --git a/packages/client/src/launchdarkly_ai_server/evaluations/module.py b/packages/client/src/launchdarkly_ai_server/evaluations/module.py index ec6e88a5..bd199ab8 100644 --- a/packages/client/src/launchdarkly_ai_server/evaluations/module.py +++ b/packages/client/src/launchdarkly_ai_server/evaluations/module.py @@ -17,6 +17,7 @@ EvaluationsError, LDApiClient, Transport, + check_string_keys, segment, urllib_transport, ) @@ -62,17 +63,6 @@ def _render_inline_row(row: DatasetRow) -> DatasetRow: ) -def _check_string_keys(value: Any) -> None: - if isinstance(value, Mapping): - for key, item in value.items(): - if not isinstance(key, str): - raise TypeError(f"keys must be str, not {type(key).__name__}") - _check_string_keys(item) - elif isinstance(value, (list, tuple)): - for item in value: - _check_string_keys(item) - - def _normalize_inline_rows(rows: Sequence[InlineDatasetRow]) -> list[DatasetRow]: """Validate caller-supplied rows, returning them raw and indexed by position. @@ -126,7 +116,7 @@ def _normalize_inline_rows(rows: Sequence[InlineDatasetRow]) -> list[DatasetRow] # references fail there instead of recursing in the key check. try: json.dumps(value, allow_nan=False) - _check_string_keys(value) + check_string_keys(value) except (TypeError, ValueError) as error: raise EvaluationsError( f"Inline dataset row {position} {field_name} must be " diff --git a/packages/client/src/launchdarkly_ai_server/evaluations/tools.py b/packages/client/src/launchdarkly_ai_server/evaluations/tools.py index 94845924..5083169e 100644 --- a/packages/client/src/launchdarkly_ai_server/evaluations/tools.py +++ b/packages/client/src/launchdarkly_ai_server/evaluations/tools.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import copy import json from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass, field @@ -13,6 +14,7 @@ EvaluationsError, LDApiClient, LDApiError, + check_string_keys, require_mapping, segment, ) @@ -38,6 +40,9 @@ class EvalTool: a library tool, because the provider supplies its schema. ``dataclasses.replace`` on a library tool returns an inline tool that keeps the library schema. + + The tool holds a deep copy of ``schema``, so changing the caller's dict + afterwards changes neither what is validated nor what is sent. """ key: str @@ -48,6 +53,16 @@ class EvalTool: version: int | None = field(default=None, init=False) project_key: str | None = field(default=None, init=False) + def __post_init__(self) -> None: + # A schema that is not a mapping, or cannot be copied, is left as it + # is for validate_tools to reject with the tool key. + if isinstance(self.schema, Mapping): + try: + schema = copy.deepcopy(dict(self.schema)) + except (TypeError, copy.Error): + return + object.__setattr__(self, "schema", schema) + @classmethod def _library( cls, @@ -76,7 +91,9 @@ def to_create_wire(self) -> dict[str, Any]: if self.source == "inline": return { "key": self.key, - "schema": self.schema, + # A copy, so a handler that changes its config cannot change + # the body. + "schema": copy.deepcopy(self.schema), "description": self.description, "source": "inline", } @@ -120,7 +137,11 @@ def _validate_inline_tool(tool: EvalTool) -> None: ) try: # allow_nan=False rejects NaN and Infinity, which are not valid JSON. + # Non-str keys are rejected rather than coerced; json.dumps runs first + # so circular references fail there instead of recursing in the key + # check. json.dumps(dict(tool.schema), allow_nan=False) + check_string_keys(tool.schema) except (TypeError, ValueError) as error: raise EvaluationsError( f"Inline tool {key!r} schema must be JSON-serializable: {error}" @@ -133,7 +154,7 @@ def validate_tools(tools: Sequence[EvalTool], project_key: str) -> None: Checks that each library tool has a project and that it matches ``project_key``. Issues no requests. """ - keys_by_identity: dict[str, str] = {} + tools_by_identity: dict[str, EvalTool] = {} for tool in tools: if not isinstance(tool, EvalTool): raise EvaluationsError( @@ -161,17 +182,18 @@ def validate_tools(tools: Sequence[EvalTool], project_key: str) -> None: f"cannot run in project {project_key!r}. Read it from " f"{project_key!r} instead." ) - # One key names one tool. Keys are compared case-insensitively. - identity = key.strip().lower() - collision = keys_by_identity.get(identity) + # One key names one tool. Never pick one entry silently: the run record + # would then misdescribe where the model's tool came from. casefold, + # unlike lower, also matches keys such as "straße" and "strasse". + identity = key.strip().casefold() + collision = tools_by_identity.get(identity) if collision is not None: - if collision == key: - raise EvaluationsError(f"Tool {key!r} appears more than once in tools.") raise EvaluationsError( - f"Tool {key!r} collides with {collision!r}. Two tools in one " - "run must not have keys that differ only by case." + f"Tool {key!r} ({tool.source}) collides with {collision.key!r} " + f"({collision.source}). A key may appear once in tools, compared " + "without case." ) - keys_by_identity[identity] = key + tools_by_identity[identity] = tool def tool_handlers(tools: Sequence[EvalTool]) -> dict[str, ToolImplementation]: @@ -182,7 +204,11 @@ def tool_handlers(tools: Sequence[EvalTool]) -> dict[str, ToolImplementation]: def handler_config_tools(tools: Sequence[EvalTool]) -> dict[str, dict[str, Any]]: """Return the ``tools`` entry of a handler config.""" return { - tool.key: {"description": tool.description, "parameters": tool.schema} + # A copy, so a handler that changes its config cannot change the body. + tool.key: { + "description": tool.description, + "parameters": copy.deepcopy(tool.schema), + } for tool in tools } diff --git a/packages/client/tests/test_evaluations_run.py b/packages/client/tests/test_evaluations_run.py index 1028a9c8..87390977 100644 --- a/packages/client/tests/test_evaluations_run.py +++ b/packages/client/tests/test_evaluations_run.py @@ -14,6 +14,10 @@ from launchdarkly_ai_server import NativeTool, create_handler from launchdarkly_ai_server.evaluations.api import HttpResponse from launchdarkly_ai_server.evaluations.runner import EvaluationsRunner +from launchdarkly_ai_server.evaluations.tools import ( + create_wire_tools, + handler_config_tools, +) from launchdarkly_ai_server.evaluations.types import ( DatasetRef, EvaluationRef, @@ -1279,6 +1283,30 @@ async def handler( "schema must be JSON-serializable", id="schema_holds_infinity", ), + pytest.param( + [ + EvalTool( + key="lookup_order", + implementation=lookup_order, + schema={1: {"type": "string"}}, # type: ignore[dict-item] + ) + ], + r"'lookup_order' schema must be JSON-serializable.*" + r"keys must be str, not int \(got 1\)", + id="schema_has_a_non_string_key", + ), + pytest.param( + [ + EvalTool( + key="lookup_order", + implementation=lookup_order, + schema={"anyOf": [{"type": "object"}, {None: "x"}]}, + ) + ], + r"'lookup_order' schema must be JSON-serializable.*" + r"keys must be str, not NoneType \(got None\)", + id="schema_has_a_non_string_key_inside_a_list", + ), pytest.param( [ EvalTool( @@ -1385,37 +1413,100 @@ async def handler(*args: object) -> dict[str, Any]: ] +def library_tool_response(key: str) -> HttpResponse: + return response( + 200, + {"key": key, "version": 7, "description": "", "schema": {"type": "object"}}, + ) + + @pytest.mark.asyncio -async def test_a_repeated_tool_key_is_rejected_with_zero_requests() -> None: +@pytest.mark.parametrize( + ("first", "second", "library_keys", "match"), + [ + pytest.param( + "library", + "inline", + ["lookup_order"], + r"'lookup_order' \(inline\) collides with 'lookup_order' \(library\)", + id="library_and_inline", + ), + pytest.param( + "inline", + "inline", + [], + r"'lookup_order' \(inline\) collides with 'lookup_order' \(inline\)", + id="inline_twice", + ), + pytest.param( + "library", + "library", + ["lookup_order"], + r"'lookup_order' \(library\) collides with 'lookup_order' \(library\)", + id="library_twice", + ), + ], +) +async def test_a_repeated_tool_key_names_both_sources_with_zero_requests( + first: str, second: str, library_keys: list[str], match: str +) -> None: """One key names one tool, whichever source each entry came from.""" - transport = SequencedTransport([]) + transport = SequencedTransport([library_tool_response(key) for key in library_keys]) + evals = init_evaluations( + project_key="proj", api_key="token", sdk_key="sdk-key", transport=transport + ) + library_tool = ( + await evals.tools.get("lookup_order", implementation=lookup_order) + if library_keys + else None + ) + + def entry(source: str) -> EvalTool: + if source == "library": + assert library_tool is not None + return library_tool + return EvalTool( + key="lookup_order", implementation=refund_order, schema=ORDER_SCHEMA + ) + + reads = len(transport.requests) + with pytest.raises(EvaluationsError, match=match): + await evals.run( + key="eval-key", + dataset="golden", + handler=tagged(successful_handler), + tools=[entry(first), entry(second)], + generation={"provider": "OpenAI", "model": "gpt-4o", "mode": "completion"}, + ) + + assert len(transport.requests) == reads + + +@pytest.mark.asyncio +async def test_keys_equal_under_case_folding_collide_with_zero_requests() -> None: + """``straße`` and ``strasse`` are both lowercase, and fold to one key.""" + transport = SequencedTransport([library_tool_response("strasse")]) evals = init_evaluations( project_key="proj", api_key="token", sdk_key="sdk-key", transport=transport ) + library_tool = await evals.tools.get("strasse", implementation=lookup_order) with pytest.raises( - EvaluationsError, match=r"'lookup_order' appears more than once" + EvaluationsError, + match=r"'straße' \(inline\) collides with 'strasse' \(library\)", ): await evals.run( key="eval-key", dataset="golden", handler=tagged(successful_handler), tools=[ - EvalTool( - key="lookup_order", - implementation=lookup_order, - schema=ORDER_SCHEMA, - ), - EvalTool( - key="lookup_order", - implementation=refund_order, - schema=ORDER_SCHEMA, - ), + library_tool, + EvalTool(key="straße", implementation=refund_order, schema={}), ], generation={"provider": "OpenAI", "model": "gpt-4o", "mode": "completion"}, ) - assert transport.requests == [] + assert len(transport.requests) == 1 @pytest.mark.asyncio @@ -4034,6 +4125,81 @@ def test_an_inline_tool_requires_a_schema() -> None: assert tool.schema == {} +def test_a_tool_holds_its_own_copy_of_the_schema() -> None: + """Changing the caller's dict afterwards changes nothing the tool sends.""" + schema: dict[str, Any] = { + "type": "object", + "properties": {"order_id": {"type": "string"}}, + } + tool = EvalTool(key="lookup_order", implementation=lookup_order, schema=schema) + + schema["properties"]["order_id"]["type"] = "integer" + schema["extra"] = True + + expected = {"type": "object", "properties": {"order_id": {"type": "string"}}} + assert tool.schema == expected + assert tool.to_create_wire()["schema"] == expected + assert handler_config_tools([tool])["lookup_order"]["parameters"] == expected + + +def test_a_handler_config_does_not_share_the_create_body_schema() -> None: + expected = json.loads(json.dumps(ORDER_SCHEMA)) + tool = EvalTool( + key="lookup_order", + implementation=lookup_order, + schema=json.loads(json.dumps(ORDER_SCHEMA)), + ) + + config_tools = handler_config_tools([tool]) + config_tools["lookup_order"]["parameters"]["properties"]["order_id"] = {} + wire = create_wire_tools([tool]) + wire[0]["schema"]["required"].append("note") + + assert tool.schema == expected + assert tool.to_create_wire()["schema"] == expected + assert handler_config_tools([tool])["lookup_order"]["parameters"] == expected + + +@pytest.mark.asyncio +async def test_a_schema_changed_after_construction_is_not_what_is_sent() -> None: + transport = SequencedTransport(hosted_dataset_responses()) + evals = init_evaluations( + project_key="proj", api_key="token", sdk_key="sdk-key", transport=transport + ) + schema = json.loads(json.dumps(ORDER_SCHEMA)) + tool = EvalTool(key="lookup_order", implementation=lookup_order, schema=schema) + seen: dict[str, Any] = {} + + async def handler( + config: dict[str, Any], + user_input: str | None, + tool_handlers: dict[str, Callable[..., Any]], + variables: dict[str, Any], + ) -> dict[str, Any]: + seen["parameters"] = json.loads( + json.dumps(config["tools"]["lookup_order"]["parameters"]) + ) + # A handler that changes its config cannot change the tool. + config["tools"]["lookup_order"]["parameters"]["required"].clear() + return {"output": "ok"} + + # Not valid JSON, so the run would fail if this reached validation. + schema["default"] = float("nan") + + await evals.run( + key="eval-key", + dataset="golden", + handler=tagged(handler), + tools=[tool], + generation={"provider": "OpenAI", "model": "gpt-4o", "mode": "completion"}, + ) + + assert transport.requests[2]["body"]["tools"][0]["schema"] == ORDER_SCHEMA + assert seen["parameters"] == ORDER_SCHEMA + assert tool.schema == ORDER_SCHEMA + assert tool.to_create_wire()["schema"] == ORDER_SCHEMA + + def test_a_tool_cannot_be_reassigned_to_a_library_tool() -> None: tool = EvalTool( key="lookup_order", implementation=lookup_order, schema=ORDER_SCHEMA