feat(analysis): turn-messages viewer with vendored fzstd decompressor#1115
Open
ajcasagrande wants to merge 1 commit into
Open
feat(analysis): turn-messages viewer with vendored fzstd decompressor#1115ajcasagrande wants to merge 1 commit into
ajcasagrande wants to merge 1 commit into
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@0651c380d600094d3da15b3378dd3911458e925dRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@0651c380d600094d3da15b3378dd3911458e925dLast updated for commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
ajcasagrande
force-pushed
the
ajc/turn-messages-viewer
branch
from
30ce0e7 to
0651c38
Compare
ajcasagrande
requested review from
FrankD412,
debermudez,
ganeshku1,
ilana-n,
lkomali and
matthewkotila
as code owners
Contributor
WalkthroughThis PR adds an ChangesTurn Messages Viewer Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/analysis/test_turn_messages.py (1)
59-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer unpacking over list concatenation.
Static analysis (RUF005) flags both
history + [...]concatenations; unpacking is more idiomatic and avoids an extra list creation.♻️ Proposed fix
for t in range(n_turns): - history = history + [{"role": "user", "content": f"question {t}"}] + history = [*history, {"role": "user", "content": f"question {t}"}] recs.append(_rec(conv, t, list(history), start_s=float(t))) - history = history + [{"role": "assistant", "content": f"answer {t}"}] + history = [*history, {"role": "assistant", "content": f"answer {t}"}]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/analysis/test_turn_messages.py` around lines 59 - 67, The `_accumulated` helper in `test_turn_messages.py` uses `history + [...]` twice, which triggers RUF005 and creates unnecessary intermediate lists. Update the `history` updates to use unpacking in the same `_accumulated` function so the list is extended idiomatically without concatenation. Keep the existing behavior and structure of `_rec` calls unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli-options.md`:
- Around line 3033-3060: The new aiperf turn-messages docs section skips heading
levels and jumps from the H2 title to H4 option entries, causing the markdown
hierarchy to be invalid. Update the section structure so the option list is
nested under an intermediate H3 (or otherwise regenerate the headings) while
preserving the existing content around aiperf turn-messages, --run-dirs, --out,
--limit-conversations, --max-turns, and --content-cap.
In `@src/aiperf/analysis/turn_messages.py`:
- Around line 358-368: The HTML generation in turn_messages currently relies on
default text encoding for VIEWER_TEMPLATE.read_text(), FZSTD_JS.read_text(), and
out_path.write_text(), which can fail on Windows with non-ASCII content. Update
the template reads and the final write in turn_messages to explicitly use utf-8
encoding, keeping the existing error handling and avoiding
safe_read_template_path since VIEWER_TEMPLATE and FZSTD_JS are internal Path
objects, not user-supplied paths.
---
Nitpick comments:
In `@tests/unit/analysis/test_turn_messages.py`:
- Around line 59-67: The `_accumulated` helper in `test_turn_messages.py` uses
`history + [...]` twice, which triggers RUF005 and creates unnecessary
intermediate lists. Update the `history` updates to use unpacking in the same
`_accumulated` function so the list is extended idiomatically without
concatenation. Keep the existing behavior and structure of `_rec` calls
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f86b47eb-ab72-4d4b-8d77-a20e7b4a50a2
📒 Files selected for processing (11)
ATTRIBUTIONS.mddocs/cli-options.mdpyproject.tomlsrc/aiperf/analysis/fzstd.umd.jssrc/aiperf/analysis/turn_messages.pysrc/aiperf/analysis/turn_messages_viewer.htmlsrc/aiperf/cli.pysrc/aiperf/cli_commands/turn_messages.pytests/unit/analysis/test_turn_messages.pytools/ergonomics_baseline.jsontools/ruff_baseline.json
Comment on lines
+3033
to
+3060
| ## `aiperf turn-messages` | ||
|
|
||
| Render a collapsible HTML viewer of the per-turn input messages a run sent. | ||
|
|
||
| #### `--run-dirs`, `--empty-run-dirs` `<list>` _(Required)_ | ||
|
|
||
| One or more AIPerf run directories. | ||
|
|
||
| #### `-o`, `--out` `<str>` | ||
|
|
||
| Output HTML path. Only valid when a single run directory is given. | ||
|
|
||
| #### `-n`, `--limit-conversations` `<int>` | ||
|
|
||
| Max conversations to render (roots first, then by earliest request time). | ||
| <br/>_Default: `40`_ | ||
|
|
||
| #### `--max-turns` `<int>` | ||
|
|
||
| Max turns rendered per conversation; the rest are summarized as a hidden count. | ||
| <br/>_Default: `60`_ | ||
|
|
||
| #### `--content-cap` `<int>` | ||
|
|
||
| Max characters kept per unique message body; longer bodies are truncated with a remaining-chars note. Raise for full fidelity. | ||
| <br/>_Default: `8000`_ | ||
|
|
||
| <hr/> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the heading hierarchy valid. The new section jumps from H2 to H4, which triggers MD001 here. Add an intermediate H3 (or regenerate the section structure) so the docs stay lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 3037-3037: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli-options.md` around lines 3033 - 3060, The new aiperf turn-messages
docs section skips heading levels and jumps from the H2 title to H4 option
entries, causing the markdown hierarchy to be invalid. Update the section
structure so the option list is nested under an intermediate H3 (or otherwise
regenerate the headings) while preserving the existing content around aiperf
turn-messages, --run-dirs, --out, --limit-conversations, --max-turns, and
--content-cap.
Source: Linters/SAST tools
Comment on lines
+358
to
+368
| html = ( | ||
| VIEWER_TEMPLATE.read_text() | ||
| .replace("__FZSTD_JS__", FZSTD_JS.read_text()) | ||
| .replace("__PAYLOAD_B64__", payload_b64) | ||
| ) | ||
| default_dir = run_dir.parent if run_dir.is_file() else run_dir | ||
| out_path = out or (default_dir / "turn_messages.html") | ||
| try: | ||
| out_path.write_text(html) | ||
| except OSError as e: | ||
| raise TurnMessagesError(f"could not write {out_path}: {e}") from e |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm helper location/signature so the read replacement is correct.
fd -t f path_safety.py src | xargs -r -I{} sh -c 'echo "== {} =="; cat -n {}'
rg -nP 'def\s+safe_read_template_path' -C2Repository: ai-dynamo/aiperf
Length of output: 2807
🏁 Script executed:
#!/bin/bash
# Find and read the relevant section of turn_messages.py
fd -t f turn_messages.py src | head -1 | xargs -r -I{} sh -c '
echo "=== Full file overview ==="
wc -l {}
echo ""
echo "=== Imports and constants at top ==="
head -50 {}
echo ""
echo "=== Lines around 358 (target area) ==="
sed -n "340,380p" {}
'Repository: ai-dynamo/aiperf
Length of output: 3881
🏁 Script executed:
#!/bin/bash
# Check imports and see correct pattern
fd -t f turn_messages.py src | head -1 | xargs -r -I{} sh -c '
echo "=== All imports in turn_messages.py ==="
grep -n "^from\|^import" {}
echo ""
echo "=== Any existing use of safe_read_template_path in codebase ==="
rg -n "safe_read_template_path" --type py -C2
'Repository: ai-dynamo/aiperf
Length of output: 7388
🏁 Script executed:
#!/bin/bash
# Check if VIEWER_TEMPLATE and FZSTD_JS are internal package data or user-supplied
rg -B5 -A2 "VIEWER_TEMPLATE\s*=" src/aiperf/analysis/turn_messages.py
echo "---"
# Check the docstring/context of safe_read_template_path more carefully
sed -n "4,12p" src/aiperf/common/path_safety.pyRepository: ai-dynamo/aiperf
Length of output: 845
Use encoding="utf-8" for template reads and HTML writes to prevent Windows crashes.
read_text() and write_text() default to platform encoding (cp1252 on Windows), which will raise UnicodeDecodeError/UnicodeEncodeError on the non-ASCII characters in the viewer template and payload (▸ … · × —). The fix is straightforward: add encoding="utf-8" to all three calls.
Note: VIEWER_TEMPLATE and FZSTD_JS are internal package-data Path objects, not user-supplied strings, so they should not use safe_read_template_path (which is designed for CWE-22 hardening of user-supplied template paths and requires str input plus None-handling).
Proposed fix
payload_b64 = base64.b64encode(compressed).decode()
html = (
- VIEWER_TEMPLATE.read_text()
- .replace("__FZSTD_JS__", FZSTD_JS.read_text())
+ VIEWER_TEMPLATE.read_text(encoding="utf-8")
+ .replace("__FZSTD_JS__", FZSTD_JS.read_text(encoding="utf-8"))
.replace("__PAYLOAD_B64__", payload_b64)
)
default_dir = run_dir.parent if run_dir.is_file() else run_dir
out_path = out or (default_dir / "turn_messages.html")
try:
- out_path.write_text(html)
+ out_path.write_text(html, encoding="utf-8")
except OSError as e:
raise TurnMessagesError(f"could not write {out_path}: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| html = ( | |
| VIEWER_TEMPLATE.read_text() | |
| .replace("__FZSTD_JS__", FZSTD_JS.read_text()) | |
| .replace("__PAYLOAD_B64__", payload_b64) | |
| ) | |
| default_dir = run_dir.parent if run_dir.is_file() else run_dir | |
| out_path = out or (default_dir / "turn_messages.html") | |
| try: | |
| out_path.write_text(html) | |
| except OSError as e: | |
| raise TurnMessagesError(f"could not write {out_path}: {e}") from e | |
| html = ( | |
| VIEWER_TEMPLATE.read_text(encoding="utf-8") | |
| .replace("__FZSTD_JS__", FZSTD_JS.read_text(encoding="utf-8")) | |
| .replace("__PAYLOAD_B64__", payload_b64) | |
| ) | |
| default_dir = run_dir.parent if run_dir.is_file() else run_dir | |
| out_path = out or (default_dir / "turn_messages.html") | |
| try: | |
| out_path.write_text(html, encoding="utf-8") | |
| except OSError as e: | |
| raise TurnMessagesError(f"could not write {out_path}: {e}") from e |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiperf/analysis/turn_messages.py` around lines 358 - 368, The HTML
generation in turn_messages currently relies on default text encoding for
VIEWER_TEMPLATE.read_text(), FZSTD_JS.read_text(), and out_path.write_text(),
which can fail on Windows with non-ASCII content. Update the template reads and
the final write in turn_messages to explicitly use utf-8 encoding, keeping the
existing error handling and avoiding safe_read_template_path since
VIEWER_TEMPLATE and FZSTD_JS are internal Path objects, not user-supplied paths.
Source: Coding guidelines
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.
Summary
Carve-out from the agentx branch (
ajc/agentx-on-main): a self-contained HTML viewer of the per-turn INPUT messages a run actually sent, plus the vendored pure-JS zstd decompressor it inflates its embedded payload with client-side.Stacked on #1102 (
ajc/metrics-accumulator-engine). Do not merge before it.aiperf turn-messages <run_dir>reads--export-level rawoutput (raw_records/*.jsonlshards orprofile_export_raw.jsonl) and renders a lazy conversation → turn → message dropdown tree. The payload is interned (each unique(role, content)stored once), zstd-compressed with a 64 MB long-distance window (pinned to fzstd-safewindowLog=26and round-trip self-checked before writing), base64-embedded, and inflated in the browser by the inlinedfzstd.umd.js— no network, no WASM, noDecompressionStream.Files
src/aiperf/analysis/turn_messages.py— raw-record loader, message interning, payload builder, HTML writersrc/aiperf/analysis/turn_messages_viewer.html— viewer template (lazy DOM, filter, roots-only toggle)src/aiperf/analysis/fzstd.umd.js— vendored fzstd v0.1.1 (10,029 bytes; MIT)src/aiperf/cli_commands/turn_messages.py—aiperf turn-messagesCLI commandsrc/aiperf/cli.py— one wiring line (turn-messages only)tests/unit/analysis/test_turn_messages.py— 33 tests: interning, zstd round-trip within the fzstd-safe window, adversarial/malformed-record tolerance, HTML-injection containment, CLI exit codesATTRIBUTIONS.md— fzstd entry onlypyproject.toml—clenadded to codespell ignore-words-list (identifier inturn_messages.py)tools/ruff_baseline.json/tools/ergonomics_baseline.json— baseline entries forbuild_payload(C901 11>10, keyword-only-args) and the CLI entrypoint, same entries the agentx branch carriesdocs/cli-options.md— regenerated viamake generate-cli-docsDeliberate overlap with
ajc/swim-lane-viewerThe sibling swim-lane PR owns
src/aiperf/cli_commands/analyze.pyand theanalyzegroup wiring incli.py. Both PRs add one adjacent line to the command-registration block incli.py, so whichever merges second gets a trivial rebase there. Post-merge follow-up: once the swim-lane PR'sanalyze.pylands, add theaiperf.cli_commands.turn_messages:apphook to theanalyzegroup (nesting this command asaiperf analyze turn-messages) instead of duplicatinganalyze.pyhere.fzstd attribution
fzstd.umd.jsis the vendored UMD build of 101arrowz/fzstd v0.1.1 (MIT, Copyright (c) 2020 Arjun Barrett). The full license text is carried in the file header comment and inATTRIBUTIONS.mdunder "Code Assets". Hygiene: 10,029 bytes (clears the 5000 KBcheck-added-large-filesthreshold), theadd-licensehook has no.jshandler so it is skipped, and the file passes codespell clean with no skip entry needed.Verification
make first-time-setupcleanaiperf.analysis.turn_messages+aiperf.cli_commands.turn_messagesimport, template/decoder assets resolveuv run pytest --collect-only -q tests/unit/— 14309/14396 collected (87 deselected), no collection errorsuv run pytest -n auto tests/unit/analysis/— 171 passedruff format --check+ruff checkclean on all changed.pyfilesmake check-ruff-baselined— OK (120 total, 120 baselined, 0 new)make check-ergonomics— OK (63 total, 63 baselined, 0 new)uv run aiperf turn-messages --helpclean; a 5-turn accumulated-history fixture rendered a 19 KBturn_messages.html; the embedded payload base64+zstd round-trips under awindowLog=26-capped decoder (1 conversations (1 shown) · 5 turns · 30 message refs (10 unique)), fzstd is inlined (.fzstd=f()/fzstd.decompress(), and no template placeholders remainGotcha sweep (#1096 postmortem): every field
turn_messages.pyreads from raw exports (conversation_id,turn_index,agent_depth,parent_correlation_id,x_correlation_id,benchmark_phase,was_cancelled,request_start_ns) verified present onMetricRecordMetadatain this base tree;OutputDefaults.RAW_RECORDS_FOLDER/PROFILE_EXPORT_RAW_JSONL_FILEverified insrc/aiperf/config/artifacts.py; no imports from otheraiperf.analysismodules, so no coupling to the sweepline code from #1102.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
turn-messagescommand that generates a self-contained HTML viewer for per-turn input messages from one or more run directories.Documentation
Chores