Clean up documentation #425
Merged
Merged
Conversation
First pass of the doc-drift cleanup surfaced by the project review. Applies the mechanical rule-1 (source exists → fix doc) and rule-2 (source gone → remove stale section) fixes; non-trivial rewrites are left for a follow-up. - ENDPOINT_CLIENT.md: socket table now matches http.py (SO_KEEPALIVE 1→0 with the connection_lost/eof_received rationale, TCP_KEEPCNT 1→5 marked inert, SO_RCVBUF/SNDBUF 4MB→128KB sliding-window buffers); is_stale() select()→poll() in §8.3/§8.5; appendix loadgen_cores 2→5. - PERF_ARCHITECTURE.md + CLIENT_PERFORMANCE_TUNING.md: loadgen cores → 5 (DEFAULT_LOADGEN_CORES); diagram marked schematic. - core/DESIGN.md: drop nonexistent StreamChunk.is_complete field. - async_utils/DESIGN.md: EventPublisherService is a plain per-instance class (not a SingletonMixin singleton). - event_logger/DESIGN.md: "Not Yet Wired" → publishing is wired in BenchmarkSession. - ready_check_design.md: receiver does NOT close the socket on timeout. - openai/DESIGN.md: decode_sse_message returns SSEChoice (not str); document completions_adapter.py / OpenAITextCompletionsAdapter. - evaluation/DESIGN.md: Files table adds bfcl_v4_*; Scoring Methods registry adds agentic_inference_inline, vbench, bfcl_v4, legacy_mlperf_deepseek_r1. - dataset_manager/DESIGN.md: preset example gpqa::Qwen/Qwen3-8B → gpqa::gptoss. - load_generator/DESIGN.md: delay.py uniform_delay_fn → make_delay_fn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The metrics design docs described a defunct SQLite EventRecorder + duckdb MetricsReporter write/read architecture (and a later KVStore BasicKVStoreReader build_report variant) that no longer exists in the tree. Rewrite both to the current design: - metrics/DESIGN.md: metrics/ is the reporting side only — it consumes the MetricsSnapshot produced by the MetricsAggregatorService subprocess (events PUB → aggregator → final_snapshot.json) and builds the Report via Report.from_snapshot(dict). Documents the data flow, files (report/metric/ early_stopping/results_plots), the Report struct + from_snapshot QPS/TPS window selection, and integration points; cross-links the aggregator spec. - metrics/report_design.md: builder-level reference for report.py — from_snapshot's dict contract, _series_to_metric_dict rollup shape, the complete flag, early-stopping placement, and QPS/TPS window selection. No remaining references to EventRecorder/MetricsReporter/KVStore/SQLite/duckdb. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…shot metrics Fills the three non-trivial gaps in the load-generator design doc flagged by the review: - Agentic benchmarking: add AgenticInferenceStrategy + ConversationManager to the file tree, load-strategy table, and a new component section (response-driven next-turn issuing, concurrency measured in active conversations, turn state via ConversationState, AgenticInferenceConfig knobs, AgenticInferenceDataset shape). Note create_load_strategy raises for AGENTIC_INFERENCE (wired separately). - Drain semantics: reframe draining as the per-phase PhaseConfig.drain_after flag (default True; warmup sets False) rather than a property intrinsic to PhaseType. Add drain_after/drain_timeout/strategy to the PhaseConfig sample. - Metrics integration: replace the removed KVStore/KVStoreReader/from_kv_reader path everywhere (lifecycle, sequence + topology diagrams, integration section, TUI-future section) with the current flow — EventPublisherService → MetricsAggregatorService → MetricsSnapshot PUB + final_snapshot.json → Report.from_snapshot. Fix the "COMPLETE is always published" stale-completions snippet (COMPLETE is suppressed for WARMUP; ERROR precedes COMPLETE). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… default) The audit plan and AGENTS.md described audit phases as unconditionally performance-only, but AuditRunSpec now carries a test_mode field and run_audit selects datasets per phase (perf-only for PERF, perf+accuracy for ACC/BOTH). Document that perf-only is the default and that the test_mode override exists but is currently unused — every registered audit (TEST04) runs perf-only. - compliance_audit_plan.md: mark the perf-only phase-config box with a footnote and expand the step-4 prose to describe the test_mode override. - AGENTS.md: update the compliance-audit rows accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request comprehensively updates the project's documentation to align with recent architectural refactors, including the transition from SQLite/DuckDB-based metrics reporting to a decoupled, aggregator-driven snapshot system (final_snapshot.json), the introduction of the response-driven AgenticInferenceStrategy for multi-turn conversations, and low-latency socket tuning adjustments like using poll() instead of select() for staleness detection. The review feedback correctly identifies a few documentation inconsistencies, specifically pointing out that docs/ENDPOINT_CLIENT.md still references the removed is_complete field and that a code snippet in docs/load_generator/DESIGN.md uses an undefined phase_issuer variable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
nvzhihanj
approved these changes
Jul 20, 2026
arekay-nv
reviewed
Jul 20, 2026
arekay-nv
left a comment
arekay-nv
left a comment
Collaborator
There was a problem hiding this comment.
Review Council — Multi-AI Documentation Review
Reviewed by Codex (gpt-5.5) + Claude + Grok (cursor-grok-4.5-high) | Depth: standard | Focus: documentation
7 inline findings below (high/medium, all doc<->source drift verified against current source). A tiered summary (incl. 2 low-severity items that fall outside the diff hunks) follows as a separate comment.
Collaborator
Review Council — Multi-AI Documentation ReviewReviewed by Codex (gpt-5.5) · Claude · Grok ( 12 raw findings → 9 verified after cross-referencing, dedup, and per-claim verification against current 🔴 Must Fix (high)
🟡 Should Fix (medium)
🔵 Consider (low — not posted inline, outside diff hunks)
One Grok finding was dropped in synthesis as a false positive (report_design L25's ASCII tree correctly describes report-output field names, not snapshot keys — distinct from the genuine L89 issue #4). All comments are |
Reconcile the automated/AI review comments on the docs cleanup: - ENDPOINT_CLIENT.md: drop StreamChunk `is_complete` from the type table + code block (field no longer exists); fix the socket table — TCP_KEEP* are gated behind `if cls.SO_KEEPALIVE` (not "set but inert"), and TCP_USER_TIMEOUT's cell no longer claims keepalive does dead-connection detection. - AGENTS.md: delete the phantom `trtllm/` component row + code-org subtree (no such package; TRT-LLM is served via the openai/videogen adapters). - load_generator/DESIGN.md: agentic turns are pre-built from the dataset — the response gates the *timing* of the next turn, not its prompt; add `phase_issuer = self._current_phase_issuer` to the _handle_response snippet; quote the real AGENTIC_INFERENCE ValueError message. - event_logger/DESIGN.md: session publishes ISSUED (prompt rides its PromptData payload), not a PROMPT event. - metrics/report_design.md: list real snapshot series keys (ttft_ns/tpot_ns/ sample_latency_ns/osl) and their SERIES_TO_SUMMARY_FIELD mapping. - metrics/DESIGN.md: OSL is captured on COMPLETE regardless of streaming, so tps is None only when no tokenizer is configured (or output empty) — not for non-streaming. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary-only items from the PR #425 review council (outside the original diff hunks, so not posted as inline threads): - Drop the dev-history narration + nonexistent `Sample` type in the UUID-generation note; state the current rationale (id published on ISSUED and set as Query.id in the same frozen-struct construction). - Rename the remaining `MetricsAggregator` sequence-diagram participant to `MetricsAggregatorService`, matching the rest of the file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
3 participants
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.
What does this PR do?
Analysis of design documents showed a lot of drift from actual source code. This PR cleans up the docs to match current source code behavior and reconciles conflicting design docs from different versions of features.
Type of change
Related issues
Testing
Checklist