core: add is_team_handle and test #5375

fnrizzi · 2022-08-23T12:02:30Z

Content

Impl of is_team_handle (based on this concept syntax) and a test.

Notes/caveats

test is disabled for OpenMPTarget because it would fail due to this :

  // FIXME_OPENMPTARGET this function has the wrong interface and currently
  // ignores the reducer passed.
  template <class ValueType, class JoinOp>
  KOKKOS_INLINE_FUNCTION ValueType team_reduce(const ValueType& value,
                                               const JoinOp&) const {

disabled for OpenACC because the team handle impl does not conform to the concept (must be fixed) will open issue for this

fnrizzi · 2022-08-23T15:03:01Z

retest this please

fnrizzi · 2022-08-23T15:08:37Z

retest this please

fnrizzi · 2022-08-24T08:48:46Z

retest this please

fnrizzi · 2022-08-24T08:52:46Z

CI failures are due to outofspace

core/src/Kokkos_Concepts.hpp

core/unit_test/TestConcepts.hpp

core/src/Kokkos_Concepts.hpp

JBludau

It is good, that we check the interface of team_handle . But do we actually require the values the team_handle returns to fulfill comply with an expected behavior?

core/src/Kokkos_Concepts.hpp

masterleinad · 2022-08-25T17:45:43Z

What is this used for?

fnrizzi · 2022-09-05T08:20:22Z

decided to:

simplify trait detection to conform to how all other concepts are done
use the full trait detection to write a test

jstrzebonski · 2022-10-27T13:51:28Z

decided to:

* simplify trait detection to conform to how all other concepts are done

* use the full trait detection to write a test

@dalg24 @masterleinad
In that case should we implement is_team_handle with KOKKOS_IMPL_IS_CONCEPT, similary to, for example, is_execution_space?

As far as I understand, that implementation would add KOKKOS_IMPL_IS_CONCEPT(team_handle) to Kokkos_Concepts.hpp, and then for every Team that we expect is_team_handle<T> to return true, we should add alias using team_handle = .... And basically, that's it.

fnrizzi · 2022-10-27T13:56:29Z

@jstrzebonski can you please post also the snippets you showed me today? So that is more clear what we propose

jstrzebonski · 2022-10-27T14:03:43Z

For example is_execution_space for Kokkos::Cuda gives true, because of the alias added to Cuda class:

class Cuda {
 public:
  //! \name Type declarations that all Kokkos execution spaces must provide.
  //@{

  //! Tag this class as a kokkos execution space
  using execution_space = Cuda;

...

Similary is_team_handle for CudaTeamMember would require adding team_handle alias, for example:

class CudaTeamMember {
 public:
  using execution_space      = Kokkos::Cuda;
  using scratch_memory_space = execution_space::scratch_memory_space;

  using team_handle = CudaTeamMember;

  ...

fnrizzi · 2022-11-28T17:00:41Z

failure is unrelated

crtrott

Not sure that we want to get into the business of implementing tests of tests?

core/unit_test/TestConcepts.hpp

fnrizzi · 2022-12-06T08:27:23Z

failure is unrelated

core/unit_test/TestConcepts.hpp

dalg24 · 2022-12-06T13:13:56Z

core/unit_test/TestConcepts.hpp

+/*-------------------------------------------------
+  begin test for team_handle concept
+
+  Read before moving on to the test below:
+  The implementation inside Kokkos/core of the team_handle concept follows
+  that of execution space, memory space, etc as shown here:
+
+    https://github.com/kokkos/kokkos/blob/61d7db55fceac3318c987a291f77b844fd94c165/core/src/Kokkos_Concepts.hpp#L160
+
+  which has a key aspect: for performance reasons, it does *not* check the
+  complete trait. So below we also provide a complete team handle concept trait
+  based on this
+
+    https://kokkos.github.io/kokkos-core-wiki/API/core/policies/TeamHandleConcept.html
+
+  which completely checks the trait and we use to validate things.
+  This complete trait was originally used as implementation but was moved
+  here as discussed in this PR: https://github.com/kokkos/kokkos/pull/5375
+
+  ------------------------------------------------- */


Suggested change

/*-------------------------------------------------

begin test for team_handle concept

Read before moving on to the test below:

The implementation inside Kokkos/core of the team_handle concept follows

that of execution space, memory space, etc as shown here:

https://github.com/kokkos/kokkos/blob/61d7db55fceac3318c987a291f77b844fd94c165/core/src/Kokkos_Concepts.hpp#L160

which has a key aspect: for performance reasons, it does *not* check the

complete trait. So below we also provide a complete team handle concept trait

based on this

https://kokkos.github.io/kokkos-core-wiki/API/core/policies/TeamHandleConcept.html

which completely checks the trait and we use to validate things.

This complete trait was originally used as implementation but was moved

here as discussed in this PR: https://github.com/kokkos/kokkos/pull/5375

------------------------------------------------- */

why you want to remove this? 12 months from now somebody will look at this and wonder why the full trait is here... the comment helps IMO

20 lines to say something (arguably) obvious. Can you say in one or two lines?

I changed it quite a bit, hope it is ok now

core/unit_test/TestConcepts.hpp

dalg24 · 2022-12-06T13:25:13Z

core/unit_test/TestConcepts.hpp

+// actual test begins here
+
+/*
+  disabling as follows:
+
+  - OpenMPTARGET: due to this
+    https://github.com/kokkos/kokkos/blob/2d6cbad7e079eb45ae69ac6a59929d9fcf10409a/core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp#L860
+
+    - OpenACC: not supporting teams yet
+   */


Suggested change

// actual test begins here

/*

disabling as follows:

- OpenMPTARGET: due to this

https://github.com/kokkos/kokkos/blob/2d6cbad7e079eb45ae69ac6a59929d9fcf10409a/core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp#L860

- OpenACC: not supporting teams yet

*/

// FIXME_OPENMPTARGET https://github.com/kokkos/kokkos/blob/2d6cbad7e079eb45ae69ac6a59929d9fcf10409a/core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.hpp#L860-L864

@rgayatri23 please advise why the interface departure is not something that can be fixed now

There is some support for teams that was merged in the OpenACC backend. Did you check since that change?

trying now again

https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/11320/pipeline/47

OpenACCTeamMember is missing the following method:

template <typename ReducerType> KOKKOS_INLINE_FUNCTION std::enable_if_t<is_reducer<ReducerType>::value> team_reduce(ReducerType const& reducer) const noexcept;

dalg24 · 2022-12-06T21:37:50Z

core/unit_test/TestConcepts.hpp

+
+template <class T>
+inline constexpr bool is_team_handle_complete_trait_check_v =
+    is_team_handle_complete_trait_check<T>::value;


Suggested change

is_team_handle_complete_trait_check<T>::value;

is_team_handle_complete_trait_check<T>::value && Kokkos::is_team_handle_v<T>;

that way you don't need to duplicate all that code below

I think its fine. Makes it more obvious that the primary team handle is tested.

crtrott · 2022-12-12T15:50:24Z

core/unit_test/TestConcepts.hpp

+
+template <class T>
+inline constexpr bool is_team_handle_complete_trait_check_v =
+    is_team_handle_complete_trait_check<T>::value;


I think its fine. Makes it more obvious that the primary team handle is tested.

fnrizzi mentioned this pull request Aug 23, 2022

team impl of std algorithms #5150

Closed

fnrizzi changed the title ~~core: add is_team_handle and test~~ [ready] core: add is_team_handle and test Aug 23, 2022

fnrizzi changed the title ~~[ready] core: add is_team_handle and test~~ core: add is_team_handle and test Aug 23, 2022

fnrizzi marked this pull request as draft August 23, 2022 13:07

fnrizzi changed the title ~~core: add is_team_handle and test~~ [ready] core: add is_team_handle and test Aug 23, 2022

fnrizzi marked this pull request as ready for review August 23, 2022 15:03

fnrizzi requested review from dalg24, nliber, crtrott, cz4rs and masterleinad August 23, 2022 15:45

dalg24 reviewed Aug 24, 2022

View reviewed changes

crtrott approved these changes Aug 25, 2022

View reviewed changes

JBludau reviewed Aug 25, 2022

View reviewed changes

core/src/Kokkos_Concepts.hpp Outdated Show resolved Hide resolved

fnrizzi marked this pull request as draft August 25, 2022 18:11

fnrizzi force-pushed the is_team_handle branch from 1de7272 to e954e27 Compare November 28, 2022 09:33

fnrizzi changed the title ~~[ready] core: add is_team_handle and test~~ core: add is_team_handle and test Nov 28, 2022

fnrizzi added this to In progress in Developer: FRANCESCO RIZZI Nov 28, 2022

fnrizzi marked this pull request as ready for review November 28, 2022 11:12

fnrizzi requested a review from PhilMiller November 28, 2022 11:12

fnrizzi force-pushed the is_team_handle branch from 4986ee3 to 0163b47 Compare December 5, 2022 16:07

crtrott requested changes Dec 5, 2022

View reviewed changes

core/unit_test/TestConcepts.hpp Show resolved Hide resolved

fnrizzi added 14 commits December 5, 2022 19:05

core: add is_team_handle and test

2b1ccb1

core: add qualifier

9066102

core: remove qualifier

b7950d0

core: fix compile issue

5c13e89

core: renable all checks for is_team_handle

83105d6

core: disable tests for valid reasons

c5cb2af

core: fix typo

93426dd

core: is_team_handle test: disable the correct section

60d47bf

core: is_team_handle: address review comments

dce5c60

is_team_handle concept: update impl and test

a08f47f

remove guard for omp target in test

e703a27

readd guards for team_handle test

633a766

fix typo in guard in test

ef568c6

is_team_handle: address comments

e4db62e

fnrizzi force-pushed the is_team_handle branch from 0163b47 to e4db62e Compare December 5, 2022 18:05

dalg24 reviewed Dec 6, 2022

View reviewed changes

fnrizzi added 4 commits December 6, 2022 14:37

address more PR comments

2c83aa9

remove namespace

666679b

more PR comments

3bdd5d1

readd OPENACC guard in test

31a74bf

fnrizzi force-pushed the is_team_handle branch from ccc3487 to 31a74bf Compare December 6, 2022 15:25

dalg24 approved these changes Dec 6, 2022

View reviewed changes

crtrott approved these changes Dec 12, 2022

View reviewed changes

crtrott merged commit 6a49537 into kokkos:develop Dec 12, 2022

fnrizzi mentioned this pull request Feb 1, 2023

CHANGELOG: 4.0 #5439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add is_team_handle and test #5375

core: add is_team_handle and test #5375

fnrizzi commented Aug 23, 2022 •

edited

fnrizzi commented Aug 23, 2022

fnrizzi commented Aug 23, 2022

fnrizzi commented Aug 24, 2022

fnrizzi commented Aug 24, 2022

JBludau left a comment •

edited

masterleinad commented Aug 25, 2022

fnrizzi commented Sep 5, 2022

jstrzebonski commented Oct 27, 2022 •

edited

fnrizzi commented Oct 27, 2022 •

edited

jstrzebonski commented Oct 27, 2022

fnrizzi commented Nov 28, 2022

crtrott left a comment

fnrizzi commented Dec 6, 2022

dalg24 Dec 6, 2022

fnrizzi Dec 6, 2022

dalg24 Dec 6, 2022

fnrizzi Dec 6, 2022

dalg24 Dec 6, 2022

fnrizzi Dec 6, 2022

fnrizzi Dec 6, 2022 •

edited

dalg24 Dec 6, 2022

crtrott Dec 12, 2022

crtrott Dec 12, 2022

	is_team_handle_complete_trait_check<T>::value;
	is_team_handle_complete_trait_check<T>::value && Kokkos::is_team_handle_v<T>;

core: add is_team_handle and test #5375

core: add is_team_handle and test #5375

Conversation

fnrizzi commented Aug 23, 2022 • edited

Content

Notes/caveats

fnrizzi commented Aug 23, 2022

fnrizzi commented Aug 23, 2022

fnrizzi commented Aug 24, 2022

fnrizzi commented Aug 24, 2022

JBludau left a comment • edited

Choose a reason for hiding this comment

masterleinad commented Aug 25, 2022

fnrizzi commented Sep 5, 2022

jstrzebonski commented Oct 27, 2022 • edited

fnrizzi commented Oct 27, 2022 • edited

jstrzebonski commented Oct 27, 2022

fnrizzi commented Nov 28, 2022

crtrott left a comment

Choose a reason for hiding this comment

fnrizzi commented Dec 6, 2022

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

fnrizzi Dec 6, 2022 • edited

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

fnrizzi commented Aug 23, 2022 •

edited

JBludau left a comment •

edited

jstrzebonski commented Oct 27, 2022 •

edited

fnrizzi commented Oct 27, 2022 •

edited

fnrizzi Dec 6, 2022 •

edited