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.
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.
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.
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 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.
|
/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.
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated 2 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>
nvzhihanj
added
type: documentation
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 2 comments.
Comments suppressed due to low confidence (1)
docs/async_utils/services/Design.md:22
- The out-of-process subscriber description suggests using a “shared socket directory” like
socket_dir=log_dir.parent, but IPC subscribers must use the publisher’sManagedZMQContext.socket_dir(the directory where the PUB socket was actually bound). If the directory differs,ctx.connect(..., socket_name)will point at a non-existent IPC path. Recommend rewording this to emphasize that the parent process must pass the publisher’s socket_dir to child processes via--socket-dir.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
nit: Can we use all-caps or all-lowercase for |
| @@ -0,0 +1,49 @@ | |||
| # Utils — Design Spec | |||
|
|
|||
| > Shared helpers (logging setup, version, tokenizer utilities) and a standalone HTTP benchmarking tool. The core helper modules have no dependencies on other project subpackages. | |||
There was a problem hiding this comment.
The core helper modules have no dependencies on other project subpackages.
This sounds misleading or misplaced - this sounds like it should be in core/DESIGN.md. Maybe it should say 'Utility classes and functions have no dependencies on other project components'?
|
|
||
| > Async infrastructure shared across the system: uvloop event loop lifecycle management, ZMQ-based IPC transport between processes, and a pub/sub event bus for real-time metric streaming. | ||
|
|
||
| **Component specs:** **async_utils** · [commands](../commands/Design.md) · [config](../config/Design.md) · [core](../core/Design.md) · [dataset_manager](../dataset_manager/Design.md) · [endpoint_client](../endpoint_client/Design.md) · [evaluation](../evaluation/Design.md) · [load_generator](../load_generator/Design.md) · [metrics](../metrics/Design.md) · [openai](../openai/Design.md) · [plugins](../plugins/Design.md) · [profiling](../profiling/Design.md) · [sglang](../sglang/Design.md) · [testing](../testing/Design.md) · [utils](../utils/Design.md) |
There was a problem hiding this comment.
I think this ToC header is cool, but how necessary is this in every file? It's not like a website side-bar where you can click it, if you're reading in a text editor or on GitHub webview you'd still need to scroll up to the top.
There was a problem hiding this comment.
Also makes it hard to maintain and inflates diffs for any changes.
| DataLoaderFactory | ||
| | | ||
| +-- format -> DatafileLoader subclass | ||
| | (jsonl / json / csv / parquet / hf) |
There was a problem hiding this comment.
Alignment here is weird
| ```python | ||
| class DataLoaderFactory: | ||
| @staticmethod | ||
| def create_loader(config: DatasetConfig, num_repeats: int = 1, **kwargs) -> Dataset |
There was a problem hiding this comment.
Inconsistent formatting compared to everywhere else (no ...)
| ┌─────────────────────────────────────────┐ | ||
| │ HTTPEndpointClient │ | ||
| │ ├── uvloop event loop │ | ||
| │ └── WorkerManager │ |
There was a problem hiding this comment.
Can you delete this file? Shouldn't be used anymore.
| Dataset Manager ──► Load Generator ──► Endpoint Client ──► External Endpoint | ||
| │ | ||
| Metrics Collector | ||
| (EventRecorder + MetricsReporter) |
There was a problem hiding this comment.
lest remove class names from top-level README (EventRecorder, MetricsReporter), additional detail not required here.
| --report-dir official_results | ||
| --config submission_template.yaml | ||
| # Note: from-config only accepts --config, --timeout, and --mode via CLI. | ||
| # Set report_dir in the YAML if you need a specific output location. |
There was a problem hiding this comment.
is this a bug?
pl. assign issue to me if so i can patch this quickly
What does this PR do?
Update the docs.
Type of change
Related issues
Testing
Checklist