Repository navigation
docs: describe the tool, not the deployment - #38
Conversation
README rewritten around the three entrypoints, with the manifest format moved to docs/manifest.md and the exporter's input schema to docs/export-plans.md. The exporter no longer defaults to any particular table names; --chains-table and --analytics-table are required when reading from ClickHouse. Claude-Session: https://claude.ai/code/session_01QZQTcscpCQf2JHkdDxLKQR
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af514a7cc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "requests": [batchbench.request_entry( | ||
| {"model": "gpt-4o-mini", "messages": [{"role": "user", "content": "ping"}], "max_tokens": 4}, | ||
| line_idx=0, input_tokens=1)], |
There was a problem hiding this comment.
Supplying requests does not replace the required request_body field: PyBenchmarkConfig deserializes request_body unconditionally before applying the optional request list (rust/src/py_bindings.rs:158-160,183-190,239-241). Anyone copying this new example receives a missing-field error before the dry run starts, so include a request_body entry or make the binding default it.
Useful? React with 👍 / 👎.
| {"schema_version":1,"trajectory_id":"agent-001","requests":[ | ||
| {"prompt_tokens":13381,"output_tokens":288}, | ||
| {"prompt_tokens":14800,"output_tokens":512,"delay_after_ms":25}, | ||
| {"prompt_tokens":7000,"output_tokens":128,"reset_before":true}]} |
There was a problem hiding this comment.
When this documented example is copied into a manifest, it is rejected because the loader deserializes every non-empty physical line as a complete trajectory (rust/src/agent.rs:1478-1485), while both examples here split one object across multiple lines. Put each object on one line, or explicitly label the snippets as pretty-printed JSON that must be compacted before use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The updated README Python example omits the required request_body field for run_benchmark, which makes the documented snippet fail when copied.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reorients BatchBench documentation around the three user-facing entrypoints (independent requests, multi-turn sessions, and manifest replay) and moves detailed format/schema information into dedicated docs pages. It also updates the batchbench-export-plans exporter to require explicit ClickHouse table names (removing hardcoded defaults) and adjusts tests/fixtures accordingly.
Changes:
- Rewrite
README.mdaround the three CLIs and link out to dedicated manifest/exporter docs. - Add
docs/manifest.mdanddocs/export-plans.mdfor the manifest format and exporter input schema. - Make
--chains-tableand--analytics-tablerequired when exporting directly from ClickHouse (not when re-exporting from--rows-jsonl), and update tests to use neutral fixture identifiers.
File summaries
| File | Description |
|---|---|
tests/test_export_plans.py |
Updates fixtures and query tests to use neutral model/upstream/table names matching the new “no defaults” behavior. |
src/batchbench/export_plans.py |
Removes default ClickHouse table constants and enforces explicit table selection when reading from ClickHouse. |
README.md |
Rewrites top-level docs around the three entrypoints and links to the new detailed docs pages. |
docs/manifest.md |
New: specifies replay manifest JSONL format (schema v1 and v2). |
docs/export-plans.md |
New: documents expected ClickHouse input schema and exporter behavior/output. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| report = batchbench.run_benchmark({ | ||
| "endpoint": "https://example.com/v1/chat/completions", | ||
| "user_count": 1, | ||
| "mode": batchbench.finite_mode(requests_per_user=1), | ||
| "requests": [batchbench.request_entry( | ||
| {"model": "gpt-4o-mini", "messages": [{"role": "user", "content": "ping"}], "max_tokens": 4}, | ||
| line_idx=0, input_tokens=1)], | ||
| "dry_run": True, | ||
| }) |
| source.add_argument("--chains-table", help="prompt-chain table or view (see docs/export-plans.md for the expected columns)") | ||
| source.add_argument("--chains-final", action="store_true", help="read the chains table with FINAL (when reading the ReplacingMergeTree base table instead of its deduplicating view)") | ||
| source.add_argument("--analytics-table", default=DEFAULT_ANALYTICS_TABLE, help=f"request analytics table (default {DEFAULT_ANALYTICS_TABLE})") | ||
| source.add_argument("--analytics-table", help="per-request usage table joined on (instance_id, correlation_id)") | ||
| source.add_argument("--analytics-ts-column", default=DEFAULT_ANALYTICS_TS_COLUMN, help=f"timestamp column used to bound the analytics side of the join (default {DEFAULT_ANALYTICS_TS_COLUMN})") |
README rewritten around the three entrypoints: independent requests, multi-turn sessions, and manifest replay. The manifest format moves to docs/manifest.md and the exporter's expected input schema to docs/export-plans.md.
The exporter no longer defaults to any particular table names:
--chains-tableand--analytics-tableare required when reading from ClickHouse (not when re-exporting from--rows-jsonl). Test fixtures use neutral model and upstream names.https://claude.ai/code/session_01QZQTcscpCQf2JHkdDxLKQR