Add ADV audio processing dependencies - #1574
Conversation
Greptile SummaryThis PR restructures the audio dependency extras by introducing a new Key findings from this review:
Confidence Score: 3/5Not safe to merge — multiple unresolved P1 issues from prior rounds plus a newly found missing dependency (seaborn) that contradicts the PR description. The prior review rounds identified several genuine P1 defects (dropped platform guard on nvidia-cublas-cu12, missing [tool.uv.sources] for torchcodec causing CPU-only wheel in a GPU extra, missing [tool.uv.conflicts] guard, unconstrained version specs for silero-vad/torchaudio/nvidia-cudnn-cu12/torchcodec, and the transformers override potentially breaking nemo-toolkit[asr]). This review adds another P1: seaborn is described as a required ADV pipeline dependency but is absent from all audio extras. The combination of unresolved prior P1s and this new gap keeps the score at 3. pyproject.toml — multiple unconstrained version specs and missing seaborn; uv.lock — dropped platform marker on nvidia-cublas-cu12 and CPU-only torchcodec wheel resolved for the GPU extra. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["nemo-curator[audio_cpu]"] --> B["nemo_curator[audio_common]"]
C["nemo-curator[audio_cuda12]"] --> B
C --> D["nemo_curator[cuda12]"]
B --> E["nemo_toolkit[asr]>=2.7.2"]
B --> F["soundfile>=0.12.0"]
B --> G["torchaudio (unpinned)"]
B --> H["onnx>=1.19.0"]
B --> I["silero-vad (unpinned)"]
A --> J["onnxruntime>=1.20.1,<1.24"]
C --> K["onnxruntime-gpu>=1.20.1,<1.24"]
C --> L["nvidia-cudnn-cu12 (unpinned)"]
C --> M["torchcodec (unpinned, CPU wheel from PyPI)"]
N["seaborn (in PR description but MISSING)"] -.->|"Should be in"| B
style N fill:#ffcccc,stroke:#cc0000
Reviews (17): Last reviewed commit: "Merge branch 'main' into pr/audio-depend..." | Re-trigger Greptile |
| "nemo_curator[audio_cpu]", | ||
| "nemo_curator[audio_common]", | ||
| "nemo_curator[cuda12]", | ||
| "nvidia-cudnn-cu12", |
There was a problem hiding this comment.
nvidia-cudnn-cu12 missing version constraint
onnxruntime-gpu has strict cuDNN version requirements that vary by release. For example, onnxruntime-gpu 1.20.x requires cuDNN 8.x or 9.x at a specific minor version. Leaving nvidia-cudnn-cu12 unconstrained means a newer incompatible cuDNN release could be installed, causing silent runtime failures or missing symbol errors at import time.
Please add a version pin or at minimum an upper bound (e.g. nvidia-cudnn-cu12>=9.1,<10), matching what onnxruntime-gpu>=1.20.1 expects.
| "nvidia-cudnn-cu12", | |
| "nvidia-cudnn-cu12>=9.1,<10", |
| audio_common = [ | ||
| "nemo_toolkit[asr]==2.4.0", | ||
| # ADV (Audio DataVerse) Dependencies | ||
| "Cython", |
There was a problem hiding this comment.
Cython is a build-time dependency, not a runtime dependency
Cython is a compiler/transpiler used at build time to generate C extensions — it is not a package that is imported at runtime. Listing it under [project.optional-dependencies] (runtime extras) is unconventional and may install an unnecessary compiler toolchain in production environments.
If it is genuinely needed at runtime (e.g. for a package that compiles extensions on first use), please add a comment explaining why. Otherwise, consider moving it to the [dependency-groups] build group or removing it from this group.
There was a problem hiding this comment.
This seems valid but I don't know enough about build vs import time, @shubhamNvidia do you know which package ends up needing this a build time thing?
There was a problem hiding this comment.
Cython is a build-time dependency needed by some transitive deps of nemo_toolkit[asr] (like texterrors). It's not imported at runtime by our code. I can verify if it's still needed or if it can be removed.
| "seaborn>=0.11.2", | ||
| "onnx>=1.19.0", | ||
| "torchcodec", | ||
| "silero-vad", |
There was a problem hiding this comment.
silero-vad missing version constraint
silero-vad has undergone significant API changes across its release history (e.g., changes to the loading API and model interface between 0.3.x, 0.4.x, and 5.x). Without a version pin or at least a lower bound, a future installation could pull in an incompatible version and break the ADV pipeline silently at runtime.
Consider pinning or bounding the version to match the API your code relies on, e.g.:
| "silero-vad", | |
| "silero-vad>=4.0,<6", |
|
Fyi @shubhamNvidia whenever the |
sarahyurick
left a comment
There was a problem hiding this comment.
uv.lock file needs to be updated by running uv lock within your Curator directory.
cc @thomasdhc who may be able to help with any dependency issues.
| "torchaudio", | ||
| "seaborn>=0.11.2", | ||
| "onnx>=1.19.0", | ||
| "torchcodec", |
There was a problem hiding this comment.
torchcodec missing from [tool.uv.sources]
All other PyTorch-ecosystem packages added as optional dependencies (torch, torchaudio, torchvision) have explicit source routing in [tool.uv.sources] to pull GPU-aware wheels from https://download.pytorch.org/whl/cu128 on x86_64 Linux:
torchaudio = [
{ index = "pytorch", marker = "platform_machine == 'x86_64' and sys_platform != 'darwin'" },
{ index = "pypi", marker = "platform_machine != 'x86_64' or sys_platform == 'darwin'" },
]torchcodec is a PyTorch-maintained package that similarly ships CUDA-specific wheels from the same PyTorch index. Without a matching entry in [tool.uv.sources], uv will fall back to PyPI (or the NVIDIA index) for torchcodec, which may resolve a CPU-only wheel, an incompatible CUDA build, or a version whose CUDA extension was not compiled against cu128. This would cause silent runtime failures for GPU-accelerated codec operations.
Please add a torchcodec entry to [tool.uv.sources] analogous to torchaudio/torchvision.
| # ADV (Audio DataVerse) Dependencies | ||
| "Cython", | ||
| "packaging", | ||
| "torchaudio", |
There was a problem hiding this comment.
torchaudio version constraint missing
torchaudio releases are strictly coupled to specific torch versions (e.g., torchaudio==2.5.0 requires exactly torch==2.5.0). Installing an unpinned torchaudio alongside torch (which is already a base dependency with no upper-bound) can result in a version mismatch that raises an ImportError or produces incorrect audio processing behaviour at runtime.
Consider adding a version constraint that mirrors what nemo_toolkit[asr]==2.4.0 (already in audio_common) expects, or at minimum align it with the torch version that NeMo 2.4.0 requires.
| "torchaudio", | |
| "torchaudio>=2.5.0,<2.6.0", |
(replace the version range with whatever NeMo 2.4.0 expects)
| "torchaudio", | ||
| "seaborn>=0.11.2", | ||
| "onnx>=1.19.0", | ||
| "torchcodec", |
There was a problem hiding this comment.
torchcodec missing version constraint
torchcodec has no version constraint, meaning any release could be installed. Its C++ extension API changes between minor versions and its CUDA kernels are compiled against a specific torch version. Without a lower bound (and ideally an upper bound), a future installation could pull an incompatible release.
| "torchcodec", | |
| "torchcodec>=0.2.0,<0.4", |
(adjust the range to match what the ADV pipeline has been validated against)
|
@sarahyurick I’ve merged with main and resolved the conflicts—you can go ahead and run the Ruff linter now |
|
/ok to test c9e6752 |
Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
|
/ok to test 5d6f6d8 |
|
/ok to test bf2ce78 |
| "apex; sys_platform == 'never'", | ||
| "distance; sys_platform == 'never'", | ||
| "huggingface-hub>=0.34,<1.0", # Override huggingface-hub, transformers and data-designer require two different versions of hugging-face hub | ||
| "transformers>=4.56.0,<5.0", # Override nemo-toolkit[asr]'s <=4.52.0 constraint; required for Nemotron VL support |
There was a problem hiding this comment.
transformers override bypasses nemo-toolkit[asr]'s compatibility bound
nemo_toolkit[asr]>=2.7.2 (resolved to 2.7.2) declares transformers<=4.52.0 as a hard upper bound. This PR adds transformers>=4.56.0,<5.0 to override-dependencies, forcing the global resolution to 4.57.6 (visible in the lock file) — a version outside the range nemo-toolkit supports.
If any ASR code path inside nemo-toolkit 2.7.2 uses a transformers API that was removed, renamed, or semantically changed between 4.52.0 and 4.56.0, the audio pipeline will fail at runtime without any installation-time error. The PR comment attributes this override to "Nemotron VL support", which is unrelated to the audio extras, yet it silently invalidates the ASR dependency contract for all audio users.
Please verify that nemo-toolkit 2.7.2 ASR functionality is tested and confirmed working with transformers 4.56+, or pin the nemo_toolkit[asr] requirement to a release whose supported transformers range already covers 4.56+.
sarahyurick
left a comment
There was a problem hiding this comment.
LGTM. Awaiting a final review from @ayushdg as well.
|
/claude review |
| "apex; sys_platform == 'never'", | ||
| "distance; sys_platform == 'never'", | ||
| "huggingface-hub>=0.34,<1.0", # Override huggingface-hub, transformers and data-designer require two different versions of hugging-face hub | ||
| "transformers>=4.56.0,<5.0", # Override nemo-toolkit[asr]'s <=4.52.0 constraint; required for Nemotron VL support |
There was a problem hiding this comment.
This is no longer needed as we have bumped nemo toolkit. L267 has this logic in constraints already
There was a problem hiding this comment.
You're right, the transformers override is redundant now that nemo-toolkit has been bumped. I'll remove it.
| audio_common = [ | ||
| "nemo_toolkit[asr]==2.4.0", | ||
| # ADV (Audio DataVerse) Dependencies | ||
| "Cython", |
There was a problem hiding this comment.
This seems valid but I don't know enough about build vs import time, @shubhamNvidia do you know which package ends up needing this a build time thing?
| version = "9.10.2.21" | ||
| source = { registry = "https://pypi.nvidia.com/" } | ||
| dependencies = [ | ||
| { name = "nvidia-cublas-cu12", marker = "platform_machine == 'x86_64' and sys_platform == 'linux'" }, | ||
| { name = "nvidia-cublas-cu12" }, | ||
| ] | ||
| wheels = [ | ||
| { url = "https://pypi.nvidia.com/nvidia-cudnn-cu12/nvidia_cudnn_cu12-9.10.2.21-py3-none-manylinux_2_27_aarch64.whl", hash = "sha256:c9132cc3f8958447b4910a1720036d9eff5928cc3179b0a51fb6d167c6cc87d8" }, | ||
| { url = "https://pypi.nvidia.com/nvidia-cudnn-cu12/nvidia_cudnn_cu12-9.10.2.21-py3-none-manylinux_2_27_x86_64.whl", hash = "sha256:949452be657fa16687d0930933f032835951ef0892b37d2d53824d1a84dc97a8" }, | ||
| { url = "https://pypi.nvidia.com/nvidia-cudnn-cu12/nvidia_cudnn_cu12-9.10.2.21-py3-none-win_amd64.whl", hash = "sha256:c6288de7d63e6cf62988f0923f96dc339cea362decb1bf5b3141883392a7d65e" }, | ||
| ] | ||
|
|
||
| [[package]] |
There was a problem hiding this comment.
nvidia-cublas-cu12 platform guard silently dropped
Adding nvidia-cudnn-cu12 as an explicit requirement in audio_cuda12 caused uv to regenerate the nvidia-cudnn-cu12 lock entry with updated upstream metadata. As a result, the marker that previously restricted nvidia-cublas-cu12 to platform_machine == 'x86_64' and sys_platform == 'linux' was removed:
- { name = "nvidia-cublas-cu12", marker = "platform_machine == 'x86_64' and sys_platform == 'linux'" },
+ { name = "nvidia-cublas-cu12" },nvidia-cublas-cu12 ships only manylinux wheels (no Windows or macOS). Now that nvidia-cudnn-cu12 also has win_amd64 and aarch64 wheels in the lock, resolving audio_cuda12 on Windows will pull in nvidia-cudnn-cu12 and attempt to satisfy its nvidia-cublas-cu12 dependency — but no matching Windows wheel exists, producing a hard installation failure on that platform.
If Windows is an unsupported target for audio_cuda12, add a platform marker to the extra itself (e.g. ; sys_platform == 'linux'), or add nvidia-cublas-cu12 with an explicit Linux-only marker so the resolver doesn't attempt to install it on Windows.
|
/ok to test 332e2c7 |
Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
|
/ok to test ebb4187 |
|
/ok to test 5ec3af4 |
| "Cython", | ||
| "packaging", | ||
| "torchaudio", | ||
| "seaborn>=0.11.2", |
| "soundfile>=0.12.0", | ||
| # ADV (Audio DataVerse) Dependencies | ||
| "Cython", | ||
| "packaging", |
There was a problem hiding this comment.
Cython and packaging: These are build-time prerequisites for nemo_toolkit[asr] — the [NeMo Sortformer model card] explicitly requires pip install Cython packaging before installing NeMo
There was a problem hiding this comment.
They're not imported at runtime. I'll move them to [dependency-groups] build as suggested by @thomasdhc
seaborn: Not used anywhere, will remove.
There was a problem hiding this comment.
@shubhamNvidia Please move build time dependencies here. cc: @ayushdg
| ] | ||
|
|
||
| audio_cpu = [ | ||
| audio_common = [ |
There was a problem hiding this comment.
Why make a distinct group for audio common? Audio cuda inherits from audio cpu
There was a problem hiding this comment.
audio_common exists because audio_cpu and audio_cuda12 need different onnxruntime packages (onnxruntime vs onnxruntime-gpu). If audio_cuda12 inherited from audio_cpu, both CPU and GPU versions of onnxruntime would be installed, which can cause conflicts. audio_common shares everything except the onnxruntime variant.
|
@shubhamNvidia The PR introduces a list of new dependencies, but there are no code changes that import these in this PR. Will there be a follow-up? Trying to understand the scope and requirements in this PR. |
|
Hi @thomasdhc this PR includes dependencies for the following implementations: |
|
/ok to test 28cd599 |
Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
|
/ok to test 23a359b |
Adds the audio_common dependency group with packages required by the Audio DataVerse (ADV) pipeline stages: silero-vad, onnx, onnxruntime/onnxruntime-gpu, nvidia-cudnn-cu12, torchaudio, Cython, seaborn, torchcodec, and packaging. Splits audio_cpu and audio_cuda12 to properly separate CPU-only and GPU-accelerated ONNX runtime variants.