Skip to content

Conversation

DavidPoliakoff
Copy link

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.

@DavidPoliakoff DavidPoliakoff changed the title Added kokkos metadata callback Added Kokkos metadata callback Jan 12, 2021
@DavidPoliakoff
Copy link
Author

This is the last blocker for Spot in the RAJA Perfsuite

@mplegendre
Copy link
Member

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.

@DavidPoliakoff
Copy link
Author

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 Kokkos::Profiling::declareMetadata in their code. The Kokkos team is pathologically opposed to making Kokkos even optionally depend on anything, I think that's a case I'd have an impossible time making.

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.

@mplegendre
Copy link
Member

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?

@DavidPoliakoff
Copy link
Author

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

@mplegendre
Copy link
Member

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).

@DavidPoliakoff
Copy link
Author

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 dlsym multiple things and track which got loaded. We have a plan in that direction long-term, maybe we can revisit then.

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.

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.

2 participants