-
Notifications
You must be signed in to change notification settings - Fork 676
feat: Media decoder and fetcher options in the MDC #4094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2e99f5f to
4ab87ec
Compare
b015a61 to
3c4aa12
Compare
4ab87ec to
9bce789
Compare
9bce789 to
35cdad7
Compare
WalkthroughThis PR integrates media preprocessing capabilities into the system. It introduces MediaDecoder and MediaFetcher classes in Python bindings, adds corresponding Rust wrapper types, propagates these fields through LocalModelBuilder and ModelDeploymentCard, and exposes them in the Python API surface. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/bindings/python/rust/lib.rs(5 hunks)lib/bindings/python/rust/llm/preprocessor.rs(2 hunks)lib/bindings/python/src/dynamo/llm/__init__.py(1 hunks)lib/llm/src/local_model.rs(7 hunks)lib/llm/src/model_card.rs(3 hunks)lib/llm/src/preprocessor/media.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-24T20:59:35.725Z
Learnt from: ishandhanani
Repo: ai-dynamo/dynamo PR: 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.
Applied to files:
lib/bindings/python/rust/llm/preprocessor.rs
📚 Learning: 2025-09-02T16:46:54.015Z
Learnt from: GuanLuo
Repo: ai-dynamo/dynamo PR: 2714
File: lib/llm/src/discovery/model_entry.rs:38-42
Timestamp: 2025-09-02T16:46:54.015Z
Learning: In lib/llm/src/discovery/model_entry.rs, GuanLuo prefers not to add serde defaults for model_type and model_input fields to keep the specification explicit and avoid user errors, relying on atomic deployment strategy to avoid backward compatibility issues.
Applied to files:
lib/llm/src/model_card.rslib/llm/src/local_model.rslib/bindings/python/rust/lib.rs
🧬 Code graph analysis (3)
lib/bindings/python/src/dynamo/llm/__init__.py (1)
lib/bindings/python/rust/lib.rs (1)
_core(127-202)
lib/bindings/python/rust/llm/preprocessor.rs (2)
lib/bindings/python/rust/lib.rs (1)
pythonize(753-753)lib/llm/src/local_model.rs (1)
default(61-86)
lib/bindings/python/rust/lib.rs (1)
lib/llm/src/local_model.rs (6)
runtime_config(172-175)runtime_config(393-395)user_data(177-180)custom_template_path(152-155)media_decoder(192-195)media_fetcher(197-200)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/4094/merge) by milesial.
lib/bindings/python/src/dynamo/llm/__init__.py
[error] 1-1: pre-commit isort hook failed: files were modified by this hook in lib/bindings/python/src/dynamo/llm/init.py. Command failed: pre-commit run --show-diff-on-failure --color=always --all-files
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: operator (amd64)
- GitHub Check: vllm (amd64)
- GitHub Check: operator (arm64)
- GitHub Check: sglang (amd64)
- GitHub Check: trtllm (amd64)
- GitHub Check: trtllm (arm64)
- GitHub Check: sglang (arm64)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: clippy (lib/bindings/python)
- GitHub Check: clippy (launch/dynamo-run)
- GitHub Check: clippy (.)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (11)
lib/llm/src/preprocessor/media.rs (1)
10-10: LGTM!The re-export correctly exposes
MediaFetcheralongsideMediaLoader, enabling its use in the Python bindings and model configuration.lib/llm/src/model_card.rs (2)
221-227: LGTM!The media configuration fields are properly declared with
#[serde(default)]attributes, consistent with other optional configuration fields in the struct. Note that these fields are intentionally excluded from themdcsum()calculation (similar toruntime_configanduser_data), which appears correct since they represent operational configuration rather than model identity.
532-533: LGTM!Initialization to
Noneis correct and consistent with the pattern established for other optional fields in thefrom_repo_checkoutmethod.lib/bindings/python/rust/lib.rs (3)
163-164: LGTM!The Python class registrations correctly expose
MediaDecoderandMediaFetcherto the Python module, following the established pattern for other PyO3 classes.
221-221: LGTM!The signature extension correctly adds the media configuration parameters as optional, maintaining backward compatibility.
Also applies to: 237-238
311-313: LGTM!The propagation correctly extracts the inner Rust types from the Python wrappers using
.map(|m| m.inner)and forwards them to the builder.lib/bindings/python/rust/llm/preprocessor.rs (1)
80-102: LGTM!The
MediaDecoderPython wrapper correctly provides a default constructor and animage_decodersetter that converts Python dict to Rust type usingdepythonize, with appropriate error handling.lib/llm/src/local_model.rs (4)
56-57: LGTM!The new fields are properly declared and initialized with
Default::default(), consistent with other optional configuration fields in the builder.Also applies to: 83-84
192-200: LGTM!The builder methods correctly follow the fluent builder pattern, accepting
Option<T>and returning&mut Selffor method chaining.
237-238: LGTM!Initializing media components with defaults in mocker mode is appropriate for testing scenarios.
250-251: LGTM!The propagation of media configuration to the
ModelDeploymentCardis correctly implemented in both the path-less and path-ful branches, using.clone()to copy the Option values.Also applies to: 302-303
35cdad7 to
0f21ad8
Compare
indrajit96
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Trying to merge this to skip the flaky failing vllm check: #4188 |
|
Updating with main to pull in #4188 to skip flaky vllm test |
|
Pulled in #4198 |
Overview:
Building on top of #3971, exposing decoder options and HTTP fetch options ini the MDC and python bindings.
Summary by CodeRabbit