ADIOS2
ADIOS2 copied to clipboard
ornladios
Allow Put/Get functions to accept Kokkos Views
Allow ADIOS2 Put/Get functions to accept Kokkos::View objects.
Kokkos::View<float*, Kokkos::CudaSpace> gpuSimData("gpuBuffer", N);
Kokkos::View<float*, Kokkos::HostSpace> hostSimData("simBuffer", N);
bpWriter.Put(data, hostSimData);
bpWriter.Put(data, gpuSimData);
Optionally data.SetMemorySpace(adios2::MemorySpace::CUDA); can be used to set the ADIOS2 memory space to CUDA
All changes are in the CXX bindings.
Kokkos creates Views for different memory spaces at compile time so I had to instantiate Put/Get functions for HostSpace and CudaSpace. It will be similar when we run with Sycl or HIP.
If anyone has a cleaner solution for this let me know (maybe @vicentebolea). Otherwise I will merge so I can start testing the next step
@anagainaru great work with this
If anyone has a cleaner solution for this let me know (maybe @vicentebolea)
I would avoid populating the ADIOS2 namespace with Kokkos symbols. I would suggest to create a new ADIOS2 class that is called ADIOS2::View that encapsulate the Kokkos::View. This class could possibly use the PIMPL pattern to relegate to its cpp the Kokkos include. You do not want includes to other libraries in headers/tcc.
On another topic I think that this change warrants a new unit test to check this.
For future myself, I would propose to equip our next CUDA build with Kokkos as well
Thanks for the comments @vicentebolea !
I would avoid populating the ADIOS2 namespace with Kokkos symbols. I would suggest to create a new ADIOS2 class that is called ADIOS2::View that encapsulate the Kokkos::View. This class could possibly use the PIMPL pattern to relegate to its cpp the Kokkos include. You do not want includes to other libraries in headers/tcc.
For internal functions I agree it will be better to create some ADIOS class, but for the interface I really think it would be more confusing to users. If their application is already using Kokkos it's easier to just feed the Kokkos View to the Put/Get functions. The only reason Kokkos is included in the header of the ADIOS source code is because I needed to define all the functions to instantiate.
What I propose is to leave the CXX bindings with Kokkos and keep the main source code of ADIOS clean, by moving the instantiation list in the CXX bindings.
On another topic I think that this change warrants a new unit test to check this. For future myself, I would propose to equip our next CUDA build with Kokkos as well
Completely agree with this, I can help create some tests with different backends. We can discuss this more.
So, I'm trying to think through the situation where, for example, an ADIOS module on some machine was built with Kokkos (and maybe whatever other package we might change our API to support). It looks like applications that use ADIOS but don't use Kokkos still just have to #include ADIOS.h, is that right? And presumably Chuck's magic takes care of linking all the appropriate libraries behind the scenes, as long as applications build with CMake. It'd be nice if having Kokkos in the build, bindings or otherwise, doesn't overly complicate the lives of users who don't use it...
So, I'm trying to think through the situation where, for example, an ADIOS module on some machine was built with Kokkos (and maybe whatever other package we might change our API to support). It looks like applications that use ADIOS but don't use Kokkos still just have to #include ADIOS.h, is that right? And presumably Chuck's magic takes care of linking all the appropriate libraries behind the scenes, as long as applications build with CMake. It'd be nice if having Kokkos in the build, bindings or otherwise, doesn't overly complicate the lives of users who don't use it...
You only have to include ADIOS.h if you don't use Kokkos in your app regardless if you build ADIOS with Kokkos or not. The only difference is that if you build it with Kokkos, there will pe Put/Get interfaces in CXX that allow you to pass a Kokkos view
In a way what @anagainaru is describing is what we do with MPI as another public dependency (something like ifdef #ADIOS2_USE_KOKKOS) although less optional than Kokkos. In the past we asked C++ users (e.g. @ax3l) to provide feedback for public facing APIs. Seeing code side-by-side might be helpful as well.
What I propose is to leave the CXX bindings with Kokkos and keep the main source code of ADIOS clean, by moving the instantiation list in the CXX bindings.
I understand this, it is in fact a great idea so you use ADIOS main source code as a black box where you just store the View pointer.
My concern is that since you only need the Kokkos::View it is overkill to include all of its namespace in on of ADIOS main headers. Even if this is only enabled when Kokkos is enabled one reason might be that the user wants to pass an Kokkos View pointer but he himself does not want to include Kokkos. The solution of having a class that represent this will also help us not to change the the adios engine template (API) if we decide to support another framework of its kind, maybe SYCL/oneapi?
class AdiosView {
virtual void* GetNativeView()
}
// in adiosviewkokkosheader
include AdiosView
include kokkos
template <class T, class MemSpace>
class AdiosViewKokkos: public AdiosView
{
AdiosView(Kokkos::View<T, Memspace>);
virtual void* GetNativeView() { return reintepreter_cast<void*>(ptr); }
Kokkos::View<T, Memspace>* ptr;
}
Instantiate all templates
// In user code
// this 3 lines can be in this file or in another one.
include adiosviewkokkosheader
Kokkos::View<10,10> v(10);
AdiosView = new AdiosViewKokkos(v);
engine.put(myvar, v);
We can use smart pointers and factory methods to make avoid the new usage and calling constructors directly.
In a way what @anagainaru is describing is what we do with MPI as another public dependency
@williamfgc MPI is a different order of library type, it is expected to be found with a stable API just about in any machine, Kokkos is a still young project and very optional, plus our support to it is very limited.
@vicentebolea I understand your point and I agree that Kokkos is a young, optional project. However, I am sure that the only time ADIOS will be build with Kokkos is when the application is using Kokkos in which case it's so much more intuitive to just pass the Kokkos View.
Agree also that for now it is an overkill to include the entire namespace, but we plan to use Kokkos internally to compute the metadata and copying the data in our buffer (I have a draft code already for it, I'll add it once I have time to do more testing).
For now, I would leave this PR the way it is and once I finish the other PR we can discuss what our strategy should be.
I have a similar design philosophy as Vicente is proposing.
Here is a longer writeup why I would also prefer a separate header for extended functionality: https://github.com/betterscientificsoftware/bssw.io/pull/1362
I would also tend to agree with @vicentebolea . Yes, if individual users are building ADIOS specifically customized to their use cases, then an ADIOS build with Kokkos is likely to only used by Kokkos applications. But we'd like to be a standard system library/module that users don't have to build a custom version of themselves.
@ax3l completely agree, there is a Kokkos header that only has forward declaration so I am using that in the h files.
Is this acceptable?
A few things:
- Kokkos has been very stable for a while, and it's a well-established ECP funded project.
- MPI has int/void API differences between MPICH and OpenMPI (there is a workaround in ADIOS constructors). Not to mention than vendor-specific MPI have subtle differences in the toolchain (e.g. libmpi_cray.so , but that's a CMake problem)
Sounds like a larger discussion. In general, I wouldn't add to the API a new class for minimal functionality when overloading might do the job. C++17 is much better at this with things like template auto-deduction or std::any for safety.
I might be missing something, but for simplicity why not just overloading Put/Get at compile-time and make Kokkos an optional dependency (just like MPI is)?
#ifdef ADIOS2_HAVE_KOKKOS
Put<T>( Variable<T>, Kokkos::View<>... );
#endif
we do this for std::vector
Get<T>(Variable<T>, std::vector<T>...);
How is ADIOSView saving maintenance costs, seems like it might be adding more.
@williamfgc I agree, this is what I am doing now, overloading Put/Get with Kokkos an optional dependency. And at this point we are only loading the function declarations when building ADIOS with Kokkos because I am not including the Kokkos_Core in any of our h files.
Let's move this discussion at one of the ADIOS meetings.
Let me also put in my two cents:
An alternative is to think of this in generic programming terms. That is, instead of having a specific overload for Kokkos views, and std::vector, and maybe xtensor and Eigen multi-d arrays, an alternative is the "if it quacks like a duck, treat it like duck" approach.
That is, one can have a generic templated interface and std::enable_if to say, e.g., if the object has a .data() method and a .size() methods, that's all we need and we'll take it -- and if it's not it'll not be considered in overload resolution.
If that turns out to not be enough at some point, one could have a traits-based approach that would allow to transparently add support for datastructures that aren't known to adios2 and that don't have .data() and .size() but can be adapted.
I think the advantages of this approach are:
- doesn't required any build system interaction, inclusion of additional headers, etc.
- it's more generic than just supporting Kokkos specifically
The disadvantage that I can see is that it might falsely make adios2 accept some data structure which does have .data() and .size(), but doesn't follow the required semantics.
I agree with @germasch . The first step is to agree that overloading Put/Get will be the user's life much easier (no extra lass to learn about, just pass an object with an accessible contiguous memory region). There still need to be a compile-time conditional as these are optional dependencies (Kokkos, eigen, etc.), std::vector is standard so gets better treatment.
I agree that overloading put/get is the easier for the user, but as any sounded API design we need to decouple our codebase from our dependencies when possible. This is not a dealbreaker but the best dependency in your software is the one that you do not have, the second best is the one at compile time, next is the one that its contained in its own independent header and the last one is at core code base header (we are a lib afterall).
Since we only need a few API components of Kokkos::View I really like the @germasch idea, we really depends on type traits of the Kokkos::View thus we can just program against those trait without really including it. Furthermore, we can extend this template methods for other frameworks such as Kokkos by adding adapters that makes them fit in our type traits. Lastly C++17/20 brings better ways to express this (no SFINAE enable_if needed).
From a build point of view this is best since we do not need Kokkos at build time.
From a build point of view this is best since we do not need Kokkos at build time.
That's the part I'm having trouble understanding, how does ADIOS know how to interpret Kokkos::View internals? At some point, a compile-time handshake is needed to interpret and access the underlying memory contiguous pointer. It would be nice to see a working proof-of-concept.
but as any sounded API design we need to decouple our codebase from our dependencies when possible
My worry is to add more boilerplate for something so lightweight that Put/Get can handle. We do this for adios2::Operator , but that's 'cause it's far from being lightweight and dependencies (e.g. zfp, blosc compression, etc.) are private in the CMake sense. As for public dependencies like MPI, and potentially Kokkos, I don't see how we can escape some sort of #ifdef handshake in the public facing region. Just my two cents as we went through this in the past, adios2 value has been on performance, functionality and interoperability with (hopefully) a relatively small API footprint for applications. As a library consumer it's good that the public interface is clear in what can be used, what's optional and what's not, etc.
@williamfgc in my last comment I was advocating for the @germasch template solution.
So I looked at this PR in some more detail now, and really all it needs from a Kokkos::View is the .data() method. I still think it'd be good for a .size() method if it isn't used -- it could be used for sanity checking at some point in the future, but more importantly, it'll help to avoid making the match to broad and match not just container-like classes.
It does look that right now in this PR the memory space, while it's known as a template type, isn't actually used to set the adios2 memory space. As such, it'd be fairly easily possible to just do SFINAE (std::enable_if -- I think ADIOS2 is still C++14, right?) on .data(), .size() and making sure that the return type of .data() matches the T from Variable<T>. (BTW, I think there's a possible issue with handling const T in the current approach, too).
The more general approach would be not to access methods from the passed type directly at all, but use adios2_view_traits or something. There could still be a default provided that'll work for kokkos or anything that has .data() or .size(), but it'd allow for adapting other containers that may not follow this pretty standard pattern of .data() and .size(). More importantly, though, the traits approach would allow for translating between kokkos memory space and adios2 memory space, and other future extensions (there's a lot I can think of, e.g., taking the dimensionality and local extents directly from the array, rather than having to set them in separate SetShape()/SetSelection() beforehand.)
When it comes to honoring memory space, I do not see a way that avoids kokkos-specific knowledge, ie., the Kokkos::XYZSpacemacros. But this could still be kept separate pretty easily, ie.,#include <adios2/kokkos.h>` would set those traits, and obviously depend on the kokkos symbols being available, while everything else remains agnostic to whether kokkos is available or not.
For people who aren't familiar with the kind of C++ pattern I'm talking about, https://en.cppreference.com/w/cpp/memory/allocator_traits or https://en.cppreference.com/w/cpp/iterator/iterator_traits are examples how traits are used to make it possible to use custom classes with the standard library, without requiring the standard library to know about them -- all the user has to do is to specialize the corresponding traits to tell the standard library how to interface with them.
- all the user has to do is to specialize the corresponding traits to tell the standard library how to interface with them.
The translation unit needs to know when linking where Kokkos::View::data() is defined (declaration can be forwarded) since templates become regular functions at compile-time and need to resolve all the types. It would become simpler for users to pass the raw pointer from data(). Users shouldn't specialize adios2 APIs. At the end performance, functionality and simple APIs are the main drivers.
This is why I am advocating for a working proof of concept. Adios2 is C++11 (not 14 or 17) and the boilerplate can get pretty messy, pretty quickly without added safety or simple APIs. I think we have done successful proof of concept PRs in the past like unifying MPI and non-MPI builds.
@williamfgc in my last comment I was advocating for the @germasch template solution
@vicentebolea that's fair. Since @anagainaru went through the effort to make this PR available, I think is also fair to ask for a PR showing the proposed alternatives. A working proof of concept would help the discussion tremendously.
At the end performance, functionality and simple APIs are the main drivers.
@williamfgc we all agree on this, consider this scenario.
What if in next kokkos release they move to c++17 (actually they are planning to). What do we do with ADIOS2 if we include them in our core headers? Now when we enabled kokkos we need to build using c++17, what if they include a c++17 feature that is not supported with c++14, since our code is now highly coupled with Kokkos we would have to migrate our full codebase to be C++17 compatible or add a myriad of ifdef to maintain compatibilty with c++17 and without for previous distros/compiler versions. Maybe this highlight the case of the "worse is better".
I would say that this approach does not scale with other future libraries that we intent to support as Kokkos.
Users shouldn't specialize adios2 APIs
Why not?
Why not?
Users come to adios 'cause they want to do their science at scale. Adding more overhead is a turn off for an already highly specialized library and niche for HPC workflows. You mention that support is limited, that's also the case for users.
The "what if" and "what about" scenarios you mentioned can be understood better with a proof of concept and see the pros and cons and be clear on what we are advocating for. In any scenario you need a handshake with Kokkos or any public facing library.
- all the user has to do is to specialize the corresponding traits to tell the standard library how to interface with them.
The translation unit needs to know when linking where Kokkos::View::data() is defined (declaration can be forwarded) since templates become regular functions at compile-time and need to resolve all the types. It would become simpler for users to pass the raw pointer from data(). Users shouldn't specialize adios2 APIs. At the end performance, functionality and simple APIs are the main drivers.
Sure, at compile time Kokkos::View and its methods need to be known. And they will be, that is, at compile time for the application that uses a Kokkos::View and passes it off to ADIOS2. That application obviously needs to be built with Kokkos headers etc, so everything will be available t the time. But at ADIOS2 build time, it's not needed, since the corresponding version of Put won't be instantiated.
I put up a draft PR #3271 to show a proof of principle just for Put, though Get can be handled analogously. And yeah, those things are not pretty, but it's more pretty much the same stuff that the C++ standard library does internally (and hides it away from the casual user).
The draft PR I put up actually just does the bare miminum, ie., memory space information is not communicated from Kokkos to ADIOS2 (nor is that done with Ana's PR here). It could be added, but at that point, it will require a specific header, say <adios2/kokkos_interop.h> that should be installed by ADIOS2, but only included by a Kokkos application relying on this kind of interoperability. That still means virtually no impact on ADIOS2, and extensibility to other, e.g., multi-d array containers, but it does mean one additional header that needs to be included on the application side.
Here's a (standalone) test for the draft PR: https://github.com/germasch/kokkos-adios2 .
I like @germasch solution. I'll give it a try this weekend (or next week) and we can discuss on a working proof of concept.
Agree with @anagainaru . Thanks @germasch , it's very cool. I like the simplicity of the API. Another aspect is to test how the CMake files you put. Agree, the pattern is well known for being a huge boilerplate, like many things C++. I'll try to take a look as well. Happy 4th.
@germasch Thanks for going the extra mile and submit a draft PR with the discussed design. I really like it, moreover years from now we can further simplify it with new c++ features. I left some comments in the draft.