Repository navigation
fix(multimodal): enforce media download limits - #15005
Conversation
Signed-off-by: host452b <32806348+host452b@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe multimodal audio, image, and video loaders now pass ChangesMultimodal media-size enforcement
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The requested media-size propagation is covered across all three loaders, and no actionable issue remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
/ok to test 6deddef |
Signed-off-by: host452b <32806348+host452b@users.noreply.github.com>
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The SGLang limit still does not cover supported deployments where NVDEC is disabled or unavailable.
_build_encode_inputsreturns validated URL strings in that branch, so SGLang fetches them with its own client and never receivesmax_bytes=max_media_bytes(). WithDYN_MM_MAX_FILE_SIZE_MBset below SGLang's own limit, a remote video above the Dynamo limit is still downloaded and buffered.
Signed-off-by: host452b <32806348+host452b@users.noreply.github.com>
|
/ok to test 14a3915 |
|
Hey @host452b, Could you resolve the merge conflicts on this PR and any remaining failing CI tests? Should be good to merge after that 🙂 |
OK will do |
Signed-off-by: host452b <32806348+host452b@users.noreply.github.com>
Already done, please check.thanks. |
|
/ok to test 55d72a8 |
rmccorm4
left a comment
There was a problem hiding this comment.
Approving docs change, 1 nit comment for future follow-up: https://github.com/ai-dynamo/dynamo/pull/15005/changes#r4076663272
Main now bounds every remote media download by DYN_MM_MAX_FILE_SIZE_MB (ai-dynamo#15005), so the shared-cache-only wording in the environment table no longer matches ImageLoader after the rebase. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: furionw <qiwa@nvidia.com>
Summary
DYN_MM_MAX_FILE_SIZE_MBto remote image, audio, and video loader downloads--max-file-size-mbby passing its configured byte limit toImageLoaderCloses #15004
Validation
106 passedacross image/audio/video loader tests and related HTTP/media-reference tests_build_encode_inputsmethod: remote capped fetch,data:passthrough, non-video passthrough, and missing-decoder fail-fast all passedpre-commit run --filesfor all modified filespython3 docs/fern/scripts/docs_lint.py --scan docs(0 errors)ImageLoaderoverride test and non-NVDEC SGLang capped-fetch reproduction both failed before their implementations, then passed after the fixesCompatibility
Remote media larger than the default 64 MiB generic limit is now rejected consistently, including SGLang software-decoder deployments. Deployments that intentionally accept larger media can raise
DYN_MM_MAX_FILE_SIZE_MB. TensorRT-LLM continues to use its existing--max-file-size-mb/DYN_TRTLLM_MAX_FILE_SIZE_MBsetting instead.