Skip to content

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

Merged
tldahlgren merged 3 commits intospack:developfrom
tldahlgren:docs/update-cudapackage
Jan 21, 2021
Merged

docs: Update the CudaPackage (build system) description#20742
tldahlgren merged 3 commits intospack:developfrom
tldahlgren:docs/update-cudapackage

Conversation

@tldahlgren
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren commented Jan 8, 2021

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
@tldahlgren tldahlgren mentioned this pull request Jan 8, 2021
Merged
@tgamblin tgamblin requested a review from ax3l January 8, 2021 03:27
ax3l
ax3l reviewed Jan 8, 2021
View reviewed changes
Copy link
Copy Markdown
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! ✨

@ax3l ax3l mentioned this pull request Jan 8, 2021
Merged
@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jan 8, 2021

@Rombur would be a good person to review too

adamjstewart
adamjstewart reviewed Jan 8, 2021
View reviewed changes
lib/spack/docs/build_systems/cudapackage.rst
cuda_arch = cuda_arch_list[0]
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
Copy Markdown
Member

@adamjstewart adamjstewart Jan 8, 2021

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
Copy Markdown
Contributor Author

@tldahlgren tldahlgren Jan 20, 2021

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
Copy Markdown
Contributor Author

@tldahlgren tldahlgren Jan 20, 2021

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
Copy Markdown
Contributor Author

@tldahlgren tldahlgren Jan 20, 2021

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 tldahlgren force-pushed the docs/update-cudapackage branch from f5e4d58 to 5cd7807 Compare January 20, 2021 19:05
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jan 21, 2021

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

@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jan 21, 2021

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
Copy Markdown
Contributor Author

tldahlgren commented Jan 21, 2021

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
Copy Markdown
Member

adamjstewart commented Jan 21, 2021

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
Copy Markdown
Contributor Author

tldahlgren commented Jan 21, 2021

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
Copy Markdown
Contributor Author

tldahlgren commented Jan 21, 2021

@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
Copy Markdown
Contributor Author

tldahlgren commented Jan 21, 2021

Thanks @adamjstewart

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

Reviewers

@ax3l ax3l ax3l left review comments

@adamjstewart adamjstewart adamjstewart approved these changes

@tgamblin tgamblin Awaiting requested review from tgamblin

@alalazo alalazo Awaiting requested review from alalazo

+1 more reviewer

@Rombur Rombur Rombur approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tldahlgren tldahlgren

Labels

cuda documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tldahlgren @adamjstewart @Rombur @ax3l @alalazo