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

docs: Update the CudaPackage (build system) description #20742

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

tldahlgren
Copy link
Contributor

This PR expands on the CudaPackage description by:

  • elaborating on the variants;
  • adding relevant URLs/links;
  • adding a discussion of conflicts with examples;
  • adding a description of the cuda_flags method; and
  • correcting and expanding on the Usage section.

@tldahlgren tldahlgren added documentation Improvements or additions to documentation cuda labels Jan 8, 2021
@tldahlgren tldahlgren self-assigned this Jan 8, 2021
@tgamblin tgamblin requested a review from ax3l January 8, 2021 03:27
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you for improving the docs of this! ✨

lib/spack/docs/build_systems/cudapackage.rst Outdated Show resolved Hide resolved
lib/spack/docs/build_systems/cudapackage.rst Outdated Show resolved Hide resolved
lib/spack/docs/build_systems/cudapackage.rst Show resolved Hide resolved
lib/spack/docs/build_systems/cudapackage.rst Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Member

@Rombur would be a good person to review too

if cuda_arch != 'none':
options.append('-DCUDA_FLAGS=-arch=sm_{0}'.format(cuda_arch[0]))
args.append('-DCUDA_FLAGS=-arch=sm_{0}'.format(cuda_arch))
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using cuda_flags here? The base class has a static method for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base class' cuda_flags returns a list of '--generate-code arch=compute_{0},code=sm_{0} --generate-code arch=compute_{0},code=compute_{0}'.format(s) for s in the list of arch's passed it. Is that equivalent when the flags are specified this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the dealii package has a comment that it has compiler errors when using the values returned from cuda_flags (see https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/dealii/package.py).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dihydrogen package only uses -arch=sm_60 (for its CMAKE_CUDA_FLAGS macro) when there is a single architecture with the value auto.

@tldahlgren
Copy link
Contributor Author

@Rombur @adamjstewart Are these changes sufficient under the circumstances?

@adamjstewart
Copy link
Member

Docs look good to me. I know very little about CUDA, I just use it for PyTorch. One thing I might suggest adding is an example where you determine the cuda_arch for your GPU and set it globally in packages.yaml.

As for the CudaPackage base class, I've been thinking this for a while now, but doesn't it make more sense to move the compiler-/platform-specific conflicts to the cuda package? That way they will always be used even if a package doesn't inherit from CudaPackage.

@tldahlgren
Copy link
Contributor Author

Docs look good to me. I know very little about CUDA, I just use it for PyTorch. One thing I might suggest adding is an example where you determine the cuda_arch for your GPU and set it globally in packages.yaml.

As I mentioned in the new ROCmPackage doc, I'm hesitant to add something like this to here since there is a risk it could cause issues in some environments. In that case, I referred to comments in the package (with a link to the package).

Unfortunately, the CudaPackage doesn't have comments with that advice.

As for the CudaPackage base class, I've been thinking this for a while now, but doesn't it make more sense to move the compiler-/platform-specific conflicts to the cuda package? That way they will always be used even if a package doesn't inherit from CudaPackage.

I'm not sure I follow the issue here. What do you mean by the cuda package? The cuda.py (build system) defines CudaPackage.

@adamjstewart
Copy link
Member

What do you mean by the cuda package?

var/spack/repos/builtin/packages/cuda/package.py

CudaPackage is just a set of common mixins for CUDA things. But not every package that uses CUDA uses CudaPackage or needs the variants. If we move this stuff to the package recipe, it will always be used for every package.

@tldahlgren
Copy link
Contributor Author

What do you mean by the cuda package?

var/spack/repos/builtin/packages/cuda/package.py

Ah! Guess my head is buried too much in the core.

CudaPackage is just a set of common mixins for CUDA things. But not every package that uses CUDA uses CudaPackage or needs the variants. If we move this stuff to the package recipe, it will always be used for every package.

Thanks for the clarification.

@tldahlgren
Copy link
Contributor Author

@adamjstewart Since Rombur approved this PR, is the content sufficient for now under the assumption it can be updated, as needed, should any of the changes you mentioned be performed?

@tldahlgren tldahlgren merged commit 6b13909 into spack:develop Jan 21, 2021
@tldahlgren tldahlgren deleted the docs/update-cudapackage branch January 21, 2021 20:43
@tldahlgren
Copy link
Contributor Author

Thanks @adamjstewart

@tldahlgren tldahlgren added this to In progress in Spack v0.16.1 release via automation Feb 3, 2021
@tldahlgren tldahlgren added this to In progress in Spack 0.17.0 Release via automation Feb 4, 2021
@tldahlgren tldahlgren removed this from In progress in Spack v0.16.1 release Feb 4, 2021
@alalazo alalazo moved this from In progress to Done in Spack 0.17.0 Release Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda documentation Improvements or additions to documentation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants