Fix numba-cuda and cuda-python installation and usage#15506
Conversation
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
requirements/requirements.txt
Outdated
| huggingface_hub>=0.24 | ||
| numba ; platform_system == 'Darwin' | ||
| numba-cuda==0.15.1 ; platform_system != 'Darwin' | ||
| numba-cuda[cu13] ; platform_system != 'Darwin' |
There was a problem hiding this comment.
Thinking from first principles:
- we have CU13 users that have cutting edge hardware / want cutting edge perf
- we have CU12 users that don't have cutting edge hardware
- we likely have CU11 users too
- we have CPU-only env users, also on platforms like MacOS
How do we cater to all of them? Sane default:
pip install nemo-toolkit- does not pull any CUDA depedencies; the codebase treats all of them as optional and raises an informative error about what to install if you try to use them
CUDA version is a bit more problematic because it depends on what pytorch was built against. We can't really control this in a robust way. So let's give options. Is it possible to do sth like this?
pip install nemo-toolkit[cu13]- pulls CUDA 13 depspip install nemo-toolkit[cu12]- pulls CUDA 12 depspip install nemo-toolkit[cu11]- pulls CUDA 11 deps
There was a problem hiding this comment.
FWIW Curator also currently only supports CUDA 12, and Curator depends on this. So +1-ing that recommendation, Curator can then depend on nemo-toolkit[cu12]
There was a problem hiding this comment.
Regarding CUDA 11.x - I think it is ok if the users of CUDA 11.x will install extra dependencies manually.
I'm unsure about the best numba version for this case, but latest versions do not support 11.x.
For cuda-python, we require at least 12.3.x, better to use 12.6.x. So, for CUDA 11.x it is useless.
Since both libraries are optional, let's for now keep only cu12 and cu13 extras.
There was a problem hiding this comment.
Since both libraries are optional, let's for now keep only cu12 and cu13 extras.
OK
nithinraok
left a comment
There was a problem hiding this comment.
can we add cuda-python to the reqs
(nemo/core/utils/cuda_python_utils.py)
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
numba-cuda and cuda-python installation and usage
Signed-off-by: artbataev <artbataev@users.noreply.github.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
| """ | ||
|
|
||
| __all__ = ["KENLM_AVAILABLE", "K2_AVAILABLE", "TRITON_AVAILABLE", "kenlm_required", "k2_required", "triton_required"] | ||
| __all__ = [ |
There was a problem hiding this comment.
Now I'm wondering if we should have API like this instead:
k2 = ext_k2.maybe_import() # returns python module or None
@ext_k2.required()
def foo(): ...
if ext_k2.available(): ...possibly implemented like this:
class OptionalDependency:
NAME = ""
INSTALATION_MESSAGE = ""
def required(self): ... # moved from _lib_required
def available(self): ... # moved from is_lib_available
class ext_k2(OptionalDependency):
NAME = "k2"
INSTALLATION_MESSAGE = "..."There was a problem hiding this comment.
k2 = ext_k2.maybe_import()
I initially thought about this, but this approach imposes makes imports more strict. You cannot anymore use from k2 import ... etc
There was a problem hiding this comment.
class OptionalDependency:
NAME = ""
INSTALATION_MESSAGE = ""
def required(self): ... # moved from _lib_required
def available(self): ... # moved from is_lib_available
class ext_k2(OptionalDependency):
NAME = "k2"
INSTALLATION_MESSAGE = "..."This approach has more overhead for required due to dynamic availability check. I prefer to check once -> create a constant.
There was a problem hiding this comment.
OK, maybe I'm overthinking this :)
requirements/requirements.txt
Outdated
| huggingface_hub>=0.24 | ||
| numba ; platform_system == 'Darwin' | ||
| numba-cuda[cu13] ; platform_system != 'Darwin' | ||
| numba-cuda ; platform_system != 'Darwin' |
There was a problem hiding this comment.
Should this be removed from main requirements? Doesn't it conflict with lines added by cu12/cu13?
setup.py
Outdated
| extras_require['all'] = list(chain(val for key, val in extras_require.items())) | ||
|
|
||
| # CUDA version extras (not included in 'all' - user must explicitly select) | ||
| extras_require['cu12'] = [ |
There was a problem hiding this comment.
For consistency, can we move to requirements_cu12.txt (same with cu13)?
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
| from nemo.utils import logging, logging_mode | ||
|
|
||
| if NUMBA_CUDA_AVAILABLE: | ||
| from nemo.collections.asr.parts.numba.spec_augment import SpecAugmentNumba, spec_augment_launch_heuristics |
There was a problem hiding this comment.
Note to myself: remove these as we have a fully vectorized pytorch native implementation that's faster for a while now.
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
# Conflicts: # README.md
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Signed-off-by: artbataev <artbataev@users.noreply.github.com>
|
/claude review |
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
|
/claude review |
Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
|
/claude review |
|
[🤖]: Hi @artbataev 👋, We wanted to let you know that a CICD pipeline for this PR just finished successfully. So it might be time to merge this PR or get some approvals. |
pzelasko
left a comment
There was a problem hiding this comment.
Excellent work @artbataev!
* Fix int type in grid * Adjust numba and cuda-python requirements * Add cuda-python guards to optional libs * Use cuda-python guards * Add guards for numba Signed-off-by: Vladimir Bataev <vbataev@nvidia.com> --------- Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
…15540) * Fix int type in grid * Adjust numba and cuda-python requirements * Add cuda-python guards to optional libs * Use cuda-python guards * Add guards for numba --------- Signed-off-by: Vladimir Bataev <vbataev@nvidia.com>
Important
The
Update branchbutton must only be pressed in very rare occassions.An outdated branch is never blocking the merge of a PR.
Please reach out to the automation team before pressing that button.
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Collection: [Note which collection this PR will affect]
Changelog
Usage
# Add a code snippet demonstrating how to use thisGitHub Actions CI
The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.
The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".
Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information