Deprecate `InitArguments` struct and replace it with `InitializationSettings` #5135

dalg24 · 2022-06-23T13:55:21Z

I drafted a replacement struct for InitArguments. ~~At this time it is still missing the implicit conversion from InitArguments.~~ I haven't put any thought into the name yet. I am looking for early feedback on the design, that is public member function to set the values that can be chained and "private" macros to query if a variable is set and and get its value.

core/unit_test/default/TestDefaultDeviceDevelop.cpp

masterleinad

I think this is conceptionally (interface-wise) good. I'm not quite sure if we really need the getter-functions but they seem to be good for testing at least.
As discussed elsewhere, we also need to see what issues we would need to revolve for interfaces taking the InitArguments right now.

cz4rs · 2022-06-23T15:07:32Z

I would be in favor of adding the getters as well. This obviously gets complicated for the cases where the variable is not set (what to return? the optional itself?).

nliber · 2022-06-23T18:51:47Z

I like this interface as well.

While I don't see a need for getters, maybe a way to print out the configuration?

PhilMiller · 2022-06-23T19:03:35Z

I would favor getters and making the members private, so that the compiler will enforce that all modifications go through the setters, rather than doing weird stuff with clunky name modifications in macros.

…ttings

masterleinad

Looks like a guard for the deprecated struct is missing:

/var/jenkins/workspace/Kokkos/core/unit_test/TestInitializationSettings.cpp:56:11: error: no type named 'InitArguments' in namespace 'Kokkos'
  Kokkos::InitArguments arguments;
  ~~~~~~~~^
1 error generated.
core/unit_test/CMakeFiles/KokkosCore_UnitTest_Default.dir/build.make:94: recipe for target 'core/unit_test/CMakeFiles/KokkosCore_UnitTest_Default.dir/TestInitializationSettings.cpp.o' failed

Rombur · 2022-07-06T14:04:03Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Instance.cpp

