Add support for execution spaces instances #209
Conversation
…streams and rename Pthreads to Threads
…pace and move kokkos.initialize() here
…stance as member variable
…ead of creating new one and remove fence call
…gument to constructor
| s1 = cp.cuda.Stream() | ||
| s2 = cp.cuda.Stream() | ||
|
|
||
| instance1 = pk.ExecutionSpaceInstance(space, s1) |
There was a problem hiding this comment.
ExecutionSpaceInstance => ExecutionSpace
There was a problem hiding this comment.
if we are instantiating a stream on top of a space, we could also go with space.make(stream). (And potentially have also a version that does not accept a stream but uses a new stream for each call.)
There was a problem hiding this comment.
That does seem like a better interface. I'll address this in a new PR
| if __name__ == "__main__": | ||
| space = pk.Cuda | ||
|
|
||
| s1 = cp.cuda.Stream() |
There was a problem hiding this comment.
Should we use pk.Stream to avoid importing cupy
There was a problem hiding this comment.
The motivation here is that users might have pre-existing CuPy code they want to integrate with pykokkos, and this example shows how.
There was a problem hiding this comment.
but the example only uses cupy to create streams not to use cupy arrays; would be good to then use cupy for all parts
| @@ -311,7 +312,7 @@ def generate_call(operation: str, functor: str, members: PyKokkosMembers, tag: c | |||
| call += f"}} else {{ {custom_call} }}" | |||
|
|
|||
| call += generate_copy_back(members) | |||
| call += generate_fence_call() | |||
| # call += generate_fence_call() | |||
There was a problem hiding this comment.
including a comment to explain the reason this is commented out would be good
| class ExecutionSpaceInstance: | ||
| """ | ||
| An instance of the execution space, corresponding to Cuda/HIP |
There was a problem hiding this comment.
If this only corresponds to streams, should we call it that way?
There was a problem hiding this comment.
Technically users can create an instance for OpenMP (which doesn't have streams), although I don't think it does anything right now. Future execution spaces might have use for this and they might not use the terminology "Streams".
There was a problem hiding this comment.
Stream would at least reflect the current usage. In the future, you could reconsider the name if needed. ExecutionSpaceInstace sounds like an instance of a class, but then instance of an instance sounds like a meta level. This is something to be discussed in the next pykokkos meeting, but seems unnecessarily complex in my view.
No description provided.