Repository navigation
fix(multimodal): stop the media loaders from importing torch #13626
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
7118da3
adde1d5
b70ad6d
aeb8cd2
657d248
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,44 +1,116 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Multimodal utilities for Dynamo components.""" | ||
| """Multimodal utilities for Dynamo components. | ||
|
|
||
| from collections.abc import Callable | ||
| `AsyncEncoderCache` and the embedding-transfer members import `torch`, while the | ||
| image, audio and video loaders do not. Re-exporting all of them eagerly meant | ||
| `from dynamo.common.multimodal import ImageLoader` pulled in the whole ML stack, | ||
| so the loaders could not be imported or unit-tested without it. The heavy names | ||
| resolve on first access instead (PEP 562); the loaders stay eager because they | ||
| cost nothing. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from dynamo.common.constants import EmbeddingTransferMode | ||
| from dynamo.common.multimodal.async_encoder_cache import AsyncEncoderCache | ||
| from dynamo.common.multimodal.audio_loader import AudioLoader | ||
| from dynamo.common.multimodal.embedding_transfer import ( | ||
| AbstractEmbeddingReceiver, | ||
| AbstractEmbeddingSender, | ||
| LocalEmbeddingReceiver, | ||
| LocalEmbeddingSender, | ||
| NixlReadEmbeddingReceiver, | ||
| NixlReadEmbeddingSender, | ||
| NixlWriteEmbeddingReceiver, | ||
| NixlWriteEmbeddingSender, | ||
| TransferRequest, | ||
| ) | ||
| from dynamo.common.multimodal.image_loader import ImageLoader | ||
| from dynamo.common.multimodal.video_loader import VideoLoader | ||
|
|
||
| EMBEDDING_SENDER_FACTORIES: dict[ | ||
| EmbeddingTransferMode, Callable[[], AbstractEmbeddingSender] | ||
| ] = { | ||
| EmbeddingTransferMode.LOCAL: LocalEmbeddingSender, | ||
| EmbeddingTransferMode.NIXL_WRITE: NixlWriteEmbeddingSender, | ||
| EmbeddingTransferMode.NIXL_READ: NixlReadEmbeddingSender, | ||
| } | ||
|
|
||
| EMBEDDING_RECEIVER_FACTORIES: dict[ | ||
| EmbeddingTransferMode, Callable[[], AbstractEmbeddingReceiver] | ||
| ] = { | ||
| EmbeddingTransferMode.LOCAL: LocalEmbeddingReceiver, | ||
| EmbeddingTransferMode.NIXL_WRITE: NixlWriteEmbeddingReceiver, | ||
| # [gluo FIXME] can't use pre-registered tensor as NIXL requires descriptors | ||
| # to be at matching size, need to overwrite nixl connect library | ||
| EmbeddingTransferMode.NIXL_READ: lambda: NixlReadEmbeddingReceiver(max_items=0), | ||
| } | ||
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
|
|
||
| from dynamo.common.constants import EmbeddingTransferMode | ||
| from dynamo.common.multimodal.async_encoder_cache import AsyncEncoderCache | ||
| from dynamo.common.multimodal.embedding_transfer import ( | ||
| AbstractEmbeddingReceiver, | ||
| AbstractEmbeddingSender, | ||
| LocalEmbeddingReceiver, | ||
| LocalEmbeddingSender, | ||
| NixlReadEmbeddingReceiver, | ||
| NixlReadEmbeddingSender, | ||
| NixlWriteEmbeddingReceiver, | ||
| NixlWriteEmbeddingSender, | ||
| TransferRequest, | ||
| ) | ||
|
|
||
| _EMBEDDING_TRANSFER_EXPORTS = frozenset( | ||
| { | ||
| "AbstractEmbeddingReceiver", | ||
| "AbstractEmbeddingSender", | ||
| "LocalEmbeddingReceiver", | ||
| "LocalEmbeddingSender", | ||
| "NixlReadEmbeddingReceiver", | ||
| "NixlReadEmbeddingSender", | ||
| "NixlWriteEmbeddingReceiver", | ||
| "NixlWriteEmbeddingSender", | ||
| "TransferRequest", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| def _build_sender_factories() -> ( | ||
| dict[EmbeddingTransferMode, Callable[[], AbstractEmbeddingSender]] | ||
| ): | ||
| from dynamo.common.constants import EmbeddingTransferMode | ||
| from dynamo.common.multimodal.embedding_transfer import ( | ||
| LocalEmbeddingSender, | ||
| NixlReadEmbeddingSender, | ||
| NixlWriteEmbeddingSender, | ||
| ) | ||
|
|
||
| return { | ||
| EmbeddingTransferMode.LOCAL: LocalEmbeddingSender, | ||
| EmbeddingTransferMode.NIXL_WRITE: NixlWriteEmbeddingSender, | ||
| EmbeddingTransferMode.NIXL_READ: NixlReadEmbeddingSender, | ||
| } | ||
|
|
||
|
|
||
| def _build_receiver_factories() -> ( | ||
| dict[EmbeddingTransferMode, Callable[[], AbstractEmbeddingReceiver]] | ||
| ): | ||
| from dynamo.common.constants import EmbeddingTransferMode | ||
| from dynamo.common.multimodal.embedding_transfer import ( | ||
| LocalEmbeddingReceiver, | ||
| NixlReadEmbeddingReceiver, | ||
| NixlWriteEmbeddingReceiver, | ||
| ) | ||
|
|
||
| return { | ||
| EmbeddingTransferMode.LOCAL: LocalEmbeddingReceiver, | ||
| EmbeddingTransferMode.NIXL_WRITE: NixlWriteEmbeddingReceiver, | ||
| # [gluo FIXME] can't use pre-registered tensor as NIXL requires descriptors | ||
| # to be at matching size, need to overwrite nixl connect library | ||
| EmbeddingTransferMode.NIXL_READ: lambda: NixlReadEmbeddingReceiver(max_items=0), | ||
| } | ||
|
|
||
|
|
||
| def __getattr__(name: str) -> Any: | ||
| if name == "AsyncEncoderCache": | ||
| from dynamo.common.multimodal.async_encoder_cache import AsyncEncoderCache | ||
|
|
||
| value: Any = AsyncEncoderCache | ||
| elif name in _EMBEDDING_TRANSFER_EXPORTS: | ||
| from dynamo.common.multimodal import embedding_transfer | ||
|
|
||
| value = getattr(embedding_transfer, name) | ||
| elif name == "EMBEDDING_SENDER_FACTORIES": | ||
| value = _build_sender_factories() | ||
| elif name == "EMBEDDING_RECEIVER_FACTORIES": | ||
| value = _build_receiver_factories() | ||
| else: | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | ||
|
|
||
| # Cache on the module so repeat access skips this function entirely. | ||
| globals()[name] = value | ||
| return value | ||
|
|
||
|
|
||
| def __dir__() -> list[str]: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in b70ad6d. You are right, and this was a regression I introduced: before this PR there was no I went slightly further than return sorted(set(__all__) | _EMBEDDING_TRANSFER_EXPORTS | set(globals()))Verified on a fresh interpreter with nothing accessed first: |
||
| return sorted(__all__) | ||
|
|
||
|
|
||
| __all__ = [ | ||
| "AsyncEncoderCache", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,44 @@ | ||||||||||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||
|
|
||||||||||||||||
| """The package must not drag `torch` in behind the lightweight media loaders.""" | ||||||||||||||||
|
|
||||||||||||||||
| from __future__ import annotations | ||||||||||||||||
|
|
||||||||||||||||
| import subprocess | ||||||||||||||||
| import sys | ||||||||||||||||
|
|
||||||||||||||||
| import pytest | ||||||||||||||||
|
|
||||||||||||||||
| pytestmark = [pytest.mark.pre_merge, pytest.mark.unit, pytest.mark.gpu_0] | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 New subprocess test lacks required timeout marker The test spawns a fresh interpreter with The repository's
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct finding, and it was already addressed before this thread got a reply, which is my fault for leaving it silent.
I used 60 rather than the 120 suggested. The probe is a bare |
||||||||||||||||
|
|
||||||||||||||||
| # A fresh interpreter is the only honest way to ask this: once any earlier test | ||||||||||||||||
| # has imported torch, an eager import here would go unnoticed. | ||||||||||||||||
| _PROBE = """ | ||||||||||||||||
| import sys | ||||||||||||||||
| from dynamo.common.multimodal import {names} | ||||||||||||||||
| print("torch" in sys.modules) | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _torch_imported_by(names: str) -> bool: | ||||||||||||||||
| result = subprocess.run( | ||||||||||||||||
| [sys.executable, "-c", _PROBE.format(names=names)], | ||||||||||||||||
| capture_output=True, | ||||||||||||||||
| text=True, | ||||||||||||||||
| check=True, | ||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separate point from the timeout thread on this line. With Catching it and re-raising with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in adde1d5. This was the most valuable comment on the PR, because the failure mode is silent and points the wrong way: the three loader cases assert the negative, so stdout noise makes them pass rather than fail. It is not hypothetical either.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one had not actually been answered, and my earlier reply is the reason it looked like it had. I posted the sentinel explanation into your Answering it properly now, and you were right. Reproduced it first rather than taking the argument on faith. On the code as it stood, a child that dies gives pytest this and nothing else: The Fixed in 657d248. Dropped assert (
result.returncode == 0
), f"probe for {names} exited {result.returncode}\nstderr:\n{result.stderr}"Same failure now reads: What the test asserts is unchanged, and the four cases still pass. |
||||||||||||||||
| ) | ||||||||||||||||
| return result.stdout.strip() == "True" | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Matching on the last line, or having the probe print a sentinel ( For what it is worth,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answering this in its own thread, because my reply to it went into the This was the most valuable comment on the PR, and it is not hypothetical. The three loader cases assert the negative, so stdout noise makes them pass rather than fail, which is the one regression the test exists to catch. Fixed in adde1d5 with the sentinel you suggested: the probe prints |
||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| @pytest.mark.parametrize("name", ["ImageLoader", "AudioLoader", "VideoLoader"]) | ||||||||||||||||
| def test_media_loader_import_does_not_pull_torch(name: str) -> None: | ||||||||||||||||
| assert not _torch_imported_by( | ||||||||||||||||
| name | ||||||||||||||||
| ), f"importing {name} pulled in torch; the loaders must stay usable without the ML stack" | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def test_heavy_members_still_import() -> None: | ||||||||||||||||
| assert _torch_imported_by( | ||||||||||||||||
| "AsyncEncoderCache" | ||||||||||||||||
| ), "AsyncEncoderCache should still resolve, and it legitimately needs torch" | ||||||||||||||||
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.
Going lazy here has two consequences, one local and one package-wide.
Local:
EMBEDDING_SENDER_FACTORIESandEMBEDDING_RECEIVER_FACTORIESare not declared in this block, so under mypy they resolve toAnyand callers lose the key and value types. The description says the names are re-declared underTYPE_CHECKINGso type checkers still resolve them — that holds for the classes, but not for these two.Package-wide: a module-level
__getattr__tells mypy the package may have any attribute, sofrom dynamo.common.multimodal import DoesNotExistAtAllnow type-checks clean — for every module importing from this package, not just this file. That is a real loss of typo detection, and nothing in the diff shows it.Declaring both dicts in this block closes the first half. The second needs a
.pyistub, or anif not TYPE_CHECKING:guard around__getattr__so the checker never sees the widening.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.
Both halves confirmed and fixed in adde1d5, and the package-wide one is the more serious of the two.
I checked rather than taking it on faith, and my first attempt to check was wrong in a way worth mentioning: I ran mypy with
--follow-imports=skip, which makes the moduleAnyregardless and reported no problem. Following imports properly reproduces exactly what you describe.Before:
After:
For the package-wide half I took the
if not TYPE_CHECKING:guard rather than a.pyi. The stub would have to restate every export and then be kept in step with__all__and_EMBEDDING_TRANSFER_EXPORTSby hand, which is a second source of truth for no extra benefit here; the guard leaves the checker seeing exactly the declarations that are already in the file. Runtime is unaffected, sinceTYPE_CHECKINGis False when it matters, andmm.NoSuchThingstill raisesAttributeError.On the factory dicts you are also right that my PR description was wrong. It said the names were re-declared so type checkers still resolve them, which held for the classes and not for those two. They are declared now.