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

Deprecate --[kokkos-]threads command line argument in favor of --[kokkos-]num-threads #5111

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 10, 2022

Rational: consistency

environment variable command line argument
N/A --[kokkos-]help
KOKKOS_DISABLE_WARNINGS --kokkos-disable-warnings
KOKKOS_TUNE_INTERNALS --kokkos-tune-internals
KOKKOS_NUM_THREADS --[kokkos-]threads
KOKKOS_NUMA --[kokkos-]numa
KOKKOS_DEVICE_ID --[kokkos-]device-id
KOKKOS_NUM_DEVICES --[kokkos-]num-devices (optional 2nd INT to skip)
KOKKOS_SKIP_DEVICE 2nd INT value with --[kokkos-]num-devices
KOKKOS_RAND_DEVICES

It was pointed out on the developer channel on Slack that --kokkos-disable-warnings and --kokkos-tune-internals do not have without the kokkos- prefix. I would prefer not settling this here. I tend to think we should only have supported --help.

@dalg24
Copy link
Member Author

dalg24 commented Jun 10, 2022

Here is a link to the documentation https://github.com/kokkos/kokkos/wiki/Initialization#51-initialization-by-command-line-arguments

Note that the changes proposed here are consistent with the naming of the data member Kokkos::InitArguments::num_threads
https://github.com/kokkos/kokkos/wiki/Initialization#52-initialization-by-struct

@dalg24
Copy link
Member Author

dalg24 commented Jun 10, 2022

Wiki is actually outdated (missing tune_internals and other kokkos-tools options)

struct InitArguments {
int num_threads;
int num_numa;
int device_id;
int ndevices;
int skip_device;
bool disable_warnings;
bool tune_internals;
bool tool_help = false;
std::string tool_lib = {};
std::string tool_args = {};

For reference, below is a table that includes the data members from the Kokkos::InitArguments struct

environment variable command line argument Kokkos::InitArguments data member
N/A --[kokkos-]help
KOKKOS_DISABLE_WARNINGS --kokkos-disable-warnings disable_warnings
KOKKOS_TUNE_INTERNALS --kokkos-tune-internals tune_internals
KOKKOS_NUM_THREADS --[kokkos-]threads num_threads
KOKKOS_NUMA --[kokkos-]numa num_numa
KOKKOS_DEVICE_ID --[kokkos-]device-id device_id
KOKKOS_NUM_DEVICES --[kokkos-]num-devices (optional 2nd INT to skip) ndevices
KOKKOS_SKIP_DEVICE 2nd INT value with --[kokkos-]num-devices skip_device
KOKKOS_RAND_DEVICES

Copy link
Contributor

@JBludau JBludau left a comment

Choose a reason for hiding this comment

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

Strongly in favor of this change. Just makes perfect sense to have a consistent naming prefix

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 12, 2022

@dalg24 now there is this:

  • KOKKOS_NUMA, --[kokkos-]numa, num_numa
    why not changing it to:
  • KOKKOS_NUM_NUMA, --[kokkos-]numa, num_numa
    so that it is consistent with threads?
    If we want to keep KOKKOS_NUMA then I personally would do numa for the InitAguments

@dalg24
Copy link
Member Author

dalg24 commented Jun 12, 2022

I noticed the inconsistency (so did I for Kokkos::InitArguments::ndevices) but these may be more controversial and I did not want to tie the fate of the PR to them.
Note that making changes to the data members in the struct might be tricky with respect to backward compatibility.

@crtrott crtrott merged commit fc3d4b2 into kokkos:develop Jun 15, 2022
@dalg24 dalg24 deleted the rename_cmdlinearg_kokkos_num_threads branch June 23, 2022 23:16
@ajpowelsnl ajpowelsnl mentioned this pull request Nov 7, 2022
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.

None yet

5 participants