Skip to content

Thread execution spaces through vector API#630

Merged
Yurlungur merged 35 commits intomainfrom
jmm/thread-spaces
Apr 10, 2026
Merged

Thread execution spaces through vector API#630
Yurlungur merged 35 commits intomainfrom
jmm/thread-spaces

Conversation

@Yurlungur
Copy link
Copy Markdown
Collaborator

@Yurlungur Yurlungur commented Apr 9, 2026

PR Summary

This MR resolves issue #628 and pulls in lanl/ports-of-call#109 from ports-of-call. Here I expose the ability to select an execution space in the singularity-eos vector API:

  • Each vector API call now optionally accepts an execution space argument. The default is PortsOfCall::Exec::Device() which is (when Kokkos is enabled) an alias to kokkos default execution space.
  • To handle the huge number of overloads, I heavily leverage use macros to shrink the number of explicit declarations of vector methods #629 so that the changes are relatively minimal.
  • I did need to heavily use SFINAE/concepts to ensure that all the different template specializations played nice.
  • The python API now explicitly calls the PortsOfCall::Exec::Host() execution space.
  • As usual, EOSPAC is a bit of an odd one out. As far as I know, EOSPAC does not have the ability to select an execution space like this. For now, I simply document that explicitly requesting an execution space with EOSPAC is undefined behavior. It will do whatever the backend decides to do. We should resolve this at some point down the road in collaboration with the EOSPAC devs if we need to.

This MR used generative AI, but agents weren't set loose, so there's no proposed change plan. I made changes by hand and then had the robots copy my changes by example.

I am still getting the CI to pass, but this is ready for review.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI
  • If ML was used, make sure to add a disclaimer at the top of a file indicating ML was used to assist in generating the file.
  • If Agentic AI was used, have the AI generate a "proposed changes" markdown file and store it in the plan_histories folder, with a filename the same as the MR number.

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Maintainers: ensure spackages are up to date:
    • LANL-internal team, update XCAP spackages
    • Current maintainer of upstream spackages, submit MR to spack

@Yurlungur Yurlungur self-assigned this Apr 9, 2026
@Yurlungur Yurlungur added bug Something isn't working enhancement New feature or request labels Apr 9, 2026
std::string &phase_names, Verbosity eospacWarn) {
using namespace EospacWrapper;

EOS_INTEGER errorCode = EOS_OK;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

eliminates an unused variable warning


template <typename T,
typename = std::enable_if_t<variadic_utils::contains<T, Ts...>::value>>
template <typename T, typename EnableIfContained =
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Naming the SFINAE/concept type produced more useful error messages from the cuda compiler.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The diffs for the modifiers are a bit hard to read for some reason, but what they're doing is adding Space to each vector call, and then a defaulted version that calls PortsOfCall::Exec::Device().

@Yurlungur
Copy link
Copy Markdown
Collaborator Author

Tests on re-git now passing. But I would like to clean this up a bit further and eliminate more of these redundant overloads.

Copy link
Copy Markdown
Collaborator

@adamdempsey90 adamdempsey90 left a comment

Choose a reason for hiding this comment

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

Lots of boilerplate, so I didn't give it too close of a look

transform.y.apply(sie_unit_);
transform.f.apply(inv_temp_unit_);
t_.TemperatureFromDensityInternalEnergy(rhos, sies, temperatures, scratch, num,
t_.TemperatureFromDensityInternalEnergy(s, rhos, sies, temperatures, scratch, num,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While I think s is a fine name, a more descriptive name like exec_space might be even better

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you think that's necessary even though the type is named Space?

@Yurlungur
Copy link
Copy Markdown
Collaborator Author

Lots of boilerplate, so I didn't give it too close of a look

Still basically all boilerplate, but significantly reduced by adding more macros a la #629 but for modifier overloads.

@Yurlungur Yurlungur linked an issue Apr 9, 2026 that may be closed by this pull request
INFO("Pressure at rho_max: " << at_max << ", Pressure beyond rho_max"
<< beyond_max);
REQUIRE(at_max == beyond_max);
REQUIRE(isClose(at_max, beyond_max));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Caused a failure on GPUs on the CI. Not sure why it only showed up now.

@Yurlungur
Copy link
Copy Markdown
Collaborator Author

@adamdempsey90 ping for re-review given the macros I added.

@Yurlungur Yurlungur requested a review from adamdempsey90 April 9, 2026 23:30
Copy link
Copy Markdown
Collaborator

@adamdempsey90 adamdempsey90 left a comment

Choose a reason for hiding this comment

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

LGTM

@Yurlungur Yurlungur merged commit a75128b into main Apr 10, 2026
9 checks passed
@Yurlungur Yurlungur deleted the jmm/thread-spaces branch April 10, 2026 15:16
@Yurlungur Yurlungur mentioned this pull request Apr 12, 2026
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python bindings in mixed CPU-gpu environment

3 participants