Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Experimental::LogicalMemorySpace #6557

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Oct 31, 2023

Proposed resolution for #5609
In short, coming to the conclusion that LogicalMemorySpace cannot achieve its design goal (that is a debugging facility for accessibility issues on systems without a discrete GPU) and suggesting to just remove it.
The code sits in the Experimental:: namespace. I am not aware of any serious use of this facility.

I did a quick search on GH and AFAICT the only code we'd break is kokkos-remote-spaces but we could fix it ourselves. It looks like Kokkos-resillience would not be affected by that change. @janciesko @nmm0

Let me know if you think I am too aggressive and that we should consider going through deprecation.

@dalg24
Copy link
Member Author

dalg24 commented Oct 31, 2023

@PhilMiller voiced an objection to this in #6555 (comment)
I expect he meant to comment here so I am pasting the comment below

The logical space stuff was actually useful for testing code in the DARMA serialization library Magistrate. Specifically, we wanted to be able to write tests that we could instantiate code that would work for CudaSpace view instances and never access them other than via deep_copy, without needing a Cuda compiler to build and run the tests. So, consider this a request not to go ahead with removing that.

@PhilMiller
Copy link
Contributor

Thanks for copying that over - I was on my phone and hadn't seen this PR put up yet.

Magistrate is here: https://github.com/DARMA-tasking/magistrate/
The test code fragment that you quoted is actually present in https://github.com/DARMA-tasking/magistrate/blob/develop/tests/unit/test_kokkos_serialize_special.cc

I realize it's disabled at the moment, but as I understand (having stepped away from the project) there are broader design changes being done in the stuff that it would be testing. It should still be applicable. One of the longer-standing NGA folks should be able to follow up on that point.

@nmm0
Copy link
Contributor

nmm0 commented Oct 31, 2023

Pinging @ElisabethGiem as this might be of interest for her due to her work with resilient execution spaces

@fnrizzi
Copy link
Contributor

fnrizzi commented Nov 6, 2023

@cz4rs since you worked a bit on magistrate, any comments on this?

@masterleinad
Copy link
Contributor

I would prefer to just deprecate. After all, the class is pretty isolated and it's not obvious that its existence would impact any refactoring or cause any other problems.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

We agreed in the team meeting to remove LogicalMemorySpace without deprecating it. The removal looks fine to me.

@dalg24 dalg24 merged commit 8e16df3 into kokkos:develop Dec 7, 2023
28 of 30 checks passed
@dalg24 dalg24 deleted the rm_logical_spaces branch December 7, 2023 03:20
@dalg24 dalg24 mentioned this pull request Dec 7, 2023
dalg24 added a commit to dalg24/magistrate that referenced this pull request Dec 7, 2023
since Kokkos::Experimental::LogicalMemorySpace was removed in
kokkos/kokkos#6557
JacobDomagala pushed a commit to dalg24/magistrate that referenced this pull request Feb 6, 2024
JacobDomagala pushed a commit to dalg24/magistrate that referenced this pull request Feb 26, 2024
JacobDomagala pushed a commit to dalg24/magistrate that referenced this pull request Feb 27, 2024
JacobDomagala pushed a commit to dalg24/magistrate that referenced this pull request Mar 6, 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.

None yet

6 participants