Repository navigation
Add ADV audio processing dependencies #1574
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -95,13 +95,27 @@ deduplication_cuda12 = [ | |||||
| "rapidsmpf-cu12==25.10.*", | ||||||
| ] | ||||||
|
|
||||||
| audio_common = [ | ||||||
| "nemo_toolkit[asr]>=2.7.2", | ||||||
| "soundfile>=0.12.0", | ||||||
| # ADV (Audio DataVerse) Dependencies | ||||||
| "Cython", | ||||||
|
Contributor
There was a problem hiding this comment.
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
Contributor
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?
Contributor
Author
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. |
||||||
| "packaging", | ||||||
|
Contributor
Author
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
Contributor
Author
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 |
||||||
| "torchaudio", | ||||||
|
Contributor
There was a problem hiding this comment.
Consider adding a version constraint that mirrors what
Suggested change
(replace the version range with whatever NeMo 2.4.0 expects) |
||||||
| "seaborn>=0.11.2", | ||||||
|
|
||||||
| "onnx>=1.19.0", | ||||||
| "silero-vad", | ||||||
|
Contributor
There was a problem hiding this comment.
Consider pinning or bounding the version to match the API your code relies on, e.g.:
Suggested change
|
||||||
| ] | ||||||
|
Comment on lines
+98
to
+104
Contributor
There was a problem hiding this comment.
If # Internal base group — do not install directly; use audio_cpu or audio_cuda12
audio_common = [
...
]This prevents accidental misuse without requiring a structural change. |
||||||
| audio_cpu = [ | ||||||
| "nemo_curator[audio_common]", | ||||||
| "onnxruntime>=1.20.1,<1.24", | ||||||
| ] | ||||||
| audio_cuda12 = [ | ||||||
| "nemo_curator[audio_common]", | ||||||
| "nemo_curator[cuda12]", | ||||||
| "nvidia-cudnn-cu12", | ||||||
|
Contributor
There was a problem hiding this comment.
Please add a version pin or at minimum an upper bound (e.g.
Suggested change
|
||||||
| "onnxruntime-gpu>=1.20.1,<1.24", | ||||||
| "torchcodec", | ||||||
| ] | ||||||
|
Comment on lines
+105
to
115
Contributor
There was a problem hiding this comment.
The rest of this file's CUDA extras avoid this by having the CUDA variant include the CPU variant (e.g. Consider adding a conflicts declaration: [tool.uv.conflicts]
audio = [
{ extra = "audio_cpu" },
{ extra = "audio_cuda12" },
]This gives uv the information it needs to raise an error if a user tries to resolve both extras simultaneously, rather than silently producing a broken environment. |
||||||
|
|
||||||
| image_cpu = [ | ||||||
|
|
||||||
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.
Why make a distinct group for audio common? Audio cuda inherits from audio cpu
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.
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.
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.
Gotcha!