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.

Sign up for GitHub

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

Jump to bottom

Package request: cuSolverMp (add Conda packages) #28294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Package request: cuSolverMp (add Conda packages) #28294

wants to merge 1 commit into from
+156 −0

Conversation

billysuh7
Copy link

@billysuh7 billysuh7 commented Nov 20, 2024 •
edited
Loading

#27809

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

cuSolverMP only supports CUDA 12 on linux-64 and linux-aarch64. Please ignore CI failures for all other variants. I will have a followup PR to rerender CI config and enable aarch64. Thanks.

@github-actions GitHub Actions
Copy link

Hi! This is the staged-recipes linter and your PR looks excellent! 🚀

@conda-forge-admin
Copy link
Contributor

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 (recipes/cusolvermp/meta.yaml, recipes/libcusolvermp/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/libcusolvermp/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). Your recipe may not receive automatic updates and/or may not be compatible with conda-forge's infrastructure. Please check the logs for more information and ensure your recipe can be parsed.

@billysuh7 billysuh7 marked this pull request as ready for review November 21, 2024 15:05
@billysuh7
Copy link
Author

@adibbley @jakirkham @carterbox , I appreciate your review.

adibbley
adibbley reviewed Nov 21, 2024
View reviewed changes
recipes/libcusolvermp/meta.yaml
Comment on lines +48 to +55
- libcusolver # [(cuda_compiler_version or "").startswith("12")]
- cuda-cudart # [(cuda_compiler_version or "").startswith("12")]
- libcublas # [(cuda_compiler_version or "").startswith("12")]
- libcal {{ libcal_version }}
run:
- libcusolver # [(cuda_compiler_version or "").startswith("12")]
- cuda-cudart # [(cuda_compiler_version or "").startswith("12")]
- libcublas # [(cuda_compiler_version or "").startswith("12")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only support CUDA12 does it make sense to clean this up?

Suggested change
- libcusolver # [(cuda_compiler_version or "").startswith("12")]
- cuda-cudart # [(cuda_compiler_version or "").startswith("12")]
- libcublas # [(cuda_compiler_version or "").startswith("12")]
- libcal {{ libcal_version }}
run:
- libcusolver # [(cuda_compiler_version or "").startswith("12")]
- cuda-cudart # [(cuda_compiler_version or "").startswith("12")]
- libcublas # [(cuda_compiler_version or "").startswith("12")]
- libcusolver
- cuda-cudart
- libcublas
- libcal {{ libcal_version }}
run:
- libcusolver
- cuda-cudart
- libcublas

carterbox
carterbox requested changes Nov 21, 2024
View reviewed changes
recipes/libcusolvermp/meta.yaml
{% set cuda_major = environ.get("cuda_compiler_version", "12.6").split(".")[0] %}

package:
name: libcusolvermp
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: libcusolvermp
name: libcusolvermp-split

Use a non-output toplevel name and then add the feedstock-name extra at the bottom.

recipes/libcusolvermp/meta.yaml
Comment on lines +116 to +118
extra:
recipe-maintainers:
- conda-forge/cuda
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extra:
recipe-maintainers:
- conda-forge/cuda
extra:
feedstock-name: libcusolvermp
recipe-maintainers:
- conda-forge/cuda

https://conda-forge.org/docs/maintainer/adding_pkgs/#feedstock-name

recipes/libcusolvermp/meta.yaml
Comment on lines +27 to +30
test:
commands:
- test -L $PREFIX/lib/libcusolverMp.so.0
- test -f $PREFIX/lib/libcusolverMp.so.{{ lib_version }}
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the appropriate outputs section once the top-level name does not duplicate one of the outputs.

recipes/libcusolvermp/meta.yaml
Comment on lines +48 to +50
- libcusolver # [(cuda_compiler_version or "").startswith("12")]
- cuda-cudart # [(cuda_compiler_version or "").startswith("12")]
- libcublas # [(cuda_compiler_version or "").startswith("12")]
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we using the "-dev" packages? Because CUDA enhanced compatibility and we pin cuda-version in the run deps?

recipes/libcusolvermp/meta.yaml
version: {{ version }}

source:
url: https://developer.download.nvidia.com/compute/cusolvermp/redist/libcusolvermp/{{ platform }}/libcusolvermp-{{ platform }}-{{ version }}_cuda{{ cuda_major }}-archive.{{ extension }}
Copy link
Member

Choose a reason for hiding this comment

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

Please add conda-forge.yml with the following contents to the libcusolvermp directory:

os_version:
  linux_64: cos7
  linux_aarch64: cos7
  linux_ppc64le: cos7

Because this is a redist we want the os version of the container to have the oldest supported glibc version. In this case glibc 2.17 (unless that's not the case?)

recipes/libcusolvermp/meta.yaml
Comment on lines +61 to +71
about:
home: https://docs.nvidia.com/cuda/cusolvermp/
license: LicenseRef-NVIDIA-End-User-License-Agreement
license_file: LICENSE
license_url: https://docs.nvidia.com/cuda/cusolvermp/license/index.html
summary: NVIDIA cuSOLVERMp is a high-performance, distributed-memory, GPU-accelerated library that provides tools for the solution of dense linear systems and eigenvalue problems.
description: |
NVIDIA cuSOLVERMp is a high-performance, distributed-memory, GPU-accelerated library that provides tools for the solution of dense linear systems and eigenvalue problems.
cuSOLVERMp is compatible with 2D block-cyclic data layout and provides ScaLAPACK-like C APIs.
A companion library, CAL, contains utilities to manage communicators and to synchronize processes in a safe way.
doc_url: https://docs.nvidia.com/cuda/cusolvermp/
Copy link
Member

Choose a reason for hiding this comment

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

Once the top level is renamed to not collide with one of the outputs, these identical about sections are redundant because they will inherit the top-level descriptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adibbley adibbley adibbley left review comments

@carterbox carterbox carterbox requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@billysuh7 @conda-forge-admin @carterbox @adibbley