feat: Validation engine for validating OpenAI api request data #1674
Conversation
nathan-barry
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
grahamking,
jthomson04,
kkranen,
oandreeva-nv,
paulhendricks,
rmccorm4,
ryanolson and
tmonty12
as code owners
|
👋 Hi nathan-barry! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
external-contribution
WalkthroughA generic request validation layer was introduced via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ValidateEngine
participant InnerEngine
Client->>ValidateEngine: generate(request, context)
ValidateEngine->>ValidateRequest: validate(request)
alt Validation fails
ValidateEngine-->>Client: error
else Validation succeeds
ValidateEngine->>InnerEngine: generate(request, context)
InnerEngine-->>ValidateEngine: response
ValidateEngine-->>Client: response
end
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
|
![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
♻️ Duplicate comments (1)
lib/llm/src/protocols/openai/chat_completions.rs (1)
180-192: LGTM! Consistent validation implementation.The temperature validation logic is identical to the implementation in
lib/llm/src/protocols/openai/completions.rs(lines 284-292), which ensures consistency across request types.This implementation duplicates the validation logic from the completions.rs file. As suggested in the completions.rs review, consider extracting this into a shared utility function.
🧹 Nitpick comments (1)
lib/llm/src/protocols/openai/completions.rs (1)
281-293: LGTM! Consider extracting common validation logic.The temperature validation logic is correct and matches the OpenAI API specification. However, this exact validation logic is duplicated in
lib/llm/src/protocols/openai/chat_completions.rs(lines 184-191).Consider extracting the temperature validation into a shared utility function to reduce code duplication:
fn validate_temperature(temperature: Option<f32>) -> Result<(), anyhow::Error> { if let Some(temp) = temperature { if temp < 0.0 || temp > 2.0 { anyhow::bail!("Temperature must be between 0.0 and 2.0, got {}", temp); } } Ok(()) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/llm/src/engines.rs(3 hunks)lib/llm/src/protocols/openai/chat_completions.rs(2 hunks)lib/llm/src/protocols/openai/completions.rs(2 hunks)lib/runtime/src/pipeline/context.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
lib/llm/src/protocols/openai/completions.rs (1)
Learnt from: ishandhanani
PR: ai-dynamo/dynamo#1626
File: lib/llm/src/preprocessor.rs:238-239
Timestamp: 2025-06-24T20:59:35.725Z
Learning: In lib/llm/src/preprocessor.rs, the `sampling_options` call in the `preprocess_request` method is placed in the common section after the match statement on `request.prompt_input_type()`, meaning it applies to both `PromptInput::Tokens` and `PromptInput::Text` request types.
🧬 Code Graph Analysis (2)
lib/llm/src/protocols/openai/completions.rs (2)
lib/llm/src/protocols/openai/chat_completions.rs (1)
validate(183-191)lib/llm/src/engines.rs (1)
validate(152-152)
lib/llm/src/protocols/openai/chat_completions.rs (2)
lib/llm/src/protocols/openai/completions.rs (1)
validate(284-292)lib/llm/src/engines.rs (1)
validate(152-152)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 (6)
lib/runtime/src/pipeline/context.rs (1)
51-58: LGTM! Clean implementation of context reconstruction.The
rejoinmethod provides a clean way to reconstruct a context with a new data type while preserving the shared internal state. This is exactly what's needed for the validation engine to rebuild the request context after validation.lib/llm/src/protocols/openai/completions.rs (1)
21-21: LGTM! Proper import of ValidateRequest trait.lib/llm/src/protocols/openai/chat_completions.rs (1)
20-20: LGTM! Proper import of ValidateRequest trait.lib/llm/src/engines.rs (3)
127-136: LGTM! Clean wrapper engine implementation.The
ValidateEnginefollows the same wrapper pattern asEngineDispatcher, providing a consistent approach to engine composition. The simple constructor and generic design make it easy to wrap any existing engine with validation.
150-153: LGTM! Well-designed validation trait.The
ValidateRequesttrait provides a clean, extensible interface for request validation. The single method design withanyhow::Errorreturn type allows for flexible error handling and descriptive error messages.
286-308: LGTM! Robust validation engine implementation.The
AsyncEngineimplementation forValidateEnginecorrectly:
- Extracts the request and context using
into_parts()- Validates the request before processing
- Returns validation errors immediately without forwarding to the inner engine
- Uses
SingleIn::rejoin()to reconstruct the request context after validation- Maintains the async engine contract
The error handling and context management are properly implemented.
|
Will remember to do cargo clippy before hand next time (and set it up in my neovim config) |
ryanolson
left a comment
ryanolson
left a comment
There was a problem hiding this comment.
Thanks. I'll let @paulhendricks do more of the review but this conceptually feels right.
We explicitly do not do validation at the http service so that users have the ability to do their own validations and apply their own model specific defaults on the processor (pre/post).
Wrapping this as an engine is a great use of the composition of engines.
We should treat this as an example of OAI validation and use as a reference for writing a custom validator.
This should be applied after model specific defaults are joined into the request (see figments definition of merge vs join).
|
Hey, in this recent commit, I basically just added most of the field verification checks in @grahamking let me know how it looks. |
|
This is fantastic! Let's get it merged. |
|
@nathan-barry Can you rebase and fix the conflict? Then I'll merge it. |
|
@grahamking got it, will do this after work later today |
Signed-off-by: Nathan Barry <38043930+nathan-barry@users.noreply.github.com>
|
@nathan-barry OK for me to merge it? |
|
@grahamking Yeah. If you want me to go back and apply those stylistic changes and also add a bunch of tests cases, I can (let me know what file or folder for the tests, not looked to extensively in that folder). Otherwise, you can just merge it |
|
Merged! Thanks again. Tests in a new PR would be lovely. In Rust the unit tests go in the file itself, usually in a |
3 participants
Overview:
Implements an OpenAI request validation engine wrapper. This adds a generic ValidateEngine that wraps another engine and validates OpenAI API parameters before forwarding requests to the inner engine. Currently just an MVP which validates temperature, waiting for feedback before validating more fields.
Details:
ValidateEngine<E>generic wrapper inlib/llm/src/engines.rsthat can wrap any AsyncEngine implementationValidateRequesttrait inlib/llm/src/engines.rsfor Request typesValidateRequestimplementations for both request types:NvCreateChatCompletionRequestinlib/llm/src/protocols/openai/chat_completions.rsNvCreateCompletionRequestinlib/llm/src/protocols/openai/completions.rsContext::rejoin()method inlib/runtime/src/pipeline/context.rsto properly reconstruct request context after validation (since we split it via.into_parts()to get access to the data)AsyncEngineimplementation forValidateEngineinlib/llm/src/engines.rs, which validates any request type implementingValidateRequestbefore sending it to the inner engine.The
ValidateEnginefollows the existing engine wrapper pattern (similar to EngineDispatcher).Where should the reviewer start?
The main additions are in
lib/llm/src/engines.rsLet me know whether this looks good. Also, let me know where I should write the tests. I'm developing locally on a MacBook, so doing integration tests might be odd (not too relevant here, but for future PRs). Rust unit tests should be simple though.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Improvements