Repository navigation
fix(common): make multimodal media loaders importable without torch - #11179
waynehacking8 wants to merge 1 commit into
Conversation
Importing anything from dynamo.common.multimodal eagerly executed the package __init__, which pulled in torch through two paths: - async_encoder_cache -> memory.multimodal_embedding_cache_manager -> torch, and embedding_transfer -> torch/safetensors (the chain reported in ai-dynamo#11172); - audio_loader's module-level best-effort vllm import, which drags in the whole vllm/torch stack whenever vllm is installed. Resolve the torch-dependent members (AsyncEncoderCache, the embedding-transfer classes, and the two embedding factory dicts) lazily via PEP 562, mirroring dynamo.common.configuration's module __getattr__, and move audio_loader's vllm import into _require_vllm_audio_media() to match video_loader's existing function-level pattern. The media loaders stay eagerly importable on their own dependency footprint (PIL/numpy/http). Public API is unchanged: all names in __all__ still resolve via plain from-imports, and the factory dicts keep stable identity after first access. Fixes ai-dynamo#11172. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfBBUwVVCtTK2TmUnPFHZf Signed-off-by: WEI CHENG CHIU <waynehacking8@gmail.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 (3)
WalkthroughThe multimodal package's ChangesLazy import refactor and validation
Estimated code review effort: 2 (Simple) | ~15 minutes Related issues: Resolves Suggested reviewers: none identified from provided context. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Template: review-only-pr User intentThe caller wants issue #11172 fixed so that lightweight multimodal imports, especially Discovery found an existing open GitHub PR that fully implements this requested behavior, so this work item should not author a duplicate fix. The remaining agency workflow should materialize and validate that existing PR's head diff under the Non-goals
DiscoveryI read the assigned brief at Relevant source files read in the checkout
Existing tests discovered:
Recent and related history checked:
Symptom and hypothesesSymptom to reproduce: in the current
PR #11179 addresses all three hypotheses and adds subprocess regression tests with negative controls, so it is the strongest existing implementation to reproduce and review. Chosen approachAdopt the The printer should materialize PR #11179's head diff (
Rejected alternatives
Validation strategyValidation must run only in the current sandbox through the recorder after the printer materializes PR #11179. The compute environment is vLLM with one A100, Planned evidence:
Expected recipe block:
Required deliverablesUnder the
Only the publisher may later comment on or publish to the configured review destination. |
Change packetWorking branch: Branch commit: Materialized review request: #11179 Supplied PR head SHA: Original author: WEI CHENG CHIU waynehacking8@gmail.com Attribution preserved in the reproduced commit author and original commit trailer metadata, including SummaryMaterialized PR #11179's head diff onto the configured local branch from Files changed
Build, lint, and testsNo build, lint, or pytest commands were run by the printer stage. Per the Safety constraints inspected
|
Change validationHardware target: local sandbox only, vLLM validation image, one NVIDIA A100-SXM4-80GB, CUDA 12.9. No Docker, Kubernetes, Slurm, SSH, or remote compute pathway was used. The planned and supported recipes were Recorded evidenceAll recipe commands were run through
No planned recipe is blocked. Upstream CI is context only; local recorded evidence is the basis for this verdict. Behavioral readingThe materialized diff matches upstream PR #11179 and changes exactly:
Source inspection supports the import-graph claim. The audio loader no longer imports vLLM media classes at module import time. The new regression tests are load-bearing: Checklist self-check
Verdict: pass |
Assessment: soundFindingsNo blocking or nit findings. Evidence reviewed
Assessment: sound |
Summary
Fixes #11172.
from dynamo.common.multimodal import ImageLoaderexecuted the package__init__.pyeagerly, which pulled intorchthrough two independent paths:__init__.py->async_encoder_cache->dynamo.common.memory.multimodal_embedding_cache_manager->import torch(plusembedding_transfer->import torch/safetensorsdirectly atembedding_transfer.py:17-19).audio_loader.pydid a module-level best-effortfrom vllm.multimodal.media import ..., so in any environment where vllm is installed, importing the package dragged in the whole vllm/torch stack even when onlyImageLoaderwas wanted.Fix
__init__.py: keep the three media loaders eagerly importable (their footprint is PIL/numpy plus the local http helpers), and resolve the torch-dependent members lazily via a module-level__getattr__(PEP 562):AsyncEncoderCache, the nine embedding-transfer names, and the two embedding factory dicts. This mirrors the existing house pattern indynamo.common.configuration.__init__anddynamo.common.__init__(option 1 from the issue, least churn).audio_loader.py: move the vllm import from module level into_require_vllm_audio_media(), matching the function-level patternvideo_loader.pyalready uses (_require_vllm_video_media).Public API is unchanged: every name in
__all__still resolves through plainfrom dynamo.common.multimodal import X; the factory dicts are built on first access, cached in module globals, and keep stable identity afterwards. The[gluo FIXME]note on the NIXL_READ receiver factory is preserved.Validation
New CPU-only regression test
components/src/dynamo/common/tests/multimodal/test_lazy_imports.py(markersgpu_0,pre_merge,unit). The torch-free assertions run in subprocesses so results are independent of what the pytest session already imported:torchandvllmout ofsys.modules;EmbeddingTransferMode;AttributeError.The 6 failures are
TestNixlReadEmbeddingTransfercases that fail identically on unmodified main in this environment (no NIXL library installed) — verified by stashing the change and rerunning.isort/black/ruffclean on the three touched files.Summary by CodeRabbit