Add basic Kokkos support for the original tests of #783 #977

gojakuch · 2024-07-10T12:14:43Z

This commit adds a test from the original Kokkos PR #783 and provides the implementation of the simplest Kokkos features in the form of custom pushforwards. This commit only addresses the forward mode.

gojakuch · 2024-07-10T12:19:33Z

The exact way this has been implemented in this PR is a subject for a discussion. I'm specifically talking about the way I put all the pushforwards in this file in the unittest directory. I also removed the initial tests I'd written in favour of the original ones from the PR #783 that basically cover the same things.

gojakuch · 2024-07-10T12:22:24Z

@vgvassilev as far as I can see, clang-format and some other checks are failing because there's no valid OpenPGP data being found, so it's probably not the PR's fault.

github-actions · 2024-07-10T12:23:31Z

clang-tidy review says "All clean, LGTM! 👍"

codecov · 2024-07-10T12:26:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.80%. Comparing base (ed26dbd) to head (faec3f7).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #977   +/-   ##
=======================================
  Coverage   93.80%   93.80%           
=======================================
  Files          55       55           
  Lines        8065     8065           
=======================================
  Hits         7565     7565           
  Misses        500      500

MihailMihov · 2024-07-10T14:05:30Z

@gojakuch It couldn't resolve the url (curl: (6) Could not resolve host: apt.llvm.org), but the url is working now, so maybe just re-run the checks?

gojakuch · 2024-07-10T14:45:08Z

@MihailMihov thanks, I'll do that a bit later.

unittests/Kokkos/KokkosImplementation.h

unittests/Kokkos/CMakeLists.txt

github-actions

clang-tidy made some suggestions

include/clad/Differentiator/KokkosBuiltins.h

gojakuch · 2024-07-13T18:42:10Z

@vgvassilev the failing check I have right now in this PR is the most important Kokkos one and yet I cannot replicate it on my machine. it works out fine for me. what shall I do in this situation?

vgvassilev · 2024-07-14T04:24:18Z

@vgvassilev the failing check I have right now in this PR is the most important Kokkos one and yet I cannot replicate it on my machine. it works out fine for me. what shall I do in this situation?

Debug on the bots?

gojakuch · 2024-07-14T20:37:25Z

Debug on the bots?

how do I do that?

vgvassilev · 2024-07-15T10:28:08Z

Debug on the bots?

how do I do that?

https://clad.readthedocs.io/en/latest/user/DevelopersDocumentation.html#debugging-github-runners

gojakuch · 2024-07-17T10:07:12Z

@vgvassilev so basically I've managed to debug and fix this on the GitHub runner, but only by modifying a line of code in Clad's main code base. the change is quite small and honestly seems reasonable to me, but that's still something I'd like to discuss during today's meeting. I'm pushing it here for now to have all the checks passing, but maybe it'd be better to have it as a separate commit.

vgvassilev

I think this is heading in a good direction.

@kliegeois, @brian-kelley can you take a look?

vgvassilev · 2024-07-17T20:54:28Z

include/clad/Differentiator/KokkosBuiltins.h

+// pullbacks. Please include it manually to use Clad on Kokkos code.
+
+#ifndef CLAD_KOKKOS_BUILTINS_H
+#define CLAD_KOKKOS_BUILTINS_H


That's a good suggestion.

vgvassilev · 2024-07-17T20:54:57Z

include/clad/Differentiator/KokkosBuiltins.h

+#ifndef CLAD_KOKKOS_BUILTINS_H
+#define CLAD_KOKKOS_BUILTINS_H
+
+#include <Kokkos_Core.hpp>


Maybe wrap this in _has_include?

vgvassilev · 2024-07-17T20:56:34Z

lib/Differentiator/BaseForwardModeVisitor.cpp

