Skip to content
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

Rename tools settings for consistency #5201

Merged
merged 8 commits into from
Jul 14, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jul 12, 2022

As part of our effort to cleanup or initialization settings, this PR is suggesting to make the following changes to the tools options:

  • InitializationSetting::{set,has,get}_...: renamed tool_lib -> tools_libs, tool_args -> tools_args, and tool_help -> tools_help
  • Command line arguments: renamed --kokkos-tools-{library -> libs}
    --kokkos-tools-args and --kokkos-tools-help unchanged.
  • Environment variables: renamed KOKKOS_{PROFILE_LIBRARY -> TOOLS_LIBS} and added KOKKOS_TOOLS_ARGS
    We still look for KOKKOS_PROFILE_LIBRARY and raise a warning stating it is deprecated. If both KOKKOS_PROFILE_LIBRARY and KOKKOS_TOOLS_LIBS are set, we ensure that they match.
    Haven't added KOKKOS_TOOLS_HELP. Let me know if you think I should do so.

The Kokkos::Tools::InitArguments struct is not updated as part of this PR.

struct InitArguments {
// NOTE DZP: PossiblyUnsetOption was introduced
// before C++17, std::optional is a better choice
// for this long-term
static const std::string unset_string_option;
enum PossiblyUnsetOption { unset, off, on };
PossiblyUnsetOption help = unset;
std::string lib = unset_string_option;
std::string args = unset_string_option;
};

We still need to decide whether we just want to change the data member name or opt for a design similar to what we did in Core. In any case I would prefer to handle it in a follow-up PR.

@dalg24
Copy link
Member Author

dalg24 commented Jul 12, 2022

STATUS QUO (BEFORE this PR) PROPOSED
Kokkos::InitializationSetting::{set,has,get}_tool_lib Kokkos::InitializationSetting::{set,has,get}_tools_libs
Kokkos::InitializationSetting::{set,has,get}_tool_args Kokkos::InitializationSetting::{set,has,get}_tools_args
Kokkos::InitializationSetting::{set,has,get}_tool_help Kokkos::InitializationSetting::{set,has,get}_tools_help
KOKKOS_PROFILE_LIBRARY KOKKOS_TOOLS_LIBS
KOKKOS_TOOLS_ARGS
--kokkos-tools-library --kokkos-tools-libs
--kokkos-tools-args --kokkos-tools-args
--kokkos-tools-help --kokkos-tools-help
Kokkos::Tools::InitArguments::lib Kokkos::Tools::InitArguments::lib
Kokkos::Tools::InitArguments::args Kokkos::Tools::InitArguments::args
Kokkos::Tools::InitArguments::help Kokkos::Tools::InitArguments::help

@dalg24
Copy link
Member Author

dalg24 commented Jul 12, 2022

I would slightly prefer spelling out "libs" -> "libraries". Anyone else on that boat?

@ldh4
Copy link
Contributor

ldh4 commented Jul 12, 2022

I would slightly prefer spelling out "libs" -> "libraries". Anyone else on that boat?

While I don't have any strong opinion on either option, it kinda looks nice to have all tools options be aligned and have the same lengths by leaving "libs" to have the same character count as "args" and "help".

@crtrott
Copy link
Member

crtrott commented Jul 12, 2022

I kinda like libraries a bit more too.

@nliber
Copy link
Contributor

nliber commented Jul 13, 2022

If we are going to spell out "libraries", should we also spell out "arguments"?

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 13, 2022
@dalg24
Copy link
Member Author

dalg24 commented Jul 13, 2022

My recollection from the dev meeting was we keep the name as they are currently implemented in this PR, that is kokkos_tools_{help,libs,args}

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me (as well as both libraries and arguments).

Copy link
Contributor

@ldh4 ldh4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recollection from the dev meeting was we keep the name as they are currently implemented in this PR, that is kokkos_tools_{help,libs,args}

That's what I recall as well.

@dalg24 dalg24 merged commit 021c09f into kokkos:develop Jul 14, 2022
@dalg24 dalg24 deleted the kokkos_tools_settings branch July 14, 2022 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants