-
Notifications
You must be signed in to change notification settings - Fork 8
Added Kokkos metadata callback #6
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
Added Kokkos metadata callback #6
Conversation
This is the last blocker for Spot in the RAJA Perfsuite |
What's the motivation here? It's not really adding functionality. Is kokkosp_declare_metadata() a known name that needs to be exported? Instead, could kokkos have kokkosp_declare_metadata, which calls adiak when compiled with adiak enabled? Also, if this stays in, it shouldn't use the C++ interface internally. Adiak's C++ only exists in its header files. The internals should be all C. Use adiak_value() instead of adiak::value(). Also, you're dropping all of value's type information associated and making it a string. For SPOT that would mean no range selections, not dates, etc. That'd be a big drop in functionality. |
Hey Matt, kokkosp_declare_metadata is a known name we look up in the Kokkos Tools interface via dlsym. When we load TAU as a tool, it calls TAU_METADATA, I'm hoping when we load Adiak-enabled Caliper that it will forward to an Adiak call. Our implementation looks a lot like void Kokkos::Profiling::initialize() {
metadata_callback = dlsym("kokkosp_declare_metadata");
}
void Kokkos::Profiling::declareMetadata(string key, string value) {
if(metadata_callback!=nullptr) {
(*metadata_callback)(key.c_str(), value.c_str());
}
} Users just call You're mostly right about typeinfo, with one caveat: things like date are declared in a wrapper library so all the required Spot things exist. But you're right about everything that isn't adiak::date, adiak::commandline, all the code I ripped from Boehme's Lulesh implementation. Re: adiak_value instead of adiak::value, sure thing. If I was to translate adiak::value to adiak_value, would this be something you'd merge? If not, I likely would have to write a wrapper library that wraps Caliper and Adiak, and has a kokkosp_declare_metadata to call into Adiak. That's feasible, but introduces complexity, especially given that you really need an installed Adiak to build Caliper with Adiak. |
I'd like to see if there's better options for dealing with the type info before merging. SPOT really needs the type of its metadata to be able to efficiently display data. Things like FOMs are important (along with dates and versions), and they just don't fit into strings. The SPOT interface also has a lot of performance issues with getting too many unique raw string values. This is going to trigger those issues. Would kokkos be willing to have some variant of: Kokkos::Profiling::declareMetadataType(string key, enum KokkosMetadataTypes atype) where KokkosMetadataTypes could include things like "version", "date", "double", "long", etc? Then if a type has been declared, adiak's declareMetadata could "cast" the string into something proper before passing it on to adiak. Or do you have any other ideas on preserving the type information and the existing interface's backward compatability? |
Hey Matt, Unfortunately, we're unlikely to change the interface, it just merged. There were discussions about various encodings of type info (including using Adiak's format strings, though I didn't have any takers on that), but the simplest, key-val thing was what we got. What we could do without interface changes would be to declare a metadata type map as metadata, for users who want casts Kokkos::Tools::declareMetadata("metadata_typemap::figure_of_merit:long::other_metadata_name:string") Anything not in the map would be a string, anything in the map would get cast. It might be the least bad design without changing the interface |
Sorry, I'd have to say no to that. It's inventing a new language/encoding and hiding it in string parameters intended for other uses. Too much of a bad hack. If we built that and people used it, we'd be paying for that for a long time to come. Better to do things the right way now, even if that's more work now. (And this would probably mess with TAU, however they use the metadata.) Best option would be to bite the bullet and add an API call to kokkos. Adding new API calls that don't break existing behavior is rarely too difficult. It could also just be an optional parameter added to Kokkos::Profiling::declareMetadata. A next best option would be to have users just go directly to Adiak for passing name/value metadata. It looks like Adiak is significantly more powerful at this then Kokkos. And adiak could be setup to pass name/values onwards to kokkos or TAU. A next option might be to skip all of this at the Kokkos/Adiak level and inject types directly into SPOT. You could have YAML file, perhaps alongside your .cali data, that maps names -> types. It'd be a pain to maintain, but not horrible (and perhaps useful for other cases where users want to refine/modify type info). |
I'll try to manage this one outside of Adiak. It's not a matter of the interface in Kokkos, it's about needing to I'm not going to get the callback interface extended for this for quite some time. Users aren't going to put Adiak directly in the code long-term here without first demonstrating the value. People really like the tools interface for insulating them from the tool implementation and overhead concerns. I'll write a wrapper library that implements kokkosp_declare_metadata and calls adiak::value. Probably have it read a (something) to determine type maps, if that becomes necessary. |
Intent: we want the metadata Kokkos declares to be captured by Spot.
So this just adds such a callback to Adiak, declaring our metadata as raw key-value.