@@ -1198,7 +1198,7 @@ StmtDiff BaseForwardModeVisitor::VisitCallExpr(const CallExpr* CE) {
  // If all arguments are constant literals, then this does not contribute to
  // the gradient.
  // FIXME: revert this when this is integrated in the activity analysis pass.
-  if (!callDiff) {
+  if (!callDiff && !CE->getType().getTypePtr()->isVoidType()) {


We should move this as a separate PR with an appropriate test.

kliegeois

Thanks @gojakuch it looks good to me!

include/clad/Differentiator/KokkosBuiltins.h

brian-kelley · 2024-07-17T23:36:36Z

@gojakuch It also looks good to me, I just suggest using variadic template arguments to simplify the constructor_pushforward specializations and support more general Kokkos::View types.

unittests/Kokkos/ViewAccess.cpp

unittests/Kokkos/ViewBasics.cpp

gojakuch · 2024-07-18T11:44:01Z

@vgvassilev the ubu22-clang16-runtime18 check started failing both here and on another PR of mine for some reason related to package setup there, so that's not on my side

vgvassilev · 2024-07-21T18:50:35Z

include/clad/Differentiator/KokkosBuiltins.h

@@ -0,0 +1,55 @@
+// This header file contains the implementation of the Kokkos framework
+// differentiation support in Clad in the form of custom pushforwards and
+// pullbacks. Please include it manually to use Clad on Kokkos code.


Suggested change

// pullbacks. Please include it manually to use Clad on Kokkos code.

// pullbacks. Please include it manually to enable Clad for Kokkos code.

vgvassilev · 2024-07-21T18:51:08Z

include/clad/Differentiator/KokkosBuiltins.h

+#define CLAD_KOKKOS_BUILTINS_H
+
+#include <Kokkos_Core.hpp>
+#include "clad/Differentiator/Differentiator.h"


We can ignore that suggestion.

vgvassilev · 2024-07-21T18:51:39Z

include/clad/Differentiator/KokkosBuiltins.h

+namespace Kokkos {
+template <typename View1, typename View2, typename T>
+inline void deep_copy_pushforward(const View1& dst, const View2& src, T param,
+                                  const View1& _d_dst, const View2& _d_src,


These suggestions are relevant. The _ identifiers are normally reserved for special builtins and we are not doing anything special here.

This commit adds a test from the original Kokkos PR vgvassilev#783 and provides the implementation of the simplest Kokkos features in the form of custom pushforwards. This commit only addresses the forward mode.

vgvassilev

LGTM!

kliegeois · 2024-07-22T19:35:44Z

Thanks a lot for having merged the PR @vgvassilev and @gojakuch !

gojakuch force-pushed the kokkos-support branch from 7fa0c09 to 85f0bf2 Compare July 10, 2024 12:17

gojakuch marked this pull request as draft July 10, 2024 12:26

vgvassilev reviewed Jul 10, 2024

View reviewed changes

unittests/Kokkos/KokkosImplementation.h Outdated Show resolved Hide resolved

vgvassilev reviewed Jul 11, 2024

View reviewed changes

unittests/Kokkos/CMakeLists.txt Show resolved Hide resolved

gojakuch force-pushed the kokkos-support branch 2 times, most recently from 35000d0 to 9244e85 Compare July 13, 2024 15:05

This comment was marked as outdated.

Sign in to view

gojakuch force-pushed the kokkos-support branch 3 times, most recently from e0d814e to dfd46aa Compare July 13, 2024 18:24

github-actions bot reviewed Jul 13, 2024

View reviewed changes

include/clad/Differentiator/KokkosBuiltins.h Show resolved Hide resolved

gojakuch force-pushed the kokkos-support branch 2 times, most recently from 79d9de3 to 3c08112 Compare July 17, 2024 09:59

gojakuch marked this pull request as ready for review July 17, 2024 10:08

gojakuch requested a review from vgvassilev July 17, 2024 14:34

vgvassilev reviewed Jul 17, 2024

View reviewed changes

kliegeois reviewed Jul 17, 2024

View reviewed changes

brian-kelley reviewed Jul 17, 2024

View reviewed changes

include/clad/Differentiator/KokkosBuiltins.h Outdated Show resolved Hide resolved

vgvassilev reviewed Jul 18, 2024

View reviewed changes

unittests/Kokkos/ViewAccess.cpp Outdated Show resolved Hide resolved

vgvassilev reviewed Jul 18, 2024

View reviewed changes

unittests/Kokkos/ViewBasics.cpp Outdated Show resolved Hide resolved

unittests/Kokkos/ViewBasics.cpp Outdated Show resolved Hide resolved

gojakuch force-pushed the kokkos-support branch 2 times, most recently from 7260cd0 to d154949 Compare July 18, 2024 09:51

This comment was marked as off-topic.

Sign in to view

gojakuch mentioned this pull request Jul 18, 2024

Prevent Clad from trying to create a void zero literal #989

Merged

vgvassilev force-pushed the kokkos-support branch from d154949 to 2443ebc Compare July 21, 2024 18:46

vgvassilev reviewed Jul 21, 2024

View reviewed changes

gojakuch force-pushed the kokkos-support branch from 2443ebc to 09acf5b Compare July 22, 2024 18:38

Add basic Kokkos support for the original tests of vgvassilev#783

faec3f7

This commit adds a test from the original Kokkos PR vgvassilev#783 and provides the implementation of the simplest Kokkos features in the form of custom pushforwards. This commit only addresses the forward mode.

gojakuch force-pushed the kokkos-support branch from 09acf5b to faec3f7 Compare July 22, 2024 18:45

vgvassilev approved these changes Jul 22, 2024

View reviewed changes

vgvassilev merged commit f03fc98 into vgvassilev:master Jul 22, 2024
89 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic Kokkos support for the original tests of #783 #977

Add basic Kokkos support for the original tests of #783 #977

gojakuch commented Jul 10, 2024

gojakuch commented Jul 10, 2024

gojakuch commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

MihailMihov commented Jul 10, 2024

gojakuch commented Jul 10, 2024

This comment was marked as outdated.

github-actions bot left a comment

gojakuch commented Jul 13, 2024

vgvassilev commented Jul 14, 2024

gojakuch commented Jul 14, 2024

vgvassilev commented Jul 15, 2024

gojakuch commented Jul 17, 2024

vgvassilev left a comment

vgvassilev Jul 17, 2024

vgvassilev Jul 17, 2024

vgvassilev Jul 17, 2024

kliegeois left a comment

brian-kelley commented Jul 17, 2024 •

edited

Loading

This comment was marked as off-topic.

gojakuch commented Jul 18, 2024 •

edited

Loading

vgvassilev Jul 21, 2024

vgvassilev Jul 21, 2024

vgvassilev Jul 21, 2024

vgvassilev left a comment

kliegeois commented Jul 22, 2024

	// pullbacks. Please include it manually to use Clad on Kokkos code.
	// pullbacks. Please include it manually to enable Clad for Kokkos code.

Add basic Kokkos support for the original tests of #783 #977

Add basic Kokkos support for the original tests of #783 #977

Conversation

gojakuch commented Jul 10, 2024

gojakuch commented Jul 10, 2024

gojakuch commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

MihailMihov commented Jul 10, 2024

gojakuch commented Jul 10, 2024

This comment was marked as outdated.

github-actions bot left a comment

Choose a reason for hiding this comment

gojakuch commented Jul 13, 2024

vgvassilev commented Jul 14, 2024

gojakuch commented Jul 14, 2024

vgvassilev commented Jul 15, 2024

gojakuch commented Jul 17, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

vgvassilev Jul 17, 2024

Choose a reason for hiding this comment

vgvassilev Jul 17, 2024

Choose a reason for hiding this comment

vgvassilev Jul 17, 2024

Choose a reason for hiding this comment

kliegeois left a comment

Choose a reason for hiding this comment

brian-kelley commented Jul 17, 2024 • edited Loading

This comment was marked as off-topic.

gojakuch commented Jul 18, 2024 • edited Loading

vgvassilev Jul 21, 2024

Choose a reason for hiding this comment

vgvassilev Jul 21, 2024

Choose a reason for hiding this comment

vgvassilev Jul 21, 2024

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

kliegeois commented Jul 22, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

brian-kelley commented Jul 17, 2024 •

edited

Loading

gojakuch commented Jul 18, 2024 •

edited

Loading