Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3048

bartlettroscoe · 2020-05-19T16:10:51Z

This fixes the tests KokkosCore_UnitTest_DefaultInit__MPI_1 that fail when
running with ctest GPU allocation feature that fail becuase they don't run on
GPU device 0 (see #3040).

Note: Since get_ctest_gpu() returns 0 if CTest has not provided anything, it
is safe to always call it.

This new function get_gpu() should really be unit tested on its own.

This fixes the tests KokkosCore_UnitTest_DefaultInit_<x>_MPI_1 that fail when running with ctest GPU allocation feature that fail becuase they don't run on GPU device 0 (see kokkos#3040). Note: Since get_ctest_gpu() returns 0 if CTest has not provided anything, it is safe to always call it. This new function get_gpu() should really be unit tested on its own.

dalg24-jenkins · 2020-05-19T16:10:53Z

Can one of the admins verify this patch?

codecov-commenter · 2020-05-19T16:46:46Z

Codecov Report

Merging #3048 into master will decrease coverage by 0.2%.
The diff coverage is 70.0%.

@@           Coverage Diff            @@
##           master   #3048     +/-   ##
========================================
- Coverage    80.1%   79.9%   -0.3%     
========================================
  Files         121     121             
  Lines        7764    7785     +21     
========================================
+ Hits         6220    6221      +1     
- Misses       1544    1564     +20

Flag	Coverage Δ
#clang	`78.8% <83.3%> (ø)`
#gcc	`79.8% <70.0%> (-0.3%)`	⬇️

Impacted Files	Coverage Δ
core/src/impl/Kokkos_Core.cpp	`37.3% <70.0%> (-1.6%)`	⬇️
core/src/impl/Kokkos_HostThreadTeam.cpp	`96.8% <0.0%> (+0.7%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d085fd...7c0f967. Read the comment docs.

masterleinad

I believe this can be fixed in a simpler way.
Also, it would be good to target the develop branch first.

masterleinad · 2020-05-19T17:34:11Z

core/unit_test/TestDefaultDeviceTypeInit.hpp

@@ -273,7 +281,7 @@ void check_correct_initialization(const Kokkos::InitArguments& argstruct) {

    int expected_device = argstruct.device_id;
    if (argstruct.device_id < 0) {
-      expected_device = 0;
+      expected_device = Kokkos::Impl::get_gpu(argstruct);


Can't you just do

Suggested change

expected_device = Kokkos::Impl::get_gpu(argstruct);

expected_device = Kokkos::Cuda().cuda_device();

instead? I think we can ask the execution space for the device id it was initialized with.

I will give that a try.

Switching to:

expected_device = Kokkos::Cuda().cuda_device();

seems to work as well. The updated commit is 0e51c8d

Even though not needed, I would argue that you should keep the refactoring to pull out the Kokkos::Impl::get_gpu(). That function could be unit tested in isolation much better and make this more solid and robust.

This function would only be used in this context to initialize the device (id) properly and this id is available via Kokkos::Cuda().cuda_device(). Hence, I don't see a reason to test the former rather than the latter.
I don't feel strongly about pulling it out, though. This looks good enough for me.

bartlettroscoe · 2020-05-19T17:58:15Z

The above coverage report does not make sense. How can the refactoring I did decrease code coverage? And why is it comparing to 'master' instead of 'develop' (the target if this PR)?

…s#3040, trilinos/Trilinos#6840) This seems to also fix the KokkosCore_UnitTest_DefaultInit_<x>_MPI_1 tests that where failing when running with ctest GPU allocation feature that fail because they don't run on GPU device 0 (see kokkos#3040).

dalg24 · 2020-05-20T01:12:03Z

OK to test

dalg24 · 2020-05-20T01:23:35Z

core/src/impl/Kokkos_Core.cpp

-#endif
-#if defined(KOKKOS_ENABLE_CUDA) || defined(KOKKOS_ENABLE_ROCM) || \
-    defined(KOKKOS_ENABLE_HIP)
+int get_gpu(const InitArguments& args) {


Why did you define it outside the anonymous namespace?

I just followed the example of get_ctest_gpu() that was also in that file:

kokkos/core/src/impl/Kokkos_Core.cpp

Line 80 in 0e51c8d

int get_ctest_gpu(const char* local_rank_str) {

Also, implementation functions like this should be independently unit tested. Having this in Kokkos::Impl instead of an anonymous namespace would allow that.

Right that's why get_ctest_gpu is outside and we test it in core/unit_test/TestCTestDevice.cpp using a forward declaration.

But get_gpu does not have dedicated tests and can (and IMO should) be within that anonymous namespace.

It is fine to move git_gpu() into an anonymous namespace for now. When someone gets around to unit testing it, it can be moved back to the Kokkos::Impl namespace.

But get_gpu does not have dedicated tests and can (and IMO should) be within that anonymous namespace.

@dalg24, completed in 7c0f967.

I did not test this last change. I guess we will test the Kokkos PR testing :-)

bartlettroscoe · 2020-05-20T17:05:36Z

OK to test

@dalg24, does this trigger the Jenkins tester?

dalg24 · 2020-05-20T20:02:51Z

OK to test

@dalg24, does this trigger the Jenkins tester?

Yes

bartlettroscoe · 2020-05-20T20:09:53Z

OK to test

This was referenced May 19, 2020

DefaultInit tests failing when using CTest resource allocation feature #3040

Closed

Use RESOURCE_GROUPS property of CTest trilinos/Trilinos#6840

Merged

masterleinad self-assigned this May 19, 2020

masterleinad requested changes May 19, 2020

View reviewed changes

masterleinad approved these changes May 19, 2020

View reviewed changes

kokkos deleted a comment from masterleinad May 20, 2020

dalg24 reviewed May 20, 2020

View reviewed changes

Move get_gpu() to anonymous namespace (kokkos#3040)

7c0f967

crtrott merged commit 953d796 into kokkos:master May 20, 2020

bartlettroscoe mentioned this pull request May 22, 2020

Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3061

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3048

Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3048

bartlettroscoe commented May 19, 2020

dalg24-jenkins commented May 19, 2020

codecov-commenter commented May 19, 2020 •

edited

masterleinad left a comment

masterleinad May 19, 2020

bartlettroscoe May 19, 2020

bartlettroscoe May 19, 2020

masterleinad May 19, 2020

bartlettroscoe commented May 19, 2020 •

edited

dalg24 commented May 20, 2020

dalg24 May 20, 2020

bartlettroscoe May 20, 2020

dalg24 May 20, 2020

bartlettroscoe May 20, 2020 •

edited

bartlettroscoe May 20, 2020

bartlettroscoe May 20, 2020

bartlettroscoe commented May 20, 2020

dalg24 commented May 20, 2020

bartlettroscoe commented May 20, 2020

	expected_device = Kokkos::Impl::get_gpu(argstruct);
	expected_device = Kokkos::Cuda().cuda_device();

Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3048

Extract and use get_gpu() (kokkos/kokkos#3040, trilinos/Trilinos#6840) #3048

Conversation

bartlettroscoe commented May 19, 2020

dalg24-jenkins commented May 19, 2020

codecov-commenter commented May 19, 2020 • edited

Codecov Report

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

bartlettroscoe commented May 19, 2020 • edited

dalg24 commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlettroscoe May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlettroscoe commented May 20, 2020

dalg24 commented May 20, 2020

bartlettroscoe commented May 20, 2020

codecov-commenter commented May 19, 2020 •

edited

bartlettroscoe commented May 19, 2020 •

edited

bartlettroscoe May 20, 2020 •

edited