Skip to content

Conversation

@davpoolechem
Copy link
Contributor

@loriab and I were talking about the Psi4/GauXC build system yesterday, and we both agreed that something along the lines of this PR would be quite useful for the Psi4/GauXC interface build system. Specifically, it would be nice for GauXC to have a property/component which would return whether the given instance of GauXC was built with GPU support or not, which is what this PR does.

Let me know if anything could be done in a more kosher fashion, of course!

Copy link
Owner

@wavefunction91 wavefunction91 left a comment

Choose a reason for hiding this comment

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

Generally in favor of this, but it should probably just encompass all the propoerties we'll need to export. Essentially all the options. Also, per discussion with @loriab, I got to thinking that we may want to export some other things that (aren't currently) configurable, but might be in the future - things like GAUXC_CPU_XC_MAX_AM GAUXC_GPU_SNLINK_MAX_AM, etc

@davpoolechem
Copy link
Contributor Author

davpoolechem commented Mar 22, 2024

Generally in favor of this, but it should probably just encompass all the propoerties we'll need to export. Essentially all the options. Also, per discussion with @loriab, I got to thinking that we may want to export some other things that (aren't currently) configurable, but might be in the future - things like GAUXC_CPU_XC_MAX_AM GAUXC_GPU_SNLINK_MAX_AM, etc

Duly noted! I expanded the list of configurable properties. I think the list now includes all of the GauXC-specific configurable options, which I gathered from a combination of the README and the root CMakeLists.txt file. There were some options from the README, like CMAKE_CUDA_ARCHITECTURES and MAGMA_ROOT_DIR, that I wasn't sure whether to include or not, so I left them out for now. Happy to add them in upon request, however!

I'm admittedly not sure of what "potentially configurable" options might want to be added to the list, but I'll be happy to add them. I at least added the ones you mentioned here, setting the default to what I saw was the default MAX_AM value (6) for Gau2Grid in the code.

@davpoolechem davpoolechem marked this pull request as ready for review March 22, 2024 16:48
@loriab
Copy link

loriab commented Mar 22, 2024

GAUXC_USING_CUDA or GAUXC_HAS_CUDA are alternatives if GAUXC_ENABLE_CUDA seems wrongly editable and GAUXC_CUDA_ENABLED too confusable (I agree). No strong feelings, though. I can live with any.

EDIT: otoh, if these are the variables already set in cmake (either main build or gauxc-config.cmake), it's probably simpler to leave them as current name rather than switching around just b/c they're now on the target.

@davpoolechem
Copy link
Contributor Author

GAUXC_USING_CUDA or GAUXC_HAS_CUDA are alternatives if GAUXC_ENABLE_CUDA seems wrongly editable and GAUXC_CUDA_ENABLED too confusable (I agree). No strong feelings, though. I can live with any.

EDIT: otoh, if these are the variables already set in cmake (either main build or gauxc-config.cmake), it's probably simpler to leave them as current name rather than switching around just b/c they're now on the target.

Yeah, these are all set in CMake already (barring the "non-configurable" options @wavefunction91 brought up), so I'll probably just leave them as the current name.

Copy link
Owner

@wavefunction91 wavefunction91 left a comment

Choose a reason for hiding this comment

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

Overall, this is in good shape. I'll make another PR to cleanup per @loriab's comments re variable naming (actually, if you or she could make an issue to that affect, that would be great). Othertiwse, LGTM after changes - I'll approve CI once fixed

@davpoolechem davpoolechem force-pushed the dpoole34/properties-baseline branch from e5addbd to 76e4f53 Compare March 25, 2024 18:08
@davpoolechem
Copy link
Contributor Author

davpoolechem commented Mar 25, 2024

Overall, this is in good shape. I'll make another PR to cleanup per @loriab's comments re variable naming (actually, if you or she could make an issue to that affect, that would be great). Othertiwse, LGTM after changes - I'll approve CI once fixed

Thanks much! I believe my recent commit should address all of the suggestions that you raised previously. Once your PR for variable renaming is set up, I'll adjust the naming scheme here to match.

@wavefunction91
Copy link
Owner

@davpoolechem can you update this PR to include #103 so we can hit the ground running once it gets checked in?

@davpoolechem davpoolechem force-pushed the dpoole34/properties-baseline branch from 76e4f53 to 196996a Compare March 30, 2024 11:56
@davpoolechem
Copy link
Contributor Author

@davpoolechem can you update this PR to include #103 so we can hit the ground running once it gets checked in?

All right, should be done now!

@wavefunction91
Copy link
Owner

CI is failing because the GAUXC_HAS_XYZ variables aren't defaulted, so when you try to expand them, they can potentially expand to nothing.... Gotta love CMake... I'll PR some changes that should fix this once merged

@wavefunction91
Copy link
Owner

Merging #114 should fix the configure error (it does locally at least)

@davpoolechem davpoolechem force-pushed the dpoole34/properties-baseline branch from 196996a to 3150481 Compare April 3, 2024 12:29
@wavefunction91
Copy link
Owner

@davpoolechem Per offline discussion, lets punt on the CMake UTs with these variables. I'll add an issue to track

@wavefunction91 wavefunction91 merged commit 2f9d022 into wavefunction91:master Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants