Skip to content
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

cuml: Build CUDA 12 packages #5318

Merged
merged 44 commits into from
Jul 13, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 31, 2023

This PR builds libcuml and cuml conda packages using CUDA 12. Resolves #5234.

@vyasr vyasr added feature request New feature or request 2 - In Progress Currenty a work in progress non-breaking Non-breaking change labels Mar 31, 2023
@vyasr vyasr self-assigned this Mar 31, 2023
@github-actions github-actions bot added ci conda conda issue labels Mar 31, 2023
@vyasr vyasr added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Mar 31, 2023
@vyasr vyasr changed the title Build CUDA 12 packages of libraft Build CUDA 12 packages of libcuml Mar 31, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Mar 31, 2023

This PR needs to be updated to pull artifacts from rapidsai/raft#1388 once those are ready, and it needs a cumlprims PR as well.

@vyasr vyasr force-pushed the feat/cuda12_nvidia_build branch from ba8f9b0 to 0a44511 Compare April 6, 2023 21:41
@vyasr vyasr changed the title Build CUDA 12 packages of libcuml libcuml: Build CUDA 12 packages Apr 10, 2023
@vyasr vyasr changed the title libcuml: Build CUDA 12 packages cuml: Build CUDA 12 packages Apr 10, 2023
@dantegd
Copy link
Member

dantegd commented Jul 11, 2023

There was one pytest failure that seems relevant and we need to investigate it but it's not a build blocker to get packages ready. Opened issue #5497 and skipped the pytest temporarily so we can get CUDA 12 packages early for testing.

@dantegd dantegd marked this pull request as ready for review July 11, 2023 13:50
@dantegd dantegd requested review from a team as code owners July 11, 2023 13:50
@dantegd
Copy link
Member

dantegd commented Jul 11, 2023

Follow up comment about the prior failed pytest, it happened to be an OOM orthogonal to CUDA 12. @viclafargue submitted a fix in #5498 and I merged the fix into this PR as well.

@bdice bdice added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Jul 11, 2023
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome progress. Comments attached.

dependencies.yaml Outdated Show resolved Hide resolved
conda/recipes/cuml/conda_build_config.yaml Outdated Show resolved Hide resolved
conda/recipes/libcuml/conda_build_config.yaml Outdated Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
dependencies.yaml Show resolved Hide resolved
python/cuml/tests/test_pca.py Show resolved Hide resolved
Copy link
Contributor Author

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting super close!

conda/environments/all_cuda-120_arch-x86_64.yaml Outdated Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
Comment on lines 55 to 72
{% if cuda_major == "11" %}
- cuda-python ==11.7.1
- cudatoolkit
- libcublas {{ cuda11_libcublas_host_version }}
- libcublas-dev {{ cuda11_libcublas_host_version }}
- libcurand {{ cuda11_libcurand_host_version }}
- libcurand-dev {{ cuda11_libcurand_host_version }}
- libcusolver {{ cuda11_libcusolver_host_version }}
- libcusolver-dev {{ cuda11_libcusolver_host_version }}
- libcusparse {{ cuda11_libcusparse_host_version }}
- libcusparse-dev {{ cuda11_libcusparse_host_version }}
{% else %}
- cuda-python ==12.0.0
- libcublas-dev
- libcurand-dev
- libcusolver-dev
- libcusparse-dev
{% endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need the -dev packages for cuml? Shouldn't we only require the runtime packages? The dependency on -dev packages should be isolated to libcuml I believe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely recall that the -dev packages are needed from when I looked at this months ago. I think cuml might do something with raft that requires these libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember well, so did a commit trying it to check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we were able to remove this?

conda/recipes/libcuml/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more round of questions about math library dependencies.

conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
conda/recipes/libcuml/meta.yaml Show resolved Hide resolved
Comment on lines 58 to 61
- libcublas {{ cuda11_libcublas_host_version }}
- libcurand {{ cuda11_libcurand_host_version }}
- libcusolver {{ cuda11_libcusolver_host_version }}
- libcusparse {{ cuda11_libcusparse_host_version }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any of these CUDA math libraries need to be in host for cuml Python? Are they directly used by cuml Python or only indirectly through libcuml? If the latter, maybe we can get away with only specifying them in libcuml's recipe.

I wonder if these (non-dev) libraries are also remnants of the changes used when migrating to GitHub Actions before RAFT recipes were complete. It would surprise me if these are used directly in cuml Python, since they're not mentioned in python/CMakeLists.txt. However, I see that there are linkages in the installed packages: for example, ldd manifold/umap.cpytho-310-x86_64-linux-gnu.so shows libcufft.so.10, libcurand.so.10, libcusolver.so.11, libcublas.so.11, libcusparse.so.11.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this more, we probably want to remove these. Putting “runtime” (non-dev) libraries in the host section probably does nothing, except perhaps satisfy conda linking checks (which we can verify by looking at the warnings).

{% if cuda_major == "11" %}
- cuda-python ==11.7.1
- cudatoolkit
- libcublas {{ cuda11_libcublas_host_version }}
Copy link
Contributor

@bdice bdice Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is libcufft missing? If so we'd need to add it to conda_build_config.yaml for cuml, too. (See also my other comment about whether any of these libraries are needed in the Python packaging.)

Suggested change
- libcublas {{ cuda11_libcublas_host_version }}
- libcublas {{ cuda11_libcublas_host_version }}
- libcufft {{ cuda11_libcufft_host_version }}

- libcusparse {{ cuda11_libcusparse_host_version }}
{% else %}
- cuda-python ==12.0.0
- libcublas
Copy link
Contributor

@bdice bdice Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is libcufft missing? (Or can all these math libraries be removed from the Python packaging, see other comment?)

Suggested change
- libcublas
- libcublas
- libcufft

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hard work on this @dantegd! LGTM.

Copy link
Contributor Author

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good for me too (can't approve since it's my PR).

@dantegd
Copy link
Member

dantegd commented Jul 13, 2023

/merge

@rapids-bot rapids-bot bot merged commit c3d4dbb into rapidsai:branch-23.08 Jul 13, 2023
@jakirkham
Copy link
Member

Thanks everyone! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team conda conda issue Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cuML: CUDA 12 Conda Packages
8 participants