-
Notifications
You must be signed in to change notification settings - Fork 753
docs: update and refactor parent readme #1914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe README.md was extensively restructured and rewritten to improve clarity and conciseness. Navigation and introductory sections were reformatted, redundant instructions and images were removed, a framework support matrix was added, installation and development instructions were streamlined, and new deployment architecture diagrams and descriptions were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant README.md
participant Framework Docs
User->>README.md: Access documentation
README.md-->>User: Show navigation, intro, and architecture
User->>README.md: View framework support matrix
README.md-->>User: Display feature status table
User->>README.md: Follow installation instructions
README.md-->>User: Show pip install snippet and links to framework docs
User->>README.md: Learn about deployment architectures
README.md-->>User: Present diagrams and architecture descriptions
User->>README.md: Set up local development
README.md-->>User: Provide concise build and run commands
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (3)
75-81: Silencemarkdownlint– add a language identifier to fenced blocks
markdownlintis flagging these diagram blocks (MD040).
Adding a dummy language such astext(ormermaidif you migrate to Mermaid later) fixes the lint error without changing rendering:-``` +```text +------+ +-----------+ +------------------+ | HTTP |----->| processor |----->| Worker | | |<-----| |<-----| (Prefill + | +------+ +-----------+ | Decode) | +------------------+ -``` +```Apply the same change to the Disaggregated Serving and KV-Aware Routing blocks.
Also applies to: 89-98, 106-115
155-155: Minor wording – drop “of” after “inside”“Inside of the container” → “inside the container”.
-... we recommend working inside of the container. +... we recommend working inside the container.
24-29: Add accessibility label to navigation<p>blockScreen-reader users have no context for this link group.
Consider addingaria-labelor converting to a list for better accessibility:-<p align="center"> +<p align="center" aria-label="Project navigation links">Optional but improves a11y without visual changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~35-~35: Consider an alternative for the overused word “exactly”.
Context: ... accelerator? This orchestration gap is exactly what NVIDIA Dynamo is built to close. ...
(EXACTLY_PRECISELY)
[style] ~155-~155: This phrase is redundant. Consider using “inside”.
Context: ...o develop locally, we recommend working inside of the container. ```bash # This builds t...
(OUTSIDE_OF)
🪛 markdownlint-cli2 (0.17.2)
README.md
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| | [**Disaggregated Serving**](../../docs/architecture/disagg_serving.md) | ✅ | ✅ | ✅ | | ||
| | [**Conditional Disaggregation**](../../docs/architecture/disagg_serving.md#conditional-disaggregation) | ✅ | 🚧 | 🚧 | | ||
| | [**KV-Aware Routing**](../../docs/architecture/kv_cache_routing.md) | ✅ | ✅ | ✅ | | ||
| | [**SLA-Based Planner**](../../docs/architecture/sla_planner.md) | ✅ | ❌ | ❌ | | ||
| | [**Load Based Planner**](../../docs/architecture/load_planner.md) | ✅ | ❌ | ❌ | | ||
| | [**KVBM**](../../docs/architecture/kvbm_architecture.md) | 🚧 | ❌ | ❌ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken documentation links – ../../ points outside the repository
Because this README lives at the repository root, the prefix ../../ walks two levels above the repo, resulting in 404s for every link in the Framework Support Matrix:
[**Disaggregated Serving**](../../docs/architecture/disagg_serving.md)Update the paths to start with docs/… instead.
-[**Disaggregated Serving**](../../docs/architecture/disagg_serving.md)
+[**Disaggregated Serving**](docs/architecture/disagg_serving.md)Repeat for every entry that currently starts with ../../.
🤖 Prompt for AI Agents
In README.md lines 55 to 60, the documentation links use the prefix ../../ which
points outside the repository and causes 404 errors. Update all these links to
start with docs/ instead of ../../ to correctly reference the files within the
repository.
|
|
||
| | **[Roadmap](https://github.com/ai-dynamo/dynamo/issues/762)** | **[Documentation](https://docs.nvidia.com/dynamo/latest/index.html)** | **[Examples](https://github.com/ai-dynamo/examples)** | **[Design Proposals](https://github.com/ai-dynamo/enhancements)** | | ||
| <p align="center"> | ||
| <a href="https://github.com/ai-dynamo/dynamo/issues/762"><b>Roadmap</b></a> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why " | "?
the whitespace on both ends undermines the the point of the non-breaking whitespace characters.
| ### Introducing NVIDIA Dynamo | ||
|
|
||
| NVIDIA Dynamo is a high-throughput low-latency inference framework designed for serving generative AI and reasoning models in multi-node distributed environments. Dynamo is designed to be inference engine agnostic (supports TRT-LLM, vLLM, SGLang or others) and captures LLM-specific capabilities such as: | ||
| <p align="center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using HTLM so heavily here?
Using HTML in markdown is kind of antithetical to the point of markdown (i.e. utility of reading a plain-text).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it looks a bit nicer. Happy to revert if the reviewers are not a fan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not so much, as it makes it harder to read it the way I read markdown: as plain-text. 😏
I am not the only audience however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you really want to center the text, using the more traditional:
<center>
markdown goes here
</center>
style is easier to read around 😄 tho, to be fair - I've never tested this on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not so much, as it makes it harder to read it the way I read markdown: as plain-text. 😏
I am not the only audience however.
Just curious - how do you view the pictures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser - most mostly just read the alt-text.
|
Overall a big fan. i like the single GPU, multi-GPU, multi-node diagrams since I think it highlights use case of Dynamo at scale, so would prefer to keep those (just centered, similar to the dynamo kv-routing diagram). Next step will likely be to direct Dynamo newcomers to specific examples so they can get up & running. This is where a model/example matrix or smth of the sort could be useful |
Summary by CodeRabbit