-
Notifications
You must be signed in to change notification settings - Fork 407
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
Metadata Declaration in Core #3729
Conversation
Confirming that this can play nicely with the Tools infrastructure. So from my side, this is what I need |
Retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if you tackled the recording the git commit hash in a separate PR
@masterleinad @dalg24 I think I've addressed the comments. I deferred the Git SHA stuff for a future PR, and almost everything else I just implemented as suggested. The only exception is that I'm not adding the machinery to print MSVC, as we didn't used to print that in our print_configuration machinery |
void declare_configuration_metadata(const std::string& category, | ||
const std::string& key, | ||
const std::string& value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the declaration in Kokkos_Core.hpp
? Where else than Kokkos_Core.cpp
would we use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be used in backends, the CUDA backend will likely declare configuration metadata as well
#else | ||
declare_configuration_metadata("options", "KOKKOS_ENABLE_MPI", "no"); | ||
#endif | ||
declare_configuration_metadata("architecture", "Default Device", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this part of "architecture"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of architecture because that's where it printed in print_configuration before. We can change that, but the underlying goal in this PR was to not shake up the printing as it happened before
@dalg24, remaining are questions about making the default device part of the architecture (which it was before, but which I'm not opposed to changing), and whether we want to expose the |
Retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the comment and remove the unused code.
Also I am curious whether you considered generating the tedious bit
#ifdef KOKKOS_ENABLE_FOO
declare_configuration_metadata("options", "KOKKOS_ENABLE_FOO", "yes");
#else
declare_configuration_metadata("options", "KOKKOS_ENABLE_FOO", "no");
#endif
with CMake. (NOT asking you to change)
@dalg24, I copied that tedious bit from our previous implementation. That implementation doesn't look good to me, but every time we change something to "something that's exactly the same" I find out that we broke something somehow |
@dalg24, believe I have addressed the comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Need to clean up history)
7230f06
to
6885817
Compare
@dalg24: done |
Currently looking for a review of this mechanism for declaring and printing metadata making sure that a code change like this wouldn't perturb anything, it needs more work. A sample output after the change, if you call print_configuration