kokkos / kokkos

Kokkos C++ Performance Portability Programming Ecosystem: The Programming Model - Parallel Execution and Memory Abstraction
https://kokkos.org
Other
1.86k stars 420 forks source link

Containers: which go in Kokkos or Kokkos Kernels #4255

Open lucbv opened 3 years ago

lucbv commented 3 years ago

@crtrott @srajama1 During the Aug-18 stand-up we discussed the possibility of moving some containers from one repo to another. Below are the two containers previously discussed, if others should be added to the list, feel free to edit this issue.

ajpowelsnl commented 3 years ago

@lucbv - I've added to "Items to Discuss" for this week's Kokkos developer meeting.

lucbv commented 3 years ago

Thanks Amy : )

ajpowelsnl commented 3 years ago

Of course, @lucbv. BTW, is everyone aware of all of the containers under consideration?

lucbv commented 3 years ago

We talked about it last week so I believe they are but we also do not have to vote on it this week if we want to expand the discussion. It's just good to have it out there for folks to debate.

crtrott commented 3 years ago

ExaMiniMD for example uses StaticCrsGraph to demonstrate CRS based neighborlists.

fnrizzi commented 3 years ago

Just my personal thought, and obviously there is a tradeoff to evaluate since it would add one more repo to manage, but what about creating a kokkos-containers repo? What other containers are in discussion (dynamic view, map, vector, etc)? In terms of dependency stack one would have:

 kernels 
---------
containers
---------
  core 

where the containers also contain the CrsMatrix. Seems pretty clean to me. As a user, it would be intuitive. Also, if I wanted to use a CrsMatrix somewhere, this would allow me to just having to handle {containers,core}. Re the pool, I personally would add to core. Just my personal opinion. I realize that handling another repo might be a pain (containers could be made a submodule of kernels for example to simplify some things), but maybe there is an advantage in keeping things highly compartmentalized.

dalg24 commented 3 years ago

Francesco's suggestion https://github.com/kokkos/kokkos/issues/4255#issuecomment-905522733 is tempting, especially if DualView gets lost in the process

stanmoore1 commented 3 years ago

especially if DualView gets lost in the process

Are you re-writing LAMMPS? :)

PhilMiller commented 3 years ago

Extra repositories make sense if there's a burden to consolidation (e.g. large size) or a benefit to separation (e.g. different policies/practices). Otherwise, I'd favor moving things into the base kokkos repository

dalg24 commented 3 years ago

Unanimous consent for moving crsgraph to kernels and the uniform memory pool to kokkos

crtrott commented 3 years ago

AH, because I missed this. Note when I answered this didn't have the vote label yet, and adding it on the day is kinda iffy in case someone misses the meeting as I did. I would have voted at least against it maybe strongly so. Largely because CRSGraph is not just a linear algebra thing, but rather a generic data structure for indirect lists. And sure its not used in Kokkos Core but neither are any of the other containers, because they are not allowed to.

As I said above, ExaMiniMD uses CrsGraph, so if this decision stand someone needs to copy CrsGraph into it, because it makes absolutely no sense to buy a dependency on KokkosKernels just for that guy. I have not checked if other things use it

Someone needs to rewrite ExaMiniMD then. Note CRSGraph is not a purely algebra data structure. Its generally useful in places where one would otherwise have used linked-lists.

stanmoore1 commented 3 years ago

Note CRSGraph is not a purely algebra data structure. Its generally useful in places where one would otherwise have used linked-lists.

I agree, and don't want a KokkosKernels dependency in ExaMiniMD just to get CrsGraph.

stanmoore1 commented 3 years ago

Also SPARTA uses CrsGraph too without a KokkosKernels dependency.

ajpowelsnl commented 3 years ago

Good morning @crtrott , @stanmoore1 , @lucbv , @dalg24 ,

I apologize if I put this item up for voting too late for this week's meeting. If you like, we can add the "To Be Voted" label now, and have another round of discussions next Wednesday. It sounds like important pieces of this topic were not well represented in our meeting on the 25th. Please let me know if you'd like to have another round of discussion.

srajama1 commented 3 years ago

Folks, the CRS in CRSGraph is compressed row sparse matrix, which is a foundation for linear algebra. If that doesn't belong in Kokkos Kernel then I don't know what does.

I am not sure why folks are so averse to adding a dependency on Kokkos Kernels. What am I missing? If it is a brand new product from some external source there is an inherent risk associated with adding a dependency to a new library. It is an SNL developed library, that is part of the Kokkos ecosystem, and follows the same process for everything. I believe ExaminiMD (and/or other mini-apps) could use the entire Kokkos ecosystem, Core for programming model, Kokkos Kernels for linear algebra data structures or other kernels, and tools for profiling, debugging etc.

We are also modularizing Kokkos Kernels next FY, so Examinimd will only compile a small portion of Kokkos Kernels (container like portion).

On the opposite I have a reservations on moving MemoryPool to Core. What is the point of this? Why can't folks who want to use MemoryPool should just depend on Kokkos Kernels?

The Containers repo proposed above is attractive, but lot of work. We are moving to that model any way without having additional repositories (one repo, but allow partial compilation, tests etc.) anyway next year. Folks can just get containers submodule of Kokkos Kernels which could have CrGraph and MemoryPool.

fnrizzi commented 3 years ago

disclaimer: everything I saw below is meant to be constructive and not a critic in any way. Also, I understand there are practical implications like timelines etc.

the CRS in CRSGraph is compressed row sparse matrix, which is a foundation for linear algebra. If that doesn't belong in Kokkos Kernel then I don't know what does.

