fix: Allow building only llamacpp or only mistralrs engine. #1328
Conversation
grahamking
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
jthomson04,
kkranen,
oandreeva-nv,
paulhendricks,
rmccorm4,
ryanolson and
tmonty12
as code owners
WalkthroughThe code refactors the logic for selecting the default output engine in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant run()
participant gguf_default()
participant safetensors_default()
User->>run(): Call run()
alt Model is GGUF
run()->>gguf_default(): Determine default output
gguf_default()-->>run(): Output::LlamaCpp / Output::MistralRs / Output::EchoFull
else Model is safetensors
run()->>safetensors_default(): Determine default output
safetensors_default()-->>run(): Output::MistralRs / Output::EchoFull
end
run()-->>User: Complete with selected output engine
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 (1)
launch/dynamo-run/src/lib.rs (1)
415-421: Appropriate default selection for safetensors files.The
safetensors_default()function correctly prioritizes MistralRs for safetensors files since this is the primary engine for that format, with EchoFull as a reasonable fallback when MistralRs is unavailable.Consider adding documentation comments to explain the priority logic:
+/// Returns the default engine for GGUF files based on enabled features. +/// Priority: LlamaCpp > MistralRs > EchoFull fn gguf_default() -> Output {+/// Returns the default engine for safetensors files based on enabled features. +/// Priority: MistralRs > EchoFull fn safetensors_default() -> Output {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launch/dynamo-run/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (2)
launch/dynamo-run/src/lib.rs (2)
119-121: LGTM! Clean integration of conditional engine selection.The replacement of hardcoded
Output::LlamaCppandOutput::MistralRswith function calls provides the flexibility needed to support building with selective features. This change maintains the existing behavior when all features are enabled while gracefully handling single-feature builds.
404-413: Well-designed priority logic for GGUF files.The
gguf_default()function implements a sensible priority hierarchy:
- LlamaCpp (native GGUF support) - highest priority
- MistralRs (fallback for GGUF) - when LlamaCpp unavailable
- EchoFull (universal fallback) - when no engines available
The conditional compilation correctly handles all feature flag combinations and ensures deterministic behavior.
This allows building:
mistral.rsengine:--no-default-features --features mistralrsllama.cppengine:--no-default-features --features llamacpp.Since llama.cpp became a default we'd only tested building both at once. The docs already said we supported that but there was some combo of Rust features that didn't build. This is the fix.
Summary by CodeRabbit