Review view: chat process display - #457
Open
rejojer wants to merge 14 commits into
Open
Conversation
…nts view
chat(stream=True) now returns a ChatStream: iterating it yields the
answer text with the run woven in by default — "[thinking] " sections,
one "[tool] name arguments" line per call with its clipped result —
and .events yields the run as typed dicts (thinking/answer deltas,
tool_call with parsed arguments, tool_result with the full output).
One run serves one view; close() kills it like a closed generator.
- show_process: on by default ("on where available"); False for the
bare answer stream; a dict (ChatProcessOptions: thinking, tool_calls,
tool_results, max_chars) selects the parts. Explicit True without
stream=True raises.
- Managed clients weave what the endpoint serves: tool-call lines
parsed from its block_metadata chunk tags (that wire carries no
thinking and no tool results); old-wire chunks stay plain answer
text, and the bare answer view no longer leaks tool-argument JSON.
- Engine: one typed-event primitive (_chat_events_agen /
_cloud_chunk_events) with _weave as a pure renderer over it; the
chat lane's prologue is shared via _chat_agent, behavior unchanged
on chat_completions/responses/messages.
453 tests green (16 new, red-verified), no-openai-agents leg simulated,
pyright flat vs main.
Claude-Session: https://claude.ai/code/session_014GN8u3zdH3RpeftHZChavP
…idate show_process before the managed request Three review findings on the show_process weave, all red-verified: - _weave nested every tool result under the most recent [tool] line, which misattributes results when a turn makes parallel calls (the SDK streams all calls, then all results). A result now nests only when the line above is its own call (by call_id); otherwise it stands alone with its call's clipped arguments echoed, so same-name parallel calls stay tellable apart. Hidden-call mode falls out unchanged (no stored arguments, no echo). - Empty deltas now stop at the event source. chat_completions and the managed chunk lane both filter them; the new local lane did not, so a mid-stream "" (litellm forwards annotated/provider-field empties) leaked into the bare view and flipped _weave sections, splitting one thinking burst into repeated labels. - The managed streaming lane sent the billed request before _process_options ran, so a config typo cost a real chat call. chat() now chokes on bad show_process before dispatching, matching the local lane's validate-first order. Two docstring truths: close()'s "a run never consumed never starts" holds only for own-model chat (the managed request is already on the wire), and the module docstring now covers the managed chunk weave. 456 tests green; the three new ones red-verified; managed-lane paths re-run with the agents package blocked; pyright adds nothing on touched lines. Claude-Session: https://claude.ai/code/session_01XeeD2214z6Vd6qKcAi9ZvJ
…am overloads
Four review fixes plus two coverage gaps, each red- or
mutation-verified:
- _cloud_chunk_events treated any non-tool_use tag inside an open tool
block as answer text, so argument JSON leaked into the
show_process=False answer — the one meant to be appended back as
conversation history. Inside an open block nothing is answer:
argument chunks now accumulate under any tag. And non-string
argument pieces stringify at the join instead of killing the whole
stream with a raw TypeError.
- _process_options sorted unknown keys before repr-ing them, so
mixed-type keys ({1: True, "foo": 1}) raised a bare TypeError past
the caller's `except PageIndexAPIError`; sorting the reprs keeps the
single error type.
- chat() gains @overload on stream, so the docstring's own `.events`
usage type-checks for py.typed consumers (previously pyright ruled
`Cannot access attribute "events" for class "str"` on the exact
documented snippet). pageindex/ error count unchanged (234).
- Coverage: the streaming lane's whole `finally` could be deleted with
the suite still green — the new abandonment test pins the teardown
(pump exits, turn 2 emits nothing, _aclose_backend closes the
per-call client). And FakeModel emitted only the reasoning event
production never sends (litellm folds reasoning into
reasoning_content, which arrives as summary deltas); it now
alternates variants, so dropping either from the isinstance tuple
goes red.
459 tests green; managed-path tests re-run with the agents package
blocked; flake8 parity on every touched file.
Claude-Session: https://claude.ai/code/session_01DWBCCTDzwuVamBf5MvQ4eP
…sy show_process message
ChatStream.events was a property whose getter latched the stream's one
view on mere attribute access: a debugger variable pane, hasattr, or
getattr(stream, "events", None) — which PageIndexAPIError escapes, as
getattr only swallows AttributeError — was enough to make a later
`for chunk in stream:` refuse, with nothing consumed. The getter now
returns a lazy generator: the managed refusal, the view claim and the
run start all happen on first consumption, so introspection is
side-effect free and the text view stays usable after a probe.
And the stream=False guard's message told falsy-but-not-False values
("show_process=0", "") that they passed show_process=True; the check
itself is the ruled falsy-{} trap and stands, but the message now
names the off values and echoes what was got.
461 tests green (2 red-verified new: inert read on both lanes, plus
the falsy-message case); changed managed-path tests re-run with the
agents package blocked; pyright pageindex/ 234 -> 234.
Claude-Session: https://claude.ai/code/session_01DWBCCTDzwuVamBf5MvQ4eP
[tool_call] and [tool_result] replace the "[tool]" label and the "->" arrow, so the text view's labels are exactly the .events type names (config keys stay plural — they switch a class of lines; each line is one instance). Claude-Session: https://claude.ai/code/session_014GN8u3zdH3RpeftHZChavP
tool_call / tool_result (singular) everywhere: event types, text labels, and now the config keys, which select event types by name. Claude-Session: https://claude.ai/code/session_014GN8u3zdH3RpeftHZChavP
rejojer
changed the title
…sults; bad show_process chokes first - ChatStream.events delegated with `yield from`, so a dropped handle (next(stream.events), for ... break) closed the shared run on GC and the rest of the run silently vanished. A plain loop leaves it alone. - _weave filled call_args only past the tool_call visibility guard, so with call lines hidden the standalone result lines never carried the arguments they promise. - show_process is validated before the stream check: an invalid value is refused as such instead of being told to add stream=True and then refused again; the managed lane's duplicate choke goes with it. - Docstring: show_process is not own-model-only. Claude-Session: https://claude.ai/code/session_016M3qaQedSK7L4DwysFRmk2
…r; stream docstring says show_process is on by default
- The managed endpoint reports a server-side failure as a final
{"error": ...} chunk after the partial answer (api.py refunds the
credits, then yields it). Neither chunk decoder looked at it, so
chat(stream=True) and chat_completions(stream=True) in both modes
ended as an apparently complete short answer with no exception. One
guard in each decoder raises PageIndexAPIError; the partial answer
is still delivered first.
- The `stream:` arg and the Returns block still described the pre-PR
contract (bare text chunks); only the show_process paragraph said it
is on by default.
Claude-Session: https://claude.ai/code/session_01PYr9yG1FPQxKCA9m7ECQWY
…ve at runtime ChatStream was importable in client.py only under TYPE_CHECKING (a real import would have dragged local_chat's asyncio stack into `import pageindex`), which left chat()'s return annotation a dangling string: typing.get_type_hints(PageIndexClient.chat) raised NameError, and so did anything that introspects signatures — agents' function_tool (client.chat) died on it before looking at a single parameter. The class touches neither asyncio nor the agent frameworks, so it moves to pageindex/chat_stream.py, client.py imports it for real, and the package exports it directly instead of lazily. `import pageindex` still leaves local_chat unloaded. Claude-Session: https://claude.ai/code/session_01PYr9yG1FPQxKCA9m7ECQWY
…nion main's chat() returns a wider union (the protocol lanes), so the exact comparison held only on this branch; membership is what the test is for. Claude-Session: https://claude.ai/code/session_01PYr9yG1FPQxKCA9m7ECQWY
…s, a guarded eager path, the old import path pinned Review of #471 found the move's guards thinner than they look: - chat_stream.py had no `from __future__ import annotations`, unlike every sibling module, which made its `-> "ChatStream"` quotes load-bearing: unquoting them — the very edit this move made in client.py, and what `ruff --select UP037 --fix` does — broke `import pageindex` outright. - The import in client.py is the whole fix and reads like a typing-only one; a comment says why it must stay real. - The lazy-import test's denylist named no framework, so agents, litellm, openai or anthropic could join the eager path with a green suite — the cost the module was split out to avoid. - The type-hints walk had no floor, so it could silently stop covering anything, and nothing pinned `pageindex.local_chat.ChatStream`, the path the class shipped under in 0.2.11-0.2.14. - local_chat's module docstring still claimed the class. All four guards mutation-checked red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review view only — do not merge. The base is the frozen commit
7d18cc0(main as of before this work landed), so this PR keeps the complete feature diff no matter how far main moves.Shipped in v0.2.13 — merged to main as 555c370 (originally #454, which carries the design record and round-1 review).
Base branch
review/base-7d18cc0never advances; merging here would only pollute that branch, never main.