I see your point. However, if I were (and I am) a user---and being picky---I would consider the CrsMatrix a container, which i personally see as distinct from kernels. Obviously, we are kind of splitting hair, and talking about "where" something belongs to. But given that the names of the main components have to remain (right?), kernels makes me think of linear algebra kernels which use/depend on containers. Personally, I would see containers as separate. If they are inside kernels, that is because it is a practical detail now.

On the opposite I have a reservations on moving MemoryPool to Core. What is the point of this? Why can't folks who want to use MemoryPool should just depend on Kokkos Kernels?

My thought about this, and the reason I expressed my opinion about moving MemPool to core is that I favor putting things where appropriate according to functionality. For MemoryPool, IMMO, it makes sense to be inside core: kernels uses it, but MemoryPool is more a core-like functionality. In some sense, I also think the name of things can be misleading and a catch-all. Core can mean a lot of things. If one wants a finer-grained solution, one could envision splitting core into: views, tasking, memory-management, parallel-dispatch, etc so in that case, I would put MemoryPool inside memory-management. The advantage is that if I wanted to only use MemoryPool, I would only build the memory-mang component (assuming everything is encapsulated as needed to easily enable individual usage).

We are also modularizing Kokkos Kernels next FY, and The Containers repo proposed above is attractive, but lot of work.

(1) modularizing sounds great! I don't understand why it would be too hard to split things into: core/containers/kernels/tutorials/tools/pykokkos. I think using some sort of "submoduling" (does not have to be git submodule, can be some of the other similar git things) would make this totally feasible. In the end, I don't see an extra container repo as causing so much overhead to maintain since it would be pretty much mirroring the build system of core (in my view). In some sense, doing such separation would at least cut to zero all debates of where something belongs to :) since there would be no interpretation of it any longer.

(2) Taking this to the other extreme, but the Kokkos ecosystem does not necessarily have to be fully split in separate repo. Phil alluded to this above. One other way to simplify all this, would be to put everything inside (dependency is top-to-bottom):

kokkos:
  kernels
  pykokkos (putting this here since I assume it does not depend on kernels) 
  containers
  core
  tools (I think it goes here right?)

or even finer-grained:

kokkos:
  kernels-batched
  kernels-dense
  kernels-sparse
  pykokkos (putting this here since I assume it does not depend on kernels) 
  containers
  core
  tools (I think it goes here right?)

BUT such that users can pick, build and use individual pieces similarly to how/what they do now. No repo-level split, no room for debating, everything in one place, users only deal with one thing. Personally, I think this would risk of becoming another Trilinos and I like (1) better but wanted to bring this up too as the other extreme.

crtrott commented 3 years ago

How I see it kokkos/kokkos was meant for general purpose stuff useful to a wide array of different application domains, while KokkosKernels was math focused. That is why we put StaticCrsGraph into Kokkos/kokkos back in the day, because we explicitly discussed it with the respective teams (and back in the day Mark and I were the ones responsible for "KokkosKernels"/"Tpetra" which owned the on-node math stuff) and we considered StaticCrsGraph more general purpose than the name indicated. Maybe we should have renamed it to LOSSL (List-Of-Static-Sized-Lists) to make that clearer, but it really is just the equivalent to a pointers of pointers array, in a Count-Allocate-Fill-Use world enforced by massive parallelism, and no viable fast parallel allocation mechanism on the device. Considering that this data structure is really trivially simple (I mean after all it is just a struct with and offset array and a values array) I doubt that anyone would think: Ah let me get a new dependency instead of writing this on my own. In fact: we have two of those in kokkos/kokkos, since we needed the same data structure internally but didn't want to make the core subpackage depend on the containers subpackage.

stanmoore1 commented 3 years ago

Folks, the CRS in CRSGraph is compressed row sparse matrix, which is a foundation for linear algebra. If that doesn't belong in Kokkos Kernel then I don't know what does.

CRS is generally useful and not just limited to linear algebra, hence the argument to stay in Kokkos/containers. ExaMiniMD and SPARTA have zero linear algebra kernels but still use CRS.

fnrizzi commented 3 years ago

we needed the same data structure internally but didn't want to make the core subpackage depend on the containers subpackage.

Could that imply that maybe core would benefit from being broken up a bit?

crtrott commented 3 years ago

No it doesn't indicate core needs breaking up. Its more the opposite, It indicates sometimes its hard to separate containers from the core programming model since one needs data structures to implement the parallelism stuff too. On the other hand containers need parallelism to implement things like deep copy, initialization and what not.

mhoemmen commented 3 years ago

That is why we put StaticCrsGraph into Kokkos/kokkos back in the day, because we explicitly discussed it with the respective teams (and back in the day Mark and I were the ones responsible for "KokkosKernels"/"Tpetra" which owned the on-node math stuff) and we considered StaticCrsGraph more general purpose than the name indicated.

Right, it's a struct with a few typedefs in it that Christian wrote 2013-4-ish. It's nothing anyone would write a paper or a proposal about.

These days, I'm in Stan's position of wanting to minimize software dependencies and reduce build time.

PhilMiller commented 3 years ago

As a potential split-the-baby proposal, how about exposing the ListOfLists name and implementation in Core, and then composing StaticCrsGraph with whatever linear-algebra-specific functionality it may offer in its interface on top of that?

More seriously, it seems like if Kernels is developed by an overlapping group of people, with the same policies and infrastructure, it would make sense to consolidate it into the same repository as Core and Containers, with whatever additional configure-/build-time modularization effort to leave out the parts any given dependent application isn't using.