-  // Prevent "unused variable" warning for 'args' input struct.  If
+void OpenMPTargetSpaceInitializer::initialize(
+    const InitializationSettings& settings) {
+  // Prevent "unused variable" warning for 'settings' input struct.  If
  // Serial::initialize() ever needs to take arguments from the input


Wrong comment. It should be OpenMPTarget::initialize()

You're right I hadn't seen it. Considering that I will remove that code in #5144, I would rather not fix here though.

Rombur · 2022-07-06T14:11:27Z

core/src/impl/Kokkos_Core.cpp

@@ -100,45 +99,105 @@ void declare_configuration_metadata(const std::string& category,
  metadata_map[category][key] = value;
 }

-ExecSpaceManager& ExecSpaceManager::get_instance() {
+void combine(Kokkos::InitializationSettings& out,
+             Kokkos::InitializationSettings const& in) {


Why do you need this function?

It replaces void combine(InitArguments& out, InitArguments const& in) and we use it in initialize(InitializationSettings const&)

masterleinad

Overall, this looks good to me. Just a few questions/suggestions/nitpicks.

core/src/Kokkos_Core_fwd.hpp

masterleinad · 2022-07-06T14:43:21Z

core/src/impl/Kokkos_Core.cpp

+#define KOKKOS_IMPL_COMBINE_SETTING(NAME) \
+  if (in.has_##NAME()) {                  \
+    out.set_##NAME(in.get_##NAME());      \
+  }                                       \
+  static_assert(true, "no-op to require trailing semicolon")
+  KOKKOS_IMPL_COMBINE_SETTING(num_threads);
+  KOKKOS_IMPL_COMBINE_SETTING(device_id);
+  KOKKOS_IMPL_COMBINE_SETTING(num_devices);
+  KOKKOS_IMPL_COMBINE_SETTING(skip_device);
+  KOKKOS_IMPL_COMBINE_SETTING(disable_warnings);
+  KOKKOS_IMPL_COMBINE_SETTING(tune_internals);
+  KOKKOS_IMPL_COMBINE_SETTING(tool_help);
+  KOKKOS_IMPL_COMBINE_SETTING(tool_lib);
+  KOKKOS_IMPL_COMBINE_SETTING(tool_args);
+#undef KOKKOS_IMPL_COMBINE_SETTING


Nitpick: I know you didn't introduce this in this pull request, but I would prefer avoiding macros here and rather do

Suggested change

#define KOKKOS_IMPL_COMBINE_SETTING(NAME) \

if (in.has_##NAME()) { \

out.set_##NAME(in.get_##NAME()); \

} \

static_assert(true, "no-op to require trailing semicolon")

KOKKOS_IMPL_COMBINE_SETTING(num_threads);

KOKKOS_IMPL_COMBINE_SETTING(device_id);

KOKKOS_IMPL_COMBINE_SETTING(num_devices);

KOKKOS_IMPL_COMBINE_SETTING(skip_device);

KOKKOS_IMPL_COMBINE_SETTING(disable_warnings);

KOKKOS_IMPL_COMBINE_SETTING(tune_internals);

KOKKOS_IMPL_COMBINE_SETTING(tool_help);

KOKKOS_IMPL_COMBINE_SETTING(tool_lib);

KOKKOS_IMPL_COMBINE_SETTING(tool_args);

#undef KOKKOS_IMPL_COMBINE_SETTING

// clang-format off

if (in.has_num_threads) out.set_num_threads(in.get_num_threads);

if (in.has_device_id) out.set_device_id(in.get_device_id);

[...]

// clang-format off

Not changing as I do not agree with the suggestion

masterleinad · 2022-07-06T15:16:06Z

core/src/impl/Kokkos_Core.cpp

+Kokkos::Impl::InitializationSettingsHelper<std::string>::storage_type const
+    Kokkos::Impl::InitializationSettingsHelper<std::string>::unspecified =
+        "some string we don't expect user would ever provide";


Shouldn't this be in its own *.cc file since you already split the implementation of the struct into its own header file?

Considering that we will drop it when C++17 is available, I'd rather not create a source file.

masterleinad · 2022-07-06T15:28:09Z

core/src/impl/Kokkos_InitializationSettings.hpp

+#define KOKKOS_IMPL_INIT_ARGS_DATA_MEMBER(NAME) \
+  impl_do_not_use_i_really_mean_it_##NAME##_
+
+#define KOKKOS_IMPL_INIT_ARGS_DATA_MEMBER_TYPE(NAME) impl_##NAME##_type


I find the code here with all the macros very hard to read and would rather see more boilerplate.
For these two macros, in particular, I don't see much of a reason to not replace them with their definition.

This was introduced before we added the getters and was really handy then. I would rather wait until C++17 is available to refactor. Then I will get rid of the member type and "inline" all data member references.

core/src/impl/Kokkos_InitializationSettings.hpp

…nitiliazationSettings setters in converting constructor

masterleinad

Good enough for me.

…nit_args_struct

nliber · 2022-07-07T18:29:55Z

core/src/Serial/Kokkos_Serial.cpp

  // Serial::initialize() ever needs to take arguments from the input
  // struct, you may remove this line of code.
-  (void)args;
+  (void)settings;


Minor: I know it wasn't done like this originally, but why not just comment out the variable name in the function declaration?

dalg24 · 2022-07-07T20:57:40Z

OpenMPTarget timeout is clearly unrelated

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

cz4rs reviewed Jun 23, 2022

View reviewed changes

core/unit_test/default/TestDefaultDeviceDevelop.cpp Outdated Show resolved Hide resolved

masterleinad reviewed Jun 23, 2022

View reviewed changes

nliber mentioned this pull request Jun 29, 2022

Better name for the InitArguments replacement #5162

Closed

dalg24 changed the title ~~Deprecate InitArguments struct and replace it with <PLACEHOLDER>~~ Deprecate InitArguments struct and replace it with InitializationSettings Jun 30, 2022

dalg24 mentioned this pull request Jul 1, 2022

Avoid InitArguments in interoperability unit tests #5173

Merged

dalg24 added 3 commits July 1, 2022 19:51

Add InitializationSettings as a replacement for the InitArgs struct

b1ffffd

Fixup enable parse cmdline args and env vars test with NVHPC

e1e0006

Add implicit converting constructor from InitArgs to InitiliazationSe…

a288a1a

…ttings

dalg24 force-pushed the new_init_args_struct branch from 7385dbf to fd19f91 Compare July 1, 2022 23:52

dalg24 marked this pull request as ready for review July 1, 2022 23:53

dalg24 force-pushed the new_init_args_struct branch 3 times, most recently from fff711c to 6840b43 Compare July 4, 2022 12:23

dalg24 added 5 commits July 4, 2022 08:43

Avoid InitArguments struct and other drive-by fixes

5447e76

Add deprecated attribute to InitArguments

35ab19f

Rename (int narg, char* argc[]) -> (int argc, char* argv)

15c3e61

Move InitializationSettings to itws own header

62a99a5

Cleanup header includes in unit tests

4b6afa2

dalg24 force-pushed the new_init_args_struct branch from 6840b43 to 45a5850 Compare July 4, 2022 12:43

masterleinad reviewed Jul 5, 2022

View reviewed changes

Guard initialization test

fa2e410

dalg24 force-pushed the new_init_args_struct branch from 45a5850 to fa2e410 Compare July 5, 2022 22:42

Rombur approved these changes Jul 6, 2022

View reviewed changes

masterleinad reviewed Jul 6, 2022

View reviewed changes

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 6, 2022

masterleinad added this to In progress in Kokkos Release 3.7 -- 2022 Target Date via automation Jul 6, 2022

masterleinad added this to the Tentative 3.7 Release milestone Jul 6, 2022

Fixup drop unnecessary cast to void when discarding return value of I…

7135645

…nitiliazationSettings setters in converting constructor

masterleinad approved these changes Jul 7, 2022

View reviewed changes

Merge branch 'develop' of https://github.com/kokkos/kokkos into new_i…

c1348cb

…nit_args_struct

nliber approved these changes Jul 7, 2022

View reviewed changes

dalg24 merged commit 0093b1b into kokkos:develop Jul 7, 2022

Kokkos Release 3.7 -- 2022 Target Date automation moved this from In progress to Done Jul 7, 2022

dalg24 deleted the new_init_args_struct branch July 7, 2022 20:58

mzuzek mentioned this pull request Jul 8, 2022

Fix GitHub CI failing on broken develop kokkos/kokkos-kernels#1461

Merged

4 tasks

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 26, 2022

tpetra: update InitArguments to InitializationSettings in cgsolve

1904590

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

adelus: update InitArguments to InitializationSettings

989353c

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

compadre: update InitArguments to InitializationSettings

a9a154b

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

basker: update InitArguments to InitializationSettings

9f34f61

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

sacado: update InitArguments to InitializationSettings

0eb349e

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

stokhos: update InitArguments to InitializationSettings

723259c

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

adelus: update InitArguments to InitializationSettings

75fd3a5

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

tpetra: update InitArguments to InitializationSettings in cgsolve

2b5b438

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

compadre: update InitArguments to InitializationSettings

4d8ba36

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

basker: update InitArguments to InitializationSettings

40b3569

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

sacado: update InitArguments to InitializationSettings

706fb9f

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

stokhos: update InitArguments to InitializationSettings

78fa342

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 30, 2022

adelus: update InitArguments to InitializationSettings

e359805

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood mentioned this pull request Aug 30, 2022

Kokkos + KokkosKernels Promotion To 3.7.00 trilinos/Trilinos#10962

Merged

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 31, 2022

adelus: more InitArguments and InitializationSettings updates

caa9f80

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Aug 31, 2022

compadre: more InitializationSettings updates

fe70220

compatibility update for kokkos deprecated code removal, corresponding to kokkos/kokkos#5135

ajpowelsnl mentioned this pull request Nov 7, 2022

CHANGELOG: 4.0 #5439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `InitArguments` struct and replace it with `InitializationSettings` #5135

Deprecate `InitArguments` struct and replace it with `InitializationSettings` #5135

dalg24 commented Jun 23, 2022 •

edited

masterleinad left a comment

cz4rs commented Jun 23, 2022 •

edited

nliber commented Jun 23, 2022

PhilMiller commented Jun 23, 2022

masterleinad left a comment

Rombur Jul 6, 2022

dalg24 Jul 6, 2022

Rombur Jul 6, 2022

dalg24 Jul 6, 2022

masterleinad left a comment

masterleinad Jul 6, 2022

dalg24 Jul 6, 2022

masterleinad Jul 6, 2022

dalg24 Jul 6, 2022

masterleinad Jul 6, 2022

dalg24 Jul 7, 2022

masterleinad left a comment

nliber Jul 7, 2022

dalg24 commented Jul 7, 2022

Deprecate InitArguments struct and replace it with InitializationSettings #5135

Deprecate InitArguments struct and replace it with InitializationSettings #5135

Conversation

dalg24 commented Jun 23, 2022 • edited

masterleinad left a comment

Choose a reason for hiding this comment

cz4rs commented Jun 23, 2022 • edited

nliber commented Jun 23, 2022

PhilMiller commented Jun 23, 2022

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 commented Jul 7, 2022

Deprecate `InitArguments` struct and replace it with `InitializationSettings` #5135

Deprecate `InitArguments` struct and replace it with `InitializationSettings` #5135

dalg24 commented Jun 23, 2022 •

edited

cz4rs commented Jun 23, 2022 •

edited