Skip to content

arborx: inherit from CudaPackage, ROCmPackage#30490

Merged
alalazo merged 1 commit intospack:developfrom
eugeneswalker:arborx-cuda-rocm-package
May 6, 2022
Merged

arborx: inherit from CudaPackage, ROCmPackage#30490
alalazo merged 1 commit intospack:developfrom
eugeneswalker:arborx-cuda-rocm-package

Conversation

@eugeneswalker
Copy link
Copy Markdown
Contributor

@eugeneswalker eugeneswalker commented May 4, 2022 •
edited
Loading

@eugeneswalker eugeneswalker added cuda e4s ROCm/hip Support for ROCm/hip labels May 4, 2022
@spackbot-app spackbot-app bot requested a review from aprokop May 4, 2022 23:37
tldahlgren
tldahlgren approved these changes May 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

LGTM. Will defer decision regarding retaining and merging this PR to a maintainer.

@tldahlgren tldahlgren self-assigned this May 5, 2022
aprokop
aprokop reviewed May 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

Inheriting from CudaPackage and ROCmPackage makes sense to me. I'm less certain about adding arch code. I guess a person only interested in ArborX would benefit from writing spack install arborx+cuda cuda_arch=<blah> vs spack install arborx+cuda ^kokkos cuda_arch=<blah>.

@aprokop
Copy link
Copy Markdown
Contributor

aprokop commented May 5, 2022

@masterleinad what do you think?

@eugeneswalker
Copy link
Copy Markdown
Contributor Author

eugeneswalker commented May 5, 2022

Inheriting from CudaPackage and ROCmPackage makes sense to me. I'm less certain about adding arch code. I guess a person only interested in ArborX would benefit from writing spack install arborx+cuda cuda_arch=<blah> vs spack install arborx+cuda ^kokkos cuda_arch=<blah>.

Just from my perspective, I think it's nice to not have to explicitly say ^kokkos +cuda cuda_arch= whenever installing arborx. If I don't do that, the cuda_arch is selected for me, and I think by default it just uses the latest cuda_arch, which is rarely what I want.

@tldahlgren
Copy link
Copy Markdown
Contributor

tldahlgren commented May 5, 2022

Inheriting from CudaPackage and ROCmPackage makes sense to me. I'm less certain about adding arch code. I guess a person only interested in ArborX would benefit from writing spack install arborx+cuda cuda_arch=<blah> vs spack install arborx+cuda ^kokkos cuda_arch=<blah>.

Just from my perspective, I think it's nice to not have to explicitly say ^kokkos +cuda cuda_arch= whenever installing arborx. If I don't do that, the cuda_arch is selected for me, and I think by default it just uses the latest cuda_arch, which is rarely what I want.

I agree. From a "usability" standpoint, IMO it is better to not required people not familiar with your software and its dependencies to know they have to be explicit in this case.

@alalazo alalazo merged commit c2afb4b into spack:develop May 6, 2022
@streeve streeve mentioned this pull request Sep 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tldahlgren tldahlgren tldahlgren approved these changes

+1 more reviewer

@aprokop aprokop aprokop approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tldahlgren tldahlgren

Labels

cuda dependencies e4s ROCm/hip Support for ROCm/hip update-package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eugeneswalker @aprokop @tldahlgren @alalazo