Skip to content

Commit 7c6936c

Browse files
fix(sdk): address PR review feedback on StepRecorder OSS/Galileo boundary
- record_step()/.child() take an opt-in container_types param instead of hardcoding "trace"/"session" as the step types that serialize empty children as [] - step types are caller-defined strings, not a core vocabulary. - Move the galileo-dependent StepRecorder integration test out of the SDK into evaluators/contrib/galileo, where its dependency belongs. - Replace the README's trace/session example with a generic workflow/lookup one so the OSS SDK docs don't lean on Galileo's hierarchy vocabulary. Co-Authored-By: Claude Code <noreply@anthropic.com>
1 parent cb872b2 commit 7c6936c

5 files changed

Lines changed: 91 additions & 70 deletions

File tree

‎evaluators/contrib/galileo/pyproject.toml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ dependencies = [
1616

1717
[project.optional-dependencies]
1818
dev = [
19+
"agent-control-sdk>=8.11.0",
20+
"agent-control-telemetry>=8.6.0",
21+
"agent-control-engine>=8.6.0",
1922
"pytest>=8.0.0",
2023
"pytest-asyncio>=0.23.0",
2124
"pytest-cov>=4.0.0",
@@ -37,3 +40,6 @@ packages = ["src/agent_control_evaluator_galileo"]
3740
[tool.uv.sources]
3841
agent-control-evaluators = { path = "../../builtin", editable = true }
3942
agent-control-models = { path = "../../../models", editable = true }
43+
agent-control-sdk = { path = "../../../sdks/python", editable = true }
44+
agent-control-telemetry = { path = "../../../telemetry", editable = true }
45+
agent-control-engine = { path = "../../../engine", editable = true }

sdks/python/tests/test_step_recorder_galileo_integration.py renamed to evaluators/contrib/galileo/tests/test_step_recorder_galileo_integration.py

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,9 @@
1-
"""Cross-package check: StepRecorder trees feed the Galileo record factory.
2-
3-
The galileo extras are normally installed in the dev environment (see
4-
``test_evaluators_optional_imports.py``), so this skips cleanly when they
5-
are not available instead of failing the suite.
6-
"""
1+
"""Cross-package check: StepRecorder trees feed the Galileo record factory."""
72

83
from __future__ import annotations
94

10-
import importlib.util
11-
12-
import pytest
13-
145
from agent_control import record_step
15-
16-
17-
def _module_available(name: str) -> bool:
18-
try:
19-
return importlib.util.find_spec(name) is not None
20-
except (ImportError, ValueError):
21-
return False
22-
23-
24-
_GALILEO_INSTALLED = _module_available("agent_control_evaluator_galileo.records")
25-
26-
pytestmark = pytest.mark.skipif(
27-
not _GALILEO_INSTALLED,
28-
reason="agent-control-evaluator-galileo extras not installed in this environment",
29-
)
30-
31-
32-
def _record_from_step(step):
33-
from agent_control_evaluator_galileo.records.factory import record_from_step
34-
35-
return record_from_step(step)
6+
from agent_control_evaluator_galileo.records.factory import record_from_step as _record_from_step
367

378

389
def test_recorder_built_trace_matches_demo_shape():

‎sdks/python/README.md‎

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -84,53 +84,59 @@ async with agent_control.AgentControlClient() as client:
8484
The existing `evaluate_controls` helper remains available for callers that
8585
prefer its field-based convenience arguments.
8686

87-
## Building trace/session steps
87+
## Building a step with children
8888

89-
Trace- and session-level controls evaluate the aggregate of several
90-
already-executed child steps, so `Step.children` has to be populated by the
91-
caller. Building that tree by hand means hand-rolling a side-channel record
92-
for every leaf call and converting it into `Step` objects afterward:
89+
A control's scope can target any step type, including one that aggregates
90+
several already-executed child steps - `Step.children` just has to be
91+
populated by the caller. Building that tree by hand means hand-rolling a
92+
side-channel record for every leaf call and converting it into `Step`
93+
objects afterward:
9394

9495
```python
9596
# Before: a hand-built dict record next to the real return value
9697
def execute_policy_search(query):
9798
docs = search_policy_documents(query)
98-
record = {"type": "retriever", "query": query, "docs": docs}
99+
record = {"type": "lookup", "query": query, "docs": docs}
99100
return StepExecution(value=docs, record=record)
100101

101-
spans = []
102+
children = []
102103
result = execute_policy_search(query)
103-
spans.append(result.record)
104+
children.append(result.record)
104105
...
105-
trace_step = build_agent_control_step({"type": "trace", "spans": spans, ...})
106+
workflow_step = build_agent_control_step({"type": "workflow", "children": children, ...})
106107
```
107108

108109
`agent_control.record_step()` builds the same tree incrementally, in `Step`
109110
vocabulary, with no intermediate dict and no converter:
110111

111112
```python
112-
with agent_control.record_step("trace", "banking_trace", input={"request": req}) as trace:
113-
with trace.child("retriever", "policy_lookup", input=query) as span:
113+
with agent_control.record_step("workflow", "banking_workflow", input={"request": req}) as workflow:
114+
with workflow.child("lookup", "policy_lookup", input=query) as span:
114115
span.output = search_policy_documents(query)
115116

116-
account = trace.call(lookup_account, account_id="acct-1001", step_type="tool")
117-
plan = trace.call(run_banking_model, req, step_type="llm", tools=TOOL_DEFINITIONS)
118-
trace.output = {"status": "planned", "message": plan["content"]}
117+
account = workflow.call(lookup_account, account_id="acct-1001", step_type="tool")
118+
plan = workflow.call(run_banking_model, req, step_type="llm", tools=TOOL_DEFINITIONS)
119+
workflow.output = {"status": "planned", "message": plan["content"]}
119120

120-
result = await trace.evaluate(stage="post")
121+
result = await workflow.evaluate(stage="post")
121122
```
122123

123-
`trace.call(...)`/`await trace.acall(...)` run the function and record it as
124-
a child using the same capture logic as `@control()` (input from bound
124+
`workflow.call(...)`/`await workflow.acall(...)` run the function and record
125+
it as a child using the same capture logic as `@control()` (input from bound
125126
arguments, output from the return value), then return the real result -
126127
removing the need for a separate `StepExecution`-style wrapper. A failed
127128
call is still recorded (with the error in `context`) before the exception is
128-
re-raised. `trace.child(...)` nests another recorder the same way, so
129-
sessions nest traces with `session.child("trace", ...)`.
130-
131-
`trace.build()` produces the frozen `Step`; `trace.evaluate(...)` builds it
132-
and evaluates it in one call via `evaluate_step()` - the same function
133-
`evaluate_controls()` uses internally once its `Step` is built.
129+
re-raised. `workflow.child(...)` nests another recorder the same way, so a
130+
parent step can nest children of any depth, e.g. `session.child("trace",
131+
...)` for callers that model multi-turn sessions of traces.
132+
133+
`workflow.build()` produces the frozen `Step`, with `children=[]` instead of
134+
omitted (`None`) for a step type passed via `container_types` even when no
135+
children were recorded - e.g. `record_step("workflow", ..., container_types=
136+
{"workflow"})` for a workflow that legitimately ran with zero children.
137+
`workflow.evaluate(...)` builds the step and evaluates it in one call via
138+
`evaluate_step()` - the same function `evaluate_controls()` uses internally
139+
once its `Step` is built.
134140

135141
## Sharing an OpenTelemetry provider with Google ADK
136142

‎sdks/python/src/agent_control/step_recorder.py‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
from __future__ import annotations
1111

12-
from collections.abc import Awaitable, Callable, Mapping
12+
from collections.abc import Awaitable, Callable, Collection, Mapping
1313
from typing import Any, Literal, TypeVar
1414

1515
from agent_control_models import EvaluationResult, JSONObject, JSONValue, Step
@@ -21,12 +21,6 @@
2121

2222
T = TypeVar("T")
2323

24-
# Step types whose children are reported as a span/trace tree; an absent
25-
# recording means "nothing happened", which the record factory treats
26-
# differently from "not applicable" (None). See models/agent.py and
27-
# records/factory.py for the trace/session child-shape rules this mirrors.
28-
_EMPTY_CHILDREN_STEP_TYPES = frozenset({"trace", "session"})
29-
3024

3125
def record_step(
3226
step_type: str,
@@ -36,15 +30,26 @@ def record_step(
3630
context: dict[str, Any] | None = None,
3731
tools: list[JSONObject] | None = None,
3832
ground_truth: JSONValue | None = None,
33+
container_types: Collection[str] = (),
3934
) -> StepRecorder:
40-
"""Start building a ``Step`` tree. Use as a context manager."""
35+
"""Start building a ``Step`` tree. Use as a context manager.
36+
37+
``container_types`` names step types whose ``children`` should be
38+
serialized as ``[]`` rather than omitted when none were recorded - e.g.
39+
a trace/session that legitimately ran with zero children, as opposed to
40+
a leaf type for which children isn't a meaningful concept at all. Step
41+
types are caller-defined strings, so this isn't assumed; opt in with
42+
e.g. ``container_types={"trace", "session"}``. Inherited by nested
43+
``.child()`` recorders unless overridden there.
44+
"""
4145
return StepRecorder(
4246
step_type,
4347
name,
4448
input=input,
4549
context=context,
4650
tools=tools,
4751
ground_truth=ground_truth,
52+
container_types=frozenset(container_types),
4853
)
4954

5055

@@ -64,6 +69,7 @@ def __init__(
6469
context: dict[str, Any] | None = None,
6570
tools: list[JSONObject] | None = None,
6671
ground_truth: JSONValue | None = None,
72+
container_types: frozenset[str] = frozenset(),
6773
_parent: StepRecorder | None = None,
6874
) -> None:
6975
self.type = ensure_step_type(step_type)
@@ -75,6 +81,7 @@ def __init__(
7581
self.ground_truth = ground_truth
7682
self._children: list[Step] = []
7783
self._parent = _parent
84+
self._container_types = container_types
7885

7986
def __enter__(self) -> StepRecorder:
8087
return self
@@ -92,15 +99,23 @@ def child(
9299
context: dict[str, Any] | None = None,
93100
tools: list[JSONObject] | None = None,
94101
ground_truth: JSONValue | None = None,
102+
container_types: Collection[str] | None = None,
95103
) -> StepRecorder:
96-
"""Start a nested recorder; it attaches to this node on ``__exit__``."""
104+
"""Start a nested recorder; it attaches to this node on ``__exit__``.
105+
106+
Inherits this node's ``container_types`` unless ``container_types``
107+
is given explicitly.
108+
"""
97109
return StepRecorder(
98110
step_type,
99111
name,
100112
input=input,
101113
context=context,
102114
tools=tools,
103115
ground_truth=ground_truth,
116+
container_types=(
117+
self._container_types if container_types is None else frozenset(container_types)
118+
),
104119
_parent=self,
105120
)
106121

@@ -178,7 +193,7 @@ def build(self) -> Step:
178193
step_dict["ground_truth"] = self.ground_truth
179194
if self._children:
180195
step_dict["children"] = list(self._children)
181-
elif self.type in _EMPTY_CHILDREN_STEP_TYPES:
196+
elif self.type in self._container_types:
182197
step_dict["children"] = []
183198
return Step(**step_dict) # type: ignore[arg-type]
184199

‎sdks/python/tests/test_step_recorder.py‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -140,25 +140,48 @@ def test_build_includes_context_tools_ground_truth():
140140
assert step.ground_truth == "hello"
141141

142142

143-
def test_empty_trace_and_session_build_empty_children_list():
144-
"""A trace/session with no recorded children gets children=[], not None."""
145-
with record_step("trace", "empty_trace") as trace:
143+
def test_container_types_opts_in_to_empty_children_list():
144+
"""A declared container type with no recorded children gets children=[], not None."""
145+
with record_step("trace", "empty_trace", container_types={"trace", "session"}) as trace:
146146
pass
147-
with record_step("session", "empty_session") as session:
147+
with record_step("session", "empty_session", container_types={"trace", "session"}) as session:
148148
pass
149149

150150
assert trace.build().children == []
151151
assert session.build().children == []
152152

153153

154-
def test_llm_step_without_children_stays_none():
155-
"""Non trace/session step types are untouched when no children were recorded."""
154+
def test_step_type_without_container_types_stays_none():
155+
"""Step types are caller-defined strings; children stays None unless the
156+
caller opts a type into container_types - no type is a container by default."""
157+
with record_step("trace", "empty_trace") as trace:
158+
pass
156159
with record_step("llm", "respond", input="hi") as leaf:
157160
leaf.output = "hello"
158161

162+
assert trace.build().children is None
159163
assert leaf.build().children is None
160164

161165

166+
def test_child_inherits_parent_container_types():
167+
"""A nested .child() recorder inherits the parent's container_types unless overridden."""
168+
with record_step("session", "s", container_types={"trace", "session"}) as session:
169+
with session.child("trace", "empty_trace") as trace:
170+
pass
171+
172+
assert trace.build().children == []
173+
assert session.build().children == [trace.build()]
174+
175+
176+
def test_child_can_override_container_types():
177+
"""A nested .child() recorder can override the inherited container_types."""
178+
with record_step("session", "s", container_types={"trace", "session"}) as session:
179+
with session.child("trace", "empty_trace", container_types=()) as trace:
180+
pass
181+
182+
assert trace.build().children is None
183+
184+
162185
def test_add_attaches_prebuilt_step_or_dict():
163186
"""add() accepts both a real Step and a plain dict (coerced via Step.model_validate)."""
164187
with record_step("trace", "t") as trace:

0 commit comments

Comments
 (0)