Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a major overhaul of the project's documentation, aiming to provide a more structured, comprehensive, and accessible knowledge base. The changes centralize development guidelines, introduce detailed design specifications for core components, and improve the overall navigation and clarity of the project's technical information. This refactor will significantly aid new contributors in onboarding and help all developers understand the system's architecture and component interactions more deeply. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly updates and expands the project's documentation, introducing detailed design specifications for various components in new Design.md files, revising development setup and contributing guidelines, and clarifying CLI usage. Notable changes include a more comprehensive "Documentation" section in README.md with links to these new design specs, updated development environment setup in DEVELOPMENT.md to reflect new modules and pre-commit hook details, and refined CLI examples in CLI_QUICK_REFERENCE.md and LOCAL_TESTING.md. Review feedback indicates a potential documentation duplication for the "Endpoint client" component, suggesting a need to clarify or deprecate the older document, and also highlights an inconsistency in the CLI_QUICK_REFERENCE.md regarding the --report-dir option for the from-config subcommand, requiring clarification.
| | ----------------- | ---------------------------------------------------------------- | | ||
| | Core types | [docs/core/Design.md](docs/core/Design.md) | | ||
| | Load generator | [docs/load_generator/Design.md](docs/load_generator/Design.md) | | ||
| | Endpoint client | [docs/endpoint_client/Design.md](docs/endpoint_client/Design.md) | |
There was a problem hiding this comment.
This new table of component design specs is a great addition for navigating the project's architecture.
I noticed that a new high-level design doc docs/endpoint_client/Design.md is introduced for the "Endpoint client". However, there's also a very detailed, pre-existing design document at docs/ENDPOINT_CLIENT.md.
Having two design documents for the same component could lead to confusion and maintenance overhead. Is the intention to deprecate or remove docs/ENDPOINT_CLIENT.md in favor of the new structured documentation? If so, it might be good to either remove it as part of this PR or add a note at the top of the old file indicating that it is deprecated and pointing to the new documentation.
There was a problem hiding this comment.
Pull request overview
This PR performs a broad documentation cleanup and restructuring, adding per-component “Design Spec” documents and refreshing contributor and usage guides to reflect the current repository structure and CLI workflows.
Changes:
- Add new component-level design specs under
docs/<component>/Design.mdto describe architecture, responsibilities, and integration points. - Refresh contributor and usage documentation (local testing, development workflow, CLI reference, GitHub setup) to match current commands and repository links.
- Expand/curate example listings and cross-link documentation from the root
README.md.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/README.md | Adds links/descriptions for new example directories. |
| docs/utils/Design.md | New utils design spec (currently contains several API/behavior mismatches vs code). |
| docs/testing/Design.md | New testing utilities design spec (echo/max/variable throughput servers). |
| docs/sglang/Design.md | New SGLang adapter design spec (currently documents non-existent adapter/accumulator methods). |
| docs/profiling/Design.md | New profiling design spec (line_profiler + pytest flag). |
| docs/plugins/Design.md | New plugin namespace design spec (currently overstates existing plugin registration API). |
| docs/openai/Design.md | New OpenAI adapter design spec (currently documents non-existent adapter/accumulator methods). |
| docs/metrics/Design.md | New metrics design spec (EventRecorder API signature section currently mismatches implementation). |
| docs/load_generator/Design.md | New load generator design spec (session/scheduler architecture). |
| docs/evaluation/Design.md | New evaluation design spec (accuracy scoring + LiveCodeBench). |
| docs/endpoint_client/Design.md | New endpoint client design spec (worker pool, ZMQ IPC, adapters). |
| docs/dataset_manager/Design.md | New dataset manager design spec (loader/transforms/presets; format inference list currently incomplete). |
| docs/core/Design.md | New core types design spec (currently mismatches actual Query/QueryResult struct fields). |
| docs/config/Design.md | New config design spec (YAML/CLI → RuntimeSettings, templates, rulesets). |
| docs/commands/Design.md | New commands layer design spec (CLI layout and command flow). |
| docs/async_utils/services/metrics_aggregator/Design.md | Adds a clearer one-line summary of the metrics aggregator service. |
| docs/async_utils/services/event_logger/Design.md | Adds a clearer one-line summary of the event logger service. |
| docs/async_utils/services/Design.md | Adds a clearer one-line summary of the pub/sub system design doc. |
| docs/async_utils/Design.md | New async_utils design spec (loop manager, ZMQ transport, pub/sub services). |
| docs/LOCAL_TESTING.md | Updates local testing instructions (dataset path, init syntax, default duration, supported formats list). |
| docs/GITHUB_SETUP.md | Updates GitHub workflow descriptions and branch protection checklist. |
| docs/ENDPOINT_CLIENT.md | Adds a short introductory summary line for the document. |
| docs/DEVELOPMENT.md | Major rewrite of development workflow guide (fork/upstream flow, tooling, formatting, links). |
| docs/CLI_QUICK_REFERENCE.md | Replaces intro text and adjusts examples/wording for config-driven usage. |
| README.md | Updates docs links, architecture diagram, LiveCodeBench link, and minor command snippets. |
| CONTRIBUTING.md | Adds pointer to docs/DEVELOPMENT.md for standards. |
| AGENTS.md | Simplifies setup section and updates repo structure/tooling references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class EventRecorder: | ||
| LIVE: "EventRecorder | None" # class variable; set on construction, cleared on close | ||
|
|
||
| def record_event( | ||
| self, | ||
| sample_uuid: str, | ||
| event_type: SampleEvent | SessionEvent, | ||
| data: dict | None = None, | ||
| ) -> None | ||
|
|
||
| def wait_for_writes(self, force_commit: bool = True) -> None | ||
| # Blocks until the background writer thread has flushed all queued events | ||
|
|
||
| @staticmethod | ||
| def db_path(session_id: str) -> Path | ||
| ``` |
There was a problem hiding this comment.
The EventRecorder public interface in this doc doesn’t match the implementation. In metrics/recorder.py, record_event is a @classmethod with signature roughly (ev_type, timestamp_ns, sample_uuid=..., data=..., ...), not an instance method taking (sample_uuid, event_type, data). Please update the signature/parameter descriptions to avoid misleading callers.
| ### `Query` | ||
|
|
||
| Represents a single inference request issued to an endpoint. | ||
|
|
||
| | Field | Type | Description | | ||
| | ------------ | ------------- | ------------------------------------------ | | ||
| | `id` | `str` | UUID hex (32 chars) for result correlation | | ||
| | `prompt` | `str` | Input text | | ||
| | `model` | `str` | Model identifier | | ||
| | `max_tokens` | `int \| None` | Token cap override | | ||
| | `stream` | `bool` | Whether to request streaming | | ||
|
|
||
| ### `QueryResult` | ||
|
|
||
| Represents a completed (success or failure) inference response. | ||
|
|
||
| | Field | Type | Description | | ||
| | ------------ | ------------------------- | -------------------------------- | | ||
| | `id` | `str` | Matches originating `Query.id` | | ||
| | `output` | `TextModelOutput \| None` | Response content | | ||
| | `error` | `str \| None` | Error message on failure | | ||
| | `ttft_ns` | `int \| None` | Time to first token, nanoseconds | | ||
| | `latency_ns` | `int` | End-to-end latency, nanoseconds | | ||
|
|
There was a problem hiding this comment.
The Query/QueryResult field tables in this doc do not reflect the actual structs in core/types.py. For example, Query is {id, data, headers, created_at} (not prompt/model/max_tokens/stream), and QueryResult fields are {id, response_output, metadata, error, completed_at}. Please update the tables (and any examples) to match the current types so downstream component docs stay consistent.
arekay-nv
force-pushed
the
arekay/doc-refactor
branch
from
4ddf686 to
c5e2ed6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
consider moving in ENDPOINT_CLIENT.md into this folder as the main design document,
and then consolidate Design.md to usage / args only?
(also CLIENT_PERFORMANCE_TUNING.md?, PERF_ARCHITECTURE.md?)
nvzhihanj
left a comment
nvzhihanj
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Claude (Codex unavailable) | Depth: thorough
Found 9 issues across 5 files.
Note: 17 existing inline comments were already present. Overlapping issues excluded.
Review Council — Multi-AI Code ReviewReviewed by: Claude (Codex unavailable — branch checkout failed) | Depth: thorough Found 9 issues across 5 files. All verified against actual source code. 17 existing inline comments from a prior review were already present. Overlapping issues have been excluded from this review. 🔴 Must Fix (high) — 3 issuesIssues where the documentation will actively mislead readers about how the code works.
🟡 Should Fix (medium) — 4 issuesReal inaccuracies that could cause confusion under specific circumstances.
🔵 Consider (low) — 2 issuesValid improvements that could be follow-ups.
🤖 Generated with Claude Code — Review Council |
arekay-nv
force-pushed
the
arekay/doc-refactor
branch
from
0db4683 to
7196894
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add Design.md specs for all 15 top-level components under src/inference_endpoint/ - Restructure AGENTS.md: move code style details to DEVELOPMENT.md, update component table with runner.py and async_utils services - Update README.md: add Component Design Specs table, use python3 in examples - Reformat DEVELOPMENT.md: remove emojis, add commit type list, exact-version pinning guidance - Update CLI_QUICK_REFERENCE.md, LOCAL_TESTING.md, ENDPOINT_CLIENT.md, GITHUB_SETUP.md for consistency - Fix stale references: pkl→jsonl throughout, CLIError for eval mode, dataset_manager Design.md reflects current supported formats Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
arekay-nv
force-pushed
the
arekay/doc-refactor
branch
from
465f559 to
9a7697b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up reviewAll 9 issues from the prior review have been addressed. Verified each against the current source code: High (must fix) — all 3 fixed:
Medium (should fix) — all 4 fixed:
Low (consider) — both fixed:
The fix commits ( No issues found in the current state of the PR. 🤖 Generated with Claude Code If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add msgspec-patterns and msgspec-struct-gc-check Claude/Cursor skills - Add design docs for services architecture and metrics aggregator - Update .gitignore for new artifact patterns - Update zmq_pubsub_demo.py for new transport API
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
docs/async_utils/services/Design.md:21
- This section says in-process subscribers “connect to the publisher’s
bind_address”. With the current ZMQ helpers,ZmqEventRecordSubscriberexpects the socket name/path (e.g.publisher.bind_path/ev_pub_<uuid>), not the fullipc://...bind address; passingbind_addresswould result in an invalid constructed address becauseManagedZMQContext.connect()prependssocket_diragain. Please update the docs to reference the socket name (bind_path) for in-process subscribers, and reservebind_addressfor cases where subscribers connect withoutManagedZMQContextaddress construction.
docs/async_utils/services/Design.md:306 - The
event_loggerlifecycle description claims error events aftersession.endedare still written. The currentEventLoggerService.process()stops writing all subsequent records once_shutdown_receivedis set, regardless of event type (src/inference_endpoint/async_utils/services/event_logger/__main__.py,process()method). Either update this doc to match current behavior, or adjustEventLoggerServiceto continue writingErrorEventType.*records after ENDED if that behavior is intended.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
docs/async_utils/services/Design.md:22
- This section says in-process subscribers connect using the publisher's
bind_address, but the ZMQ APIs take the socket name (publisherbind_path, e.g.ev_pub_<uuid>) andManagedZMQContext.connect()constructs the fullipc://...address. Usingbind_addressdirectly would not work withconnect()as implemented. Suggest updating wording to match the actual API (path=publisher.bind_path).
docs/async_utils/services/Design.md:306 - Doc/code mismatch: this claims the event logger continues writing error events after
session.ended, butEventLoggerService.process()currently drops all subsequent records once_shutdown_receivedis set (seesrc/inference_endpoint/async_utils/services/event_logger/__main__.py, where itcontinues when_shutdown_received). Either update the implementation to allowErrorEventTypethrough after ENDED, or adjust this doc to reflect current behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EventRecord( | ||
| event_type=SampleEventType.COMPLETE, | ||
| timestamp_ns=10211, | ||
| sample_uuid=uuid1, | ||
| data={"tokens": 50}, | ||
| data="Hello world", | ||
| ), | ||
| EventRecord( | ||
| event_type=SampleEventType.RECV_NON_FIRST, | ||
| timestamp_ns=10214, | ||
| sample_uuid=uuid2, | ||
| ), | ||
| EventRecord( | ||
| event_type=SampleEventType.RECV_NON_FIRST, | ||
| timestamp_ns=10217, | ||
| sample_uuid=uuid2, | ||
| ), | ||
| EventRecord( | ||
| event_type=SampleEventType.RECV_NON_FIRST, | ||
| timestamp_ns=10219, | ||
| sample_uuid=uuid2, | ||
| ), | ||
| EventRecord( | ||
| event_type=SampleEventType.COMPLETE, | ||
| timestamp_ns=10219, | ||
| sample_uuid=uuid2, | ||
| data={"tokens": 75}, | ||
| data="Sample output for uuid2", | ||
| ), |
There was a problem hiding this comment.
EventRecord.data is typed as TextModelOutput | PromptData | ErrorData | None (see core.record.EventRecord), so publishing a plain str here will fail msgspec validation/encoding at runtime. Wrap outputs in TextModelOutput(output=...) (or set data=None if unused) so the demo can actually publish COMPLETE events.
| # Start event_logger as a subprocess (logs to both JSONL and SQL under event_log_dir). | ||
| event_logger_cmd = [ | ||
| sys.executable, | ||
| "-m", | ||
| "inference_endpoint.async_utils.services.event_logger", | ||
| "--log-dir", | ||
| str(event_log_dir), | ||
| "--socket-dir", | ||
| str(zmq_ctx.socket_dir), | ||
| "--socket-name", | ||
| str(connect_path), | ||
| "--writers", | ||
| "jsonl", | ||
| "sql", | ||
| ] |
There was a problem hiding this comment.
The demo always starts event_logger with --writers jsonl sql, but the SQL writer is only available when sqlalchemy is installed (the service omits the sql choice otherwise and will exit immediately with an argparse error). Consider detecting sqlalchemy availability in this script (or falling back to JSONL-only) so the demo works with a base install.
| try: | ||
| event_logger_proc.wait(timeout=5) | ||
| except subprocess.TimeoutExpired: | ||
| logger.warning("event_logger did not exit within 5s, terminating") | ||
| event_logger_proc.terminate() | ||
| event_logger_proc.wait(timeout=2) | ||
| except KeyboardInterrupt: | ||
| logger.info("Interrupted by user") | ||
| finally: | ||
| logger.info("Cleaning up (closing subscribers)...") | ||
| console_sub.close() | ||
| file_sub.close() | ||
| duration_sub.close() | ||
| if event_logger_proc.poll() is None: | ||
| event_logger_proc.terminate() | ||
| event_logger_proc.wait(timeout=2) | ||
|
|
There was a problem hiding this comment.
Process termination handling can raise subprocess.TimeoutExpired and break cleanup: after terminate(), both event_logger_proc.wait(timeout=2) calls are unguarded. If the subprocess ignores SIGTERM (or is stuck), the demo will raise and exit before closing subscribers / printing verification. Catch the second timeout and escalate to kill() (and/or continue cleanup) to keep teardown reliable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly expands the project's documentation by adding detailed design specifications for all core components, including the load generator, metrics aggregator, and endpoint client. It also introduces new AI agent skills for msgspec optimization and garbage collection safety. Review feedback identified a missing required argument in a documentation code example and a missing field in the metrics aggregator's data model description.
| query = Query(prompt="Test", model="test-model") | ||
| assert query.prompt == "Test" | ||
| assert query.model == "test-model" | ||
| query = Query(data={"prompt": "Test", "model": "test-model"}) |
There was a problem hiding this comment.
The Query instantiation in this example is missing the headers argument. According to the core types design specification in docs/core/Design.md, headers is a required field of type dict[str, str]. Omitting it will result in a TypeError during execution.
| query = Query(data={"prompt": "Test", "model": "test-model"}) | |
| query = Query(data={"prompt": "Test", "model": "test-model"}, headers={}) |
| SampleRow | ||
| ├── sample_uuid: str | ||
| ├── issued_ns: int | None ← set on ISSUED | ||
| ├── complete_ns: int | None ← set on COMPLETE | ||
| ├── recv_first_ns: int | None ← set on RECV_FIRST | ||
| ├── last_recv_ns: int | None ← set on RECV_FIRST, updated on each RECV_NON_FIRST | ||
| ├── client_send_ns: int | None ← set on CLIENT_SEND | ||
| ├── client_resp_done_ns: int | None ← set on CLIENT_RESP_DONE | ||
| ├── prompt_text: str | None ← from ISSUED event data (for ISL tokenization) | ||
| ├── first_chunk_text: str | None ← from RECV_FIRST event data (for TPOT denominator) | ||
| └── output_chunks: list[str] ← accumulated from RECV_FIRST/RECV_NON_FIRST data |
There was a problem hiding this comment.
The SampleRow struct definition is missing the first_chunk_tokens (or first_chunk_token_count) field. This field is required for the tpot_ns calculation described in the formula on line 143 and used in the event dispatch logic on line 184. Without storing this count, the aggregator cannot correctly compute the TPOT denominator for streaming responses.
| SampleRow | |
| ├── sample_uuid: str | |
| ├── issued_ns: int | None ← set on ISSUED | |
| ├── complete_ns: int | None ← set on COMPLETE | |
| ├── recv_first_ns: int | None ← set on RECV_FIRST | |
| ├── last_recv_ns: int | None ← set on RECV_FIRST, updated on each RECV_NON_FIRST | |
| ├── client_send_ns: int | None ← set on CLIENT_SEND | |
| ├── client_resp_done_ns: int | None ← set on CLIENT_RESP_DONE | |
| ├── prompt_text: str | None ← from ISSUED event data (for ISL tokenization) | |
| ├── first_chunk_text: str | None ← from RECV_FIRST event data (for TPOT denominator) | |
| └── output_chunks: list[str] ← accumulated from RECV_FIRST/RECV_NON_FIRST data | |
| SampleRow | |
| ├── sample_uuid: str | |
| ├── issued_ns: int | None ← set on ISSUED | |
| ├── complete_ns: int | None ← set on COMPLETE | |
| ├── recv_first_ns: int | None ← set on RECV_FIRST | |
| ├── last_recv_ns: int | None ← set on RECV_FIRST, updated on each RECV_NON_FIRST | |
| ├── client_send_ns: int | None ← set on CLIENT_SEND | |
| ├── client_resp_done_ns: int | None ← set on CLIENT_RESP_DONE | |
| ├── prompt_text: str | None ← from ISSUED event data (for ISL tokenization) | |
| ├── first_chunk_text: str | None ← from RECV_FIRST event data (for TPOT denominator) | |
| ├── first_chunk_tokens: int | None ← token count of first_chunk_text | |
| └── output_chunks: list[str] ← accumulated from RECV_FIRST/RECV_NON_FIRST data |
What does this PR do?
Update the docs.
Type of change
Related issues
Testing
Checklist