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

Specify that cuda provides cublas #19269

Closed
wants to merge 1 commit into from

Conversation

RobertRosca
Copy link
Contributor

As mentioned on the nvidia documentation page:

Using cuBLAS, applications automatically benefit from regular performance improvements and new GPU architectures. The cuBLAS library is included in both the NVIDIA HPC SDK and the CUDA Toolkit.

Since CUDA provides cuBLAS, I think it might be worth explicitly adding this to the package file so it could be used as a virtual dependency. The versions of cublas provided by cuda are taken from the 'Release Notes' section of the archived documentation here: https://docs.nvidia.com/cuda/archive/

To be honest I'm not sure if this PR is 'correct'. The documentation mentions virtual dependencies in the context of multiple packages providing the same dependency (e.g. MPI), but in this case it's one package providing multiple dependencies (since CUDA contains a lot of individual components).

Actually this is a broader question: in situations like this, where the package you're defining is a toolkit comprising a lot of individual components, should you try to list all of the contained tools as providers?

@alalazo
Copy link
Member

alalazo commented Oct 12, 2020

@svenevs @ax3l

@svenevs
Copy link
Member

svenevs commented Oct 12, 2020

I'm not super familiar with how the spack providers work, but

  1. If we can get away with just a single provides("cublas") without any versions we should. You can't mix different versions of the toolkit, but I don't know if that means versions are needed or not needed for the concretizer.
  2. We probably only need to care about cuBLAS, the other libraries are not really a replacement for anything as far as I know AKA unless there's a specific need to mark it we shouldn't.

Edit: maybe it should be provides("blas")?

@adamjstewart
Copy link
Member

@Rombur may want to review

There's no need for a virtual provider if no other package provides cublas. Maybe it should be provides('blas') if it can be used interchangeably with other BLAS libs?

@Rombur
Copy link
Contributor

Rombur commented Oct 12, 2020

I am not sure I understand what's the point of doing this? Are there cases where you want to have a dependency on cuBLAS but you don't want an explicit dependency on CUDA?

Maybe it should be provides('blas') if it can be used interchangeably with other BLAS libs?

It cannot. The name of the functions are different (they are preceded by cublas) and some APIs are slightly different. This is not like switching OpenBLAS with Atlas for example.

@ax3l
Copy link
Member

ax3l commented Oct 14, 2020

Thank you for your PR! 🎉
Please do not be discouraged from my answer :)

We can document the bundled dependencies in CUDA, but I am not 100% sure we will be up to maintain this sufficiently. Here is a list of what is bundled in the CUDA Toolkit today:
https://docs.nvidia.com/cuda/cuda-toolkit-release-notes/index.html#cuda-major-component-versions

CUDA is going to release at improved speed over the next years: with rolling ~bi-monthly (?) NVIDIA HPC Software Development Kit (SDK) releases that bundle all software. There are some open source libraries like thrust, cub and libcu++ among others in that stack that are open source and contributable and might be worth shipping extra - but I am not sure if we want to mirror all them here and do book keeping for each cuda release.

Unfortunately, I am also not 100% sure what feature we could address from exposing cuBLAS separately - if something is bugged or needs an other version there will soon be plenty of NVIDIA HPC Software Development Kit (SDK) releases we can move to.

As you already indicated, my personal impression is also to maybe restrain from adding this specific book keeping feature.

@RobertRosca
Copy link
Contributor Author

I am not sure I understand what's the point of doing this? Are there cases where you want to have a dependency on cuBLAS but you don't want an explicit dependency on CUDA?

The motivation for me wasn't really a technical one, it was more that I think it would help clarify the purpose and of a dependency inside a package specification.

If you have, a package that can be built with some optional cuBLAS support right now you would just have depends_on('CUDA') or depends_on('CUDA@10.2.89'). However that package actually depends on cuBLAS@10.2.2, which isn't explicitly mentioned or written down anywhere in the package specification which feels a bit wrong to me on the conceptual side of things.

But you're right that on the 'technical' side this is a non-issue, as (with what I proposed here) a dependency on cuBLAS@10.2.2 would just be an alias for CUDA@10.2.89 so there would be no difference between the two. The only minor benefit is that it's a bit clearer what the actual dependency you want is.

Also, I think that these kind of questions will probably end up coming up more as Spack keeps growing, so finding a way to handle toolkit packages which bundle together multiple bits of software internally would be nice.

Please do not be discouraged from my answer :)

No problem! I wasn't very sure that this was a good idea to begin with 😛

but I am not sure if we want to mirror all them here and do book keeping for each cuda release.
...
As you already indicated, my personal impression is also to maybe restrain from adding this specific book keeping feature.

Yeah displaying all of the bundled dependencies would get pretty messy pretty quickly, I also don't think that's the best approach, but on the other hand I think something would be done for this as well.

@ax3l
Copy link
Member

ax3l commented Oct 15, 2020

Thanks, so in the light of the discussed above: should we go with provides('cublas') without a version or try not to start this book keeping at all (close)? Otherwise thrust would definitely also need to go in and libcu++ and cub.

@adamjstewart
Copy link
Member

I wouldn't bother with provides('cublas') unless:

  1. multiple packages can provide cublas, or
  2. cublas versioning is different from cuda versioning and we want to be able to depends_on a specific version of cublas

@ax3l
Copy link
Member

ax3l commented Oct 21, 2020

Oh, I am afraid to report that all packaged versions in CUDA 11.0+ are rather random in minor and patch level...
https://docs.nvidia.com/cuda/cuda-toolkit-release-notes/index.html#title-new-features

But I also I think that 1. alone is good enough for now, mainly because it will be a lot of work to document all these and I think in practice people will document and depend dominantly on a bundled CUDA release when communicating their requirements. Let us also evaluate this in light of #19365.

If we have new volunteers to maintain and copy the above table for each release I am 👍 Maybe in a separate file? Let's do all or nothing, I would say.

@wyphan
Copy link
Contributor

wyphan commented Mar 2, 2022

@RobertRosca Revisiting this PR as I work on #29155, I think this is a good idea to track the cublas version numbers, but prolly not as a virtual package since that's kinda overkill IMHO. I'm thinking something along the line of a dict in the package.py file. Permission to adapt this PR there?

@RobertRosca
Copy link
Contributor Author

RobertRosca commented Mar 2, 2022

Sure @wyphan feel free to take over! Thanks 👍

Would you like me to close this PR so that you can work on it in your issue?

@wyphan
Copy link
Contributor

wyphan commented Mar 2, 2022

No need to close it yet. I'll let you know once the PR I'm currently working on #29155 gets merged.

@alalazo
Copy link
Member

alalazo commented May 5, 2022

I think we decided in community PRs that it is not worth to turn each CUDA library into a virtual (discussion on cudatoolkit vs. nvhpc), so closing this. Feel free to reopen if you think otherwise.

@alalazo alalazo closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants