fix: client wheel with a README and valid SPDX identifier#160
fix: client wheel with a README and valid SPDX identifier#160saturley-hall wants to merge 3 commits intomainfrom
Conversation
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
WalkthroughDocumentation and configuration updates for the Python ModelExpress client package, including a new comprehensive README file, .gitignore rule addition for Python packaging artifacts, and project metadata changes in pyproject.toml. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelexpress_client/python/README.md`:
- Around line 21-22: The README currently links to the local repo using a
relative URL "../../modelexpress_server/" which won't resolve on PyPI; update
the link in modelexpress_client/python/README.md to point to the canonical
GitHub repository URL for the ModelExpress server (replace
"../../modelexpress_server/" with the full GitHub HTTPS URL for the
modelexpress_server repo) so the reference renders correctly outside the
monorepo.
- Around line 7-15: Update the Installation section of README.md to lead with
the PyPI install command for end users (e.g., "pip install modelexpress"), then
add a separate "Development" subsection that contains the editable/source
install instructions and the dev-dependencies install (the current "pip install
-e ." and "pip install -e '.[dev]'" lines); ensure the README still includes the
dev commands but they are moved under the "Development" heading so the top-level
install is the PyPI command users see first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea92b9f0-e6fc-445b-8069-8fbd5b2a2b12
📒 Files selected for processing (3)
modelexpress_client/python/.gitignoremodelexpress_client/python/README.mdmodelexpress_client/python/pyproject.toml
| ## Installation | ||
|
|
||
| ```bash | ||
| # From source | ||
| pip install -e . | ||
|
|
||
| # With dev dependencies (pytest, grpcio-tools) | ||
| pip install -e ".[dev]" | ||
| ``` |
There was a problem hiding this comment.
Lead with the PyPI install command.
This README is the package description users will see on PyPI, but the only commands here assume a local repo checkout. pip install -e . and pip install -e ".[dev]" are useful for contributors, not end users installing the published package.
Suggested README tweak
## Installation
```bash
-# From source
-pip install -e .
+pip install modelexpress
+```
+
+### Development
+
+```bash
+# From source
+pip install -e .
# With dev dependencies (pytest, grpcio-tools)
pip install -e ".[dev]"</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
## Installation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelexpress_client/python/README.md` around lines 7 - 15, Update the
Installation section of README.md to lead with the PyPI install command for end
users (e.g., "pip install modelexpress"), then add a separate "Development"
subsection that contains the editable/source install instructions and the
dev-dependencies install (the current "pip install -e ." and "pip install -e
'.[dev]'" lines); ensure the README still includes the dev commands but they are
moved under the "Development" heading so the top-level install is the PyPI
command users see first.
nv-hwoo
left a comment
nv-hwoo
left a comment
There was a problem hiding this comment.
Looks good overall, and I left one small comment. Thanks for adding the readme!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Harrison Saturley-Hall <harrison.saturley.hall@gmail.com>
AndyDai-nv
left a comment
AndyDai-nv
left a comment
There was a problem hiding this comment.
Overall LGTM, just one small nit
modelexpress_client/python/README.md
Outdated
|
|
||
| vllm serve deepseek-ai/DeepSeek-V3 \ | ||
| --load-format mx-source \ | ||
| --tensor-parallel-size 8 |
There was a problem hiding this comment.
I think we need to specify --worker-cls modelexpress.vllm_worker.ModelExpressWorker? Correct me if I am wrong here. @nv-hwoo @KavinKrishnan
There was a problem hiding this comment.
Ah good catch, yes, you are correct :)
|
|
||
| ```bash | ||
| # From source | ||
| pip install -e . |
There was a problem hiding this comment.
Should we also add pip install from PyPI?
| pip install -e . | |
| # From PyPI | |
| pip install modelexpress | |
| # From source | |
| pip install -e . |
- Lead with PyPI install command, move source installs to Development subsection - Add --worker-cls modelexpress.vllm_worker.ModelExpressWorker to vLLM examples
3 participants
The current pyproject.toml misses a couple things:
Since this is a published artifact it will need to be cherry-picked into the
release/0.3.0branch.Summary by CodeRabbit
Documentation
Chores