docs: overhaul AI instructions and extract reference documentation#146
Open
nicolasnoble wants to merge 4 commits intomainfrom
Open
docs: overhaul AI instructions and extract reference documentation#146nicolasnoble wants to merge 4 commits intomainfrom
nicolasnoble wants to merge 4 commits intomainfrom
Conversation
Consolidated the duplicated project context across CLAUDE.md, Copilot instructions, and Cursor rules into concise pointers to new standalone reference docs. Extracted architecture details into docs/ARCHITECTURE.md and deployment/configuration info into docs/DEPLOYMENT.md. Updated CONTRIBUTING.md, README.md, and example READMEs to match the new structure. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
WalkthroughThis pull request consolidates ModelExpress architectural documentation from multiple instruction files into dedicated reference documents (ARCHITECTURE.md and DEPLOYMENT.md), while reorienting instruction files toward development workflows and contributor guidelines. The primary documentation hub (README.md) is simplified to reference external docs, and CONTRIBUTING.md is expanded with explicit setup, command, and environment variable information. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
CLAUDE.md (1)
105-105: Style nit: "not complete" → "incomplete" (applies to all three agent files).The closing directive in the Documentation Updates table reads identically in
CLAUDE.md,.github/copilot-instructions.md, and.cursor/rules/rust.mdc. A slightly more concise form:-**A feature is not complete until documentation is updated.** +**A feature is incomplete until documentation is updated.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 105, Replace the phrase "not complete" with the more concise "incomplete" in the closing directive found in the Documentation Updates table across the three agent files (the CLAUDE.md entry and the identical directives in .github/copilot-instructions.md and .cursor/rules/rust.mdc); locate the table row that contains the closing directive text (the sentence that currently reads "**A feature is not complete until documentation is updated.**") and update it to "**A feature is incomplete until documentation is updated.**"..cursor/rules/rust.mdc (1)
84-92: Minor wording inconsistency withCLAUDE.mdin Tips section.
CLAUDE.mdline 81 reads "Always read files to understand context before making changes." — both this file and.github/copilot-instructions.mddrop the "Always". Since the three agent files are meant to stay in sync, align the wording.✏️ Suggested fix
- - Read files to understand context before making changes. + - Always read files to understand context before making changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cursor/rules/rust.mdc around lines 84 - 92, Update the "## Tips" section so the first bullet matches CLAUDE.md's wording by prefixing it with "Always" (i.e., change "Read files to understand context before making changes." to "Always read files to understand context before making changes."); locate the "## Tips" header and the first bullet in .cursor/rules/rust.mdc and make the wording consistent with CLAUDE.md and .github/copilot-instructions.md.README.md (1)
108-112: Minor: three consecutive sentences opening with "For".Static analysis flagged this. Consider varying the lead-in on at least one of the three entries to improve readability, e.g.:
✏️ Suggested rewrite
-For Helm-based deployment, see [`helm/README.md`](helm/README.md). +Helm-based deployment details are in [`helm/README.md`](helm/README.md). For launching ModelExpress with Dynamo on Kubernetes, see the [aggregated K8s example](examples/aggregated_k8s/). -For GPU-to-GPU P2P weight transfers, see the [P2P transfer example](examples/p2p_transfer_k8s/). +GPU-to-GPU P2P weight transfer instructions are in the [P2P transfer example](examples/p2p_transfer_k8s/).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 108 - 112, Three consecutive lines in the README start with "For" (the entries linking to "helm/README.md", "aggregated K8s example", and "P2P transfer example"); reword at least one of them to avoid repetitive lead-ins—e.g., change one to "See", "Use", or "To launch" and ensure the link text remains unchanged so navigation still works; update the sentence(s) near the existing link lines (the helm, aggregated_k8s, and p2p_transfer_k8s entries) to improve readability while preserving links and meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 160-165: Replace the incorrect phrase "The Dynamo project" with
"The ModelExpress project" in the CONTRIBUTING.md paragraph that currently reads
"The Dynamo project is spread across multiple repositories. The team will
provide guidance about how and where your enhancement should be implemented."
Update that sentence to say "The ModelExpress project is spread across multiple
repositories..." so the repository reference is accurate.
In `@docs/ARCHITECTURE.md`:
- Around line 169-171: The docs directory tree in the README-like listing is
incomplete: it only shows CLI.md but the PR adds ARCHITECTURE.md and
DEPLOYMENT.md; update the tree block that lists docs to include both new files
(ARCHITECTURE.md and DEPLOYMENT.md) under docs/ so the repository structure
accurately reflects the new files.
- Around line 255-276: The ServerConfig YAML sample is missing the
eviction.policy field min_free_space_bytes; update the YAML block in
ARCHITECTURE.md by adding "min_free_space_bytes: null" under eviction.policy
(next to unused_threshold and max_models) and include the corresponding
env-comment key MODEL_EXPRESS_CACHE_EVICTION_MIN_FREE_SPACE_BYTES so the example
matches the schema shown elsewhere; ensure placement and indentation match the
existing eviction.policy entries.
In `@docs/DEPLOYMENT.md`:
- Around line 220-244: Add the missing REDIS_URL entry to the P2P Environment
Variables table so operators know to configure server-side metadata
coordination; specifically add a row with variable name `REDIS_URL`, default
`redis://localhost:6379`, and description like "Redis server URL for P2P
metadata coordination (required for leader election and transfer metadata)" near
the existing entries (e.g., alongside `MODEL_EXPRESS_URL` and
`MX_REGISTER_LOADERS`) to keep the table complete and consistent with
CONTRIBUTING.md and examples.
---
Duplicate comments:
In @.github/copilot-instructions.md:
- Around line 84-92: Update the inconsistent wording in the Tips list by
changing the bullet "- Read files to understand context before making changes."
to match CLAUDE.md's phrasing "Always read files to understand context before
making changes."; also ensure the same phrasing is applied in the other
referenced file(s) where the comment noted divergence (the entry in
.cursor/rules/rust.mdc and rust.mdc) so all three places use the exact "Always
read files..." wording for consistency.
---
Nitpick comments:
In @.cursor/rules/rust.mdc:
- Around line 84-92: Update the "## Tips" section so the first bullet matches
CLAUDE.md's wording by prefixing it with "Always" (i.e., change "Read files to
understand context before making changes." to "Always read files to understand
context before making changes."); locate the "## Tips" header and the first
bullet in .cursor/rules/rust.mdc and make the wording consistent with CLAUDE.md
and .github/copilot-instructions.md.
In `@CLAUDE.md`:
- Line 105: Replace the phrase "not complete" with the more concise "incomplete"
in the closing directive found in the Documentation Updates table across the
three agent files (the CLAUDE.md entry and the identical directives in
.github/copilot-instructions.md and .cursor/rules/rust.mdc); locate the table
row that contains the closing directive text (the sentence that currently reads
"**A feature is not complete until documentation is updated.**") and update it
to "**A feature is incomplete until documentation is updated.**".
In `@README.md`:
- Around line 108-112: Three consecutive lines in the README start with "For"
(the entries linking to "helm/README.md", "aggregated K8s example", and "P2P
transfer example"); reword at least one of them to avoid repetitive
lead-ins—e.g., change one to "See", "Use", or "To launch" and ensure the link
text remains unchanged so navigation still works; update the sentence(s) near
the existing link lines (the helm, aggregated_k8s, and p2p_transfer_k8s entries)
to improve readability while preserving links and meaning.
- Fix copy-paste artifact in CONTRIBUTING.md ("The Dynamo project" -> "The ModelExpress project")
- Add ARCHITECTURE.md and DEPLOYMENT.md to the docs/ directory tree in ARCHITECTURE.md
- Add missing min_free_space_bytes field to the ServerConfig YAML example in ARCHITECTURE.md to match DEPLOYMENT.md and the actual config struct
- Add REDIS_URL to the P2P environment variables table in DEPLOYMENT.md, consistent with CONTRIBUTING.md and the K8s examples
Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The "Enable P2P Transfers via NIXL" PR (#135) introduced significant new functionality without updating all documentation files per the project's documentation rules. This commit closes the remaining gaps: ARCHITECTURE.md: - Fix __init__.py description (no longer claims auto-registration on import) - Add --worker-cls usage to vllm_worker.py description - Add check_session_changed() and close() to MxClient methods table - Fix NixlTransferManager: correct __init__ params, add get_registered_descriptors(), fix destroy() -> shutdown(), update receive_from_source signature - Correct MxTargetModelLoader base class to DummyModelLoader - Document transfer-time coalescing of contiguous regions - Document _raw_tensor_registry and _nixl_managers globals - Add MODEL_NAME and MX_SERVER_ADDRESS environment variables DEPLOYMENT.md: - Add MODEL_NAME and MX_SERVER_ADDRESS to P2P env var table - Add --worker-cls usage note for vLLM instances CONTRIBUTING.md: - Add Python 3.10+ to prerequisites - Add pip install -e for Python client dev setup - Add pytest and generate_proto.sh to Available Commands table Also includes pre-commit auto-fixes: trailing whitespace cleanup and missing final newlines in several files. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
- "not complete" -> "incomplete" in CLAUDE.md, copilot-instructions.md, and rust.mdc - "Read files" -> "Always read files" in copilot-instructions.md and rust.mdc to match CLAUDE.md - Vary repeated "For" sentence openers in README.md Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
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
1 participant
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.
Consolidated the duplicated project context across CLAUDE.md, Copilot instructions, and Cursor rules into pointers to new standalone reference docs. Extracted architecture details into docs/ARCHITECTURE.md and deployment/configuration info into docs/DEPLOYMENT.md. Updated CONTRIBUTING.md, README.md, and example READMEs to match the new structure, and have a self-updating, cascading set of instructions.
Summary by CodeRabbit
Release Notes