Skip to content

Paraview: uses canonical cuda_arch variant#23257

Merged
sethrj merged 1 commit intospack:developfrom
vicentebolea:fix-paraview-cuda-arch
Aug 17, 2021
Merged

Paraview: uses canonical cuda_arch variant#23257
sethrj merged 1 commit intospack:developfrom
vicentebolea:fix-paraview-cuda-arch

Conversation

@vicentebolea
Copy link
Copy Markdown
Member

@vicentebolea vicentebolea commented Apr 26, 2021 •
edited
Loading

This PR removes the custom variant cuda_arch from the ParaView pkg as this conflict with the cuda_arch variant provided by the CudaPackage super-class that we inherit.

From now on ParaView will only accept as args for cuda_arch the Cuda arch versions numbers. This is, instead of writing cuda_arch=volta you might write cuda_arch=70.

Signed-off-by: Vicente Adolfo Bolea Sanchez [email protected]

@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from bda37cb to 10d7c76 Compare April 26, 2021 16:22
@vicentebolea vicentebolea mentioned this pull request Apr 26, 2021
Closed
4 tasks
tldahlgren
tldahlgren reviewed Apr 26, 2021
View reviewed changes
tldahlgren
tldahlgren reviewed Apr 26, 2021
View reviewed changes
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from c7b4764 to 7f30084 Compare April 26, 2021 17:25
@psakievich psakievich mentioned this pull request Apr 27, 2021
Closed
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch 3 times, most recently from 231c70e to dcdeb80 Compare May 7, 2021 23:17
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch 2 times, most recently from 3672e6e to 6da0635 Compare May 8, 2021 00:16
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from 6da0635 to 9e32fe2 Compare May 21, 2021 00:58
elleryames
elleryames reviewed Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown

@elleryames elleryames left a comment

Choose a reason for hiding this comment

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

This PR fixes the issue I reported here: issue #22970.

@alalazo alalazo mentioned this pull request Jun 15, 2021
Merged
5 tasks
@vicentebolea vicentebolea mentioned this pull request Jul 13, 2021
Merged
@chuckatkins
Copy link
Copy Markdown

chuckatkins commented Aug 5, 2021

Does the package even need to have the string based variant in the spec at all? Can the package just rely on the inherited integer variant and map that accordingly to the string value that paraview needs to build?

chuckatkins
chuckatkins reviewed Aug 5, 2021
View reviewed changes
sethrj
sethrj reviewed Aug 11, 2021
View reviewed changes
alalazo
alalazo reviewed Aug 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

I just left a question and a minor comment. What would be needed to push this forward? Let me know if you need any help here.

@alalazo alalazo self-assigned this Aug 12, 2021
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from 9e32fe2 to 567af8b Compare August 13, 2021 00:16
@vicentebolea
Copy link
Copy Markdown
Member Author

vicentebolea commented Aug 13, 2021

@chuckatkins @alalazo after some exploratory work in the spack packages I can see that within Spack there is not a package which depends on Paraview with particular cuda_arch which is a string which was my main fear. For instance you can see this with:

git grep 'depends.*paraview.*cuda

Moreover while those flags are there, for newer ParaView releases we are still working to provide support of ParaView + VTKm + CUDA, we are not quite done yet. So at this point we do not really expect users to use this flag.

All in all, I vote for getting rid of the string cuda_arch variant, stick to the provided CudaPackage cuda_arch variant, and do this translation from the cuda_arch variant numeric value to the family name opaquely.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Aug 13, 2021

All in all, I vote for getting rid of the string cuda_arch variant, stick to the provided CudaPackage cuda_arch variant, and do this translation from the cuda_arch variant numeric value to the family name opaquely

👍

@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch 3 times, most recently from 746a41f to 23ff46b Compare August 13, 2021 23:22
@vicentebolea vicentebolea changed the title Change cuda_arch variant in Paraview package Paraview: uses canonical cuda_arch variant Aug 13, 2021
@vicentebolea vicentebolea requested a review from alalazo August 13, 2021 23:37
sethrj
sethrj previously approved these changes Aug 14, 2021
View reviewed changes
sethrj
sethrj reviewed Aug 14, 2021
View reviewed changes
alalazo
alalazo reviewed Aug 14, 2021
View reviewed changes
alalazo
alalazo requested changes Aug 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

I left a few comments above. Let me know if you need help or want to discuss anything.

@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from ce87dd6 to 3adecac Compare August 14, 2021 17:24
@vicentebolea vicentebolea requested a review from alalazo August 14, 2021 17:25
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from 3adecac to b577ff9 Compare August 14, 2021 17:31
vicentebolea
vicentebolea commented Aug 14, 2021
View reviewed changes
alalazo
alalazo reviewed Aug 16, 2021
View reviewed changes
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch 2 times, most recently from 5d2a617 to c791b82 Compare August 16, 2021 22:44
@vicentebolea
Paraview: uses canonical cuda_arch variant
38afb78 
cuda_arch in ParaView will not longer accept CUDA architecture names

Co-authored-by: Seth R. Johnson <[email protected]
Signed-off-by: Vicente Adolfo Bolea Sanchez <[email protected]>
@vicentebolea vicentebolea force-pushed the fix-paraview-cuda-arch branch from c791b82 to 38afb78 Compare August 16, 2021 22:56
@sethrj sethrj enabled auto-merge (squash) August 17, 2021 11:43
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Aug 17, 2021

@spackbot run pipeline

@spackbot-app
Copy link
Copy Markdown

spackbot-app bot commented Aug 17, 2021

I've started that pipeline for you!

@sethrj sethrj merged commit 4c0f1bf into spack:develop Aug 17, 2021
@vicentebolea vicentebolea removed the WIP label Aug 18, 2021
@vicentebolea vicentebolea mentioned this pull request Aug 18, 2021
Closed
@sethrj sethrj linked an issue Aug 18, 2021 that may be closed by this pull request
Installation issue: paraview #22970
Closed
4 tasks
@vicentebolea vicentebolea deleted the fix-paraview-cuda-arch branch July 14, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tldahlgren tldahlgren tldahlgren left review comments

@sethrj sethrj sethrj approved these changes

@alalazo alalazo alalazo approved these changes

+2 more reviewers

@chuckatkins chuckatkins chuckatkins left review comments

@elleryames elleryames elleryames left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@alalazo alalazo

Labels

conflicts cuda e4s ecp new-variant update-package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Installation issue: paraview

6 participants

@vicentebolea @chuckatkins @alalazo @sethrj @tldahlgren @elleryames