Add NVIDIA TransformerEngine - #29403
Conversation
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
pstjohn
changed the title
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/transformer-engine/meta.yaml:
For recipes/transformer-engine/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13796775976. Examine the logs at this URL for more detail. |
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/transformer-engine/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/14780992719. Examine the logs at this URL for more detail. |
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
| - url: https://github.com/NVIDIA/cudnn-frontend/archive/refs/tags/v1.8.0.tar.gz | ||
| sha256: 5907bf8baa8f3b74ad430d54027aadd14a0a61166ae4b00c2daece0df384409e | ||
| folder: 3rdparty/cudnn-frontend |
There was a problem hiding this comment.
Is it ok to vendor cudnn-frontend here? I've packaged this at https://github.com/conda-forge/python-cudnn-frontend-feedstock and have been trying to get it to work (unsuccessfully) at #26296 (comment). But this solution looks much simpler 🙂
There was a problem hiding this comment.
Yeah that's a good question. In conversations with the TE team, they work pretty closely with cudnn-frontend and sometimes have odd tags or versions here. I also don't think the repo is set up to be able to pull these dependencies from a source outside 3rdparty. That might be something we could try to fix in subsequent versions though. cudnn-frontend is a header-only library though, so it should be fairly lightweight.
There was a problem hiding this comment.
From https://conda-forge.org/docs/maintainer/guidelines/#generalities
The source should not contain vendored code. If it does:
Package the vendored project separately and specify the needed dependency in the requirements section. Preferred if the vendored code is needed at runtime.
Allow the vendored code, but make sure the license files are included in the about.license field. Usually ok if it's only a build-time dependency (e.g. headers-only library)
There was a problem hiding this comment.
Revending headers for compile-time is OK. What's not OK is revending them at runtime. This package is shipping a copy of CUDNN in $PREFIX/targets.
There was a problem hiding this comment.
Ah, yes, that is a problem. I moved some headers around so the CMAKE command could find them, but didn't realize they were getting re-distributed. Since we're patching those cmake build flags anyways I'll try to point cmake to the right place to find them in these conda builds
There was a problem hiding this comment.
Well it's not easy to get the cmake command to find these in a different location. The alternative could just be to clean these up post-build, right? Let me try doing that.
|
@conda-forge/help-python, ready for review! |
|
cc @gforsyth (for review) |
gforsyth
left a comment
gforsyth
left a comment
There was a problem hiding this comment.
Hi, I have a few questions, largely because I'm not as familiar with this project, so apologies if the questions are off-base.
Overall I think this looks reasonable -- the build links don't appear to be valid, so I haven't been able to review the build logs, which should be looked at before this gets merged in.
Some other questions:
- This is for version 1.13, but I see upstream there are releases in the 2.x series -- is the plan to get this version in and then also add those later versions?
- In the
setup.pyupstream, there is support forjaxandpaddle, in addition topytorch. I think it's fine to keep things limited to one of those to start with, but it is worth considering if you want to rename the output to reflect that. It might be a little odd if there is (in the future) an explicittransformer_engine_jax, buttransformer_enginegets you thepytorchversion
| if [[ ${cuda_compiler_version} == 12.6 ]]; then | ||
| export TORCH_CUDA_ARCH_LIST="5.0;6.0;6.1;7.0;7.5;8.0;8.6;8.9;9.0+PTX" |
There was a problem hiding this comment.
Does this need to be this restrictive, or can we set 12.6 as the lower bound?
There was a problem hiding this comment.
i actually don't think we need this... unless I'm mistaken for some reason, this might have been leftover from the torch build recipe. Anyways, deleted and the build still seems to pass
| {% if cuda_major >= 12 %} | ||
| - cuda-driver-dev | ||
| - cuda-cudart-dev | ||
| - cuda-nvrtc-dev | ||
| - cuda-nvtx-dev | ||
| - cuda-nvml-dev | ||
| - libcublas-dev | ||
| - libcusparse-dev | ||
| - libcusolver-dev | ||
| - cudnn | ||
| - cuda-profiler-api | ||
| {% endif %} |
There was a problem hiding this comment.
| {% if cuda_major >= 12 %} | |
| - cuda-driver-dev | |
| - cuda-cudart-dev | |
| - cuda-nvrtc-dev | |
| - cuda-nvtx-dev | |
| - cuda-nvml-dev | |
| - libcublas-dev | |
| - libcusparse-dev | |
| - libcusolver-dev | |
| - cudnn | |
| - cuda-profiler-api | |
| {% endif %} | |
| - cuda-driver-dev | |
| - cuda-cudart-dev | |
| - cuda-nvrtc-dev | |
| - cuda-nvtx-dev | |
| - cuda-nvml-dev | |
| - libcublas-dev | |
| - libcusparse-dev | |
| - libcusolver-dev | |
| - cudnn | |
| - cuda-profiler-api |
Since cuda 11 builds are skipped anyway, I think we can remove the conditional here
There was a problem hiding this comment.
Yup, thanks -- cleaned up some of this logic
| - scipy | ||
| - pytorch_scatter |
There was a problem hiding this comment.
I don't see these as requirements upstream -- should they be here?
There was a problem hiding this comment.
whoops, probably leftover from another recipe. Removed!
| if [[ ${cuda_compiler_version} != "None" && "$target_platform" == linux-64 ]]; then | ||
| export FORCE_CUDA="1" | ||
| if [[ ${cuda_compiler_version} == 12.6 ]]; then | ||
| export TORCH_CUDA_ARCH_LIST="5.0;6.0;6.1;7.0;7.5;8.0;8.6;8.9;9.0+PTX" |
There was a problem hiding this comment.
And is this env-var used by TransformerEngine? Or is this for pytorch?
There was a problem hiding this comment.
Probably for pytorch; I actually suspect we could just delete this
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Yeah that's right. We may even need to add a 1.12 version; I think I may have overshot the oldest version folks are using. But we'd want to quickly add the 2.x series as well.
IIUC there's not any paddle support anymore? but I'll double check. Anyways yes; that's a good idea. We're going to hopefully clean up the packaging here to make these targets more obvious, but in python currently you use optional dependencies (i.e. |
This change doesn't need to be made upstream -- handling it with optional dependencies is the pattern of choice with I think what you've suggested re: *_core, *_pytorch makes sense to start with. You don't want to have to rugpull any early users by changing what the packages do, but it's an accepted pattern that the names in As an example, for the
|
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
|
Hi! This is the staged-recipes linter and your PR looks excellent but I have some suggestions. File-specific lints and/or hints:
|
|
@conda-forge-admin please rerender |
FYI, optional dependency groups might be coming to conda soon with this CEP, xref conda/conda#7502 (comment). So you could also just do |
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
jakirkham
left a comment
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Peter for the contribution! 🙏
Also thanks Gil and Wei for your reviews!
Had a couple suggestions below
| "-B", | ||
| build_dir, | ||
| - f"-DPython_EXECUTABLE={sys.executable}", | ||
| + "-DPython_EXECUTABLE=$PYTHON", |
There was a problem hiding this comment.
Think we can do this in a more Pythonic way
| + "-DPython_EXECUTABLE=$PYTHON", | |
| + "-DPython_EXECUTABLE={os.environ['PYTHON']}", |
Also what happens when sys.executable is here?
There was a problem hiding this comment.
sys.executable led to this failure during build:
CMake Error at /home/conda/staged-recipes/build_artifacts/transformer-engine_1745610886178/_build_env/share/cmake-4.0/Modules/FindPackageHandleStandardArgs.cmake:227 (message):
Could NOT find Python (missing: Interpreter Development.Module) (found
version "3.11.12")
There was a problem hiding this comment.
Think we can do this in a more Pythonic way
No oddly, I get the same missing: Interpreter Development.Module when I do f"-DPython_EXECUTABLE={os.environ['PYTHON']}",; at least locally. Not sure why these would be different?
| @@ -0,0 +1 @@ | |||
| export CUDA_HOME=$CONDA_PREFIX | |||
There was a problem hiding this comment.
Would check if CUDA_HOME is set and if so keep a copy
| export CUDA_HOME=$CONDA_PREFIX | |
| if [[ ! -z "${CUDA_HOME +x}" ]] | |
| then | |
| export CUDA_HOME_BACKUP="${CUDA_HOME}" | |
| fi | |
| export CUDA_HOME="${CONDA_PREFIX}" |
Then would add a separate deactivate script to restore the original value
if [[ ! -z "${CUDA_HOME_BACKUP+x}" ]]
then
export CUDA_HOME="${CUDA_HOME_BACKUP}"
unset CUDA_HOME_BACKUP
fi
| extra: | ||
| recipe-maintainers: | ||
| - pstjohn |
There was a problem hiding this comment.
| extra: | |
| recipe-maintainers: | |
| - pstjohn | |
| extra: | |
| feedstock-name: transformer-engine | |
| recipe-maintainers: | |
| - pstjohn |
| cp $PREFIX/include/cudnn*.h $PREFIX/targets/x86_64-linux/include | ||
|
|
||
| echo "Installing transformer-engine" | ||
| ${PYTHON} -m pip install . |
There was a problem hiding this comment.
Logs show that the pip package version metadata contains some kind of hash.
e.g. transformer_engine-1.13.0+6a31486c93
Is this intentional? Or is there an override variable you can set.
There was a problem hiding this comment.
Good catch; I think I can override this with NVTE_NO_LOCAL_VERSION. Will do
https://github.com/NVIDIA/TransformerEngine/blob/d33525249d08ee6b2c80fded1b326248a2a1cb77/build_tools/te_version.py#L15
| - cuda-driver-dev | ||
| - cuda-cudart-dev | ||
| - cuda-nvrtc-dev | ||
| - cuda-nvtx-dev | ||
| - cuda-nvml-dev | ||
| - libcublas-dev | ||
| - libcusparse-dev | ||
| - libcusolver-dev |
There was a problem hiding this comment.
There are various unused dependencies at runtime from these CUDA libraries. Are we using dlopen or statically linking? Or are the deps actually unused?
There was a problem hiding this comment.
Yeah good question; they might only be needed at build time. Would I move these from host to build then? I'm pretty sure pydantic just wasn't exercised during the import tests, added an additional import to hopefully hit that.
There was a problem hiding this comment.
It depends:
statically linked? This should only be cudart. Use the cudart-static package instead.
dlopen'd? Ignore run_exports from these packages and add any relevant constraints to `requirements/run_constrained'
Unused? Remove it.
None of these packages are build tools, so they shouldn't go in requirements/build.
There was a problem hiding this comment.
As discussed offline, we are assuming these packages are dlopen'd and will decide whether to make them optional at a later time.
| - url: https://github.com/NVIDIA/cudnn-frontend/archive/refs/tags/v1.8.0.tar.gz | ||
| sha256: 5907bf8baa8f3b74ad430d54027aadd14a0a61166ae4b00c2daece0df384409e | ||
| folder: 3rdparty/cudnn-frontend |
There was a problem hiding this comment.
Revending headers for compile-time is OK. What's not OK is revending them at runtime. This package is shipping a copy of CUDNN in $PREFIX/targets.
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
| @@ -0,0 +1,5 @@ | |||
| if [[ ! -z "${CUDA_HOME +x}" ]] | |||
There was a problem hiding this comment.
Latest build is failing tests on this line:
2025-05-01T17:46:08.8928287Z /home/conda/staged-recipes/build_artifacts/transformer-engine_1746110968067/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/etc/conda/activate.d/transformer-engine-activate.sh: line 1: ${CUDA_HOME +x}: bad substitution
There was a problem hiding this comment.
Autocomplete kept adding spaces 🤦♂️
Thought I removed them all from the suggestion, but this was missed here
| if [[ ! -z "${CUDA_HOME +x}" ]] | |
| if [[ ! -z "${CUDA_HOME+x}" ]] |
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
|
Thanks all! 🙏 |
7 participants
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).