feat(dynamo-run): Default to building with CUDA #1295
Conversation
grahamking
requested review from
a team,
GuanLuo,
biswapanda,
kkranen,
oandreeva-nv,
paulhendricks,
rmccorm4,
ryanolson and
tmonty12
as code owners
WalkthroughThe documentation for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docs
participant Cargo
User->>Docs: Reads build instructions
Docs->>User: Describes default engines and build features (CUDA, mistralrs, llamacpp)
User->>Cargo: Runs cargo build (default)
Cargo->>User: Builds with cuda, mistralrs, llamacpp features enabled
Possibly related PRs
Poem
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/guides/dynamo_run.md (3)
38-38: Fix grammar: use past participle “built” and add missing period for “etc.”- `dynamo-run` is build for CUDA by default. For other builds (CPU, Metal, etc) see the [Setup section](#setup). + `dynamo-run` is built for CUDA by default. For other builds (CPU, Metal, etc.) see the [Setup section](#setup).🧰 Tools
🪛 LanguageTool
[grammar] ~38-~38: Make sure that the noun ‘build’ is correct. Did you mean the past participle “built”?
Context: ... syntax asRUST_LOG.dynamo-runis build for CUDA by default. For other builds (...(BE_VB_OR_NN)
[style] ~38-~38: In American English, abbreviations like “etc.” require a period.
Context: ... default. For other builds (CPU, Metal, etc) see the Setup section. ## Q...(ETC_PERIOD)
46-46: Unify engine naming for consistency.
The text usesmistral.rsandllama.cpp, but elsewhere engine names aremistralrsandllamacpp. For consistency across documentation, consider aligning these identifiers.
259-283: Add language identifiers to fenced code blocks and standardize list markers.
To satisfy markdownlint checks (MD040 & MD004), consider:
- Replacing dashes
-with asterisks*for list items.- Adding a language label (e.g.,
bashorshell) to all fenced code blocks, for example:- ``` + ```bash cargo build - ``` + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
259-259: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
260-260: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
270-270: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
271-271: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
275-275: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
276-276: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
280-280: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
281-281: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/guides/dynamo_run.md(3 hunks)launch/dynamo-run/Cargo.toml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/guides/dynamo_run.md
[grammar] ~38-~38: Make sure that the noun ‘build’ is correct. Did you mean the past participle “built”?
Context: ... syntax as RUST_LOG. dynamo-run is build for CUDA by default. For other builds (...
(BE_VB_OR_NN)
[style] ~38-~38: In American English, abbreviations like “etc.” require a period.
Context: ... default. For other builds (CPU, Metal, etc) see the Setup section. ## Q...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
docs/guides/dynamo_run.md
259-259: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
260-260: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
270-270: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
271-271: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
275-275: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
276-276: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
280-280: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
281-281: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/bindings/python)
🔇 Additional comments (4)
launch/dynamo-run/Cargo.toml (3)
17-17: Default features updated to include CUDA.
Enablingcudain the default features matches the PR goal of default GPU support fordynamo-run.
17-17: Confirm CI readiness for default CUDA build.
By addingcudato the default feature set,cargo buildwill now attempt to compile CUDA code out-of-the-box. Please verify that CI environments and developer machines have the necessary CUDA toolkit and environment variables (e.g.,CUDA_HOME) installed, or update CI configurations/documentation accordingly to avoid build failures.
18-19: Validatedep:prefix usage in feature definitions.
The featuresmistralrsandllamacppreference dependencies using adep:prefix which is non-standard in Cargo. Ensure this syntax is supported by your workspace tooling, or consider switching to the canonical form:-mistralrs = ["dep:dynamo-engine-mistralrs"] +mistralrs = ["dynamo-engine-mistralrs"] -llamacpp = ["dep:dynamo-engine-llamacpp"] +llamacpp = ["dynamo-engine-llamacpp"]docs/guides/dynamo_run.md (1)
27-27: Update supported engines list.
The documentation now correctly lists the supported engines includingsglangandtensorrt-llm.
|
See #1217 for next steps. The docker base image needs to be include the CUDA toolkit. |
|
|
||
| Set the environment variable `DYN_LOG` to adjust the logging level; for example, `export DYN_LOG=debug`. It has the same syntax as `RUST_LOG`. | ||
|
|
||
| `dynamo-run` is build for CUDA by default. For other builds (CPU, Metal, etc) see the [Setup section](#setup). |
There was a problem hiding this comment.
| `dynamo-run` is build for CUDA by default. For other builds (CPU, Metal, etc) see the [Setup section](#setup). | |
| `dynamo-run` is built with CUDA support by default. For other builds (CPU, Metal, etc) see the [Setup section](#setup). |
There was a problem hiding this comment.
question: the build will support CPU as well as CUDA - right? maybe dynamo is built with CPU and GPU support by default ...
There was a problem hiding this comment.
Currently we have to decide that at build time. We could (probably should) release two versions, one for CPU and CUDA.
It would be valuable to investigate doing a multi-build that can support either, and detect at runtime. Need engineering time there though, so many other priorities.
| which is equivalent to | ||
| ``` | ||
| cargo build --features cuda,mistralrs,llamacpp |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
Make
cargo buildtarget CUDA. Update docs to show how to do non-CUDA builds: CPU-only, Metal, etc.Let's find out if this works in CI.
Summary by CodeRabbit
Documentation
Chores