Add DistributedTree Nearest query with callback #737

masterleinad · 2022-09-09T19:51:09Z

The strategy here is to do a regular query that gets us the MPI ranks, and primitive indices for each query, send them back to the MPI rank owning the primitives, execute the callback, and send them back to the MPI rank owning the queries.

masterleinad · 2022-09-12T19:34:52Z

This should work now replaying the found matches on the remote MPI rank and communicating the results back. Currently, I don't treat the default callback special to validate that this behaves as expected.

masterleinad · 2022-09-15T15:07:38Z

This should be ready for inital review. As said before, the strategy here is to do a regular query that get's us the MPI ranks, and primitive indices for each query, send them back to the MPI rank owning the primitives, execute the callback, and send them back to the MPI rank owning the queries.

masterleinad · 2022-09-15T15:15:41Z

I will still adopt some more tests to use the callback.

masterleinad · 2022-09-22T13:39:46Z

@aprokop ping

examples/distributed_tree/distributed_knn_callback.cpp

dalg24 · 2022-09-26T20:50:30Z

examples/distributed_tree/distributed_knn_callback.cpp

+  {
+    auto data = ArborX::getData(predicate);
+    auto const &point = points(primitive_index);
+    printf("Intersection for query %d from MPI rank %d on MPI rank %d for "


Do we need to guard it with SYCL?

I don't like our saying "intersection". Commenting so we can get back to it.

examples/distributed_tree/distributed_knn_callback.cpp

src/details/ArborX_DetailsDistributedTreeImpl.hpp

src/geometry/ArborX_HyperSphere.hpp

dalg24 · 2022-09-26T21:23:59Z

test/tstDistributedTree.cpp

 using PairIndexRank = Kokkos::pair<int, int>;
+using PairRankIndex = Kokkos::pair<int, int>;


Absolutely not. Aliasing Kokkos::pair was (probably me) already being lazy, this does not protect us against bugs and is awfully confusing.
Any good reason to have both PairIndexRank and PairRankIndex?

As I said before, I need to work more on the tests. Here, I wanted the reverse order to make sure that the new test really executes the callback version properly. I would have gotten the same result ignoring the callback otherwise.

src/details/ArborX_DetailsDistributedTreeImpl.hpp

aprokop

Overall, I think the PR is in the right direction. So far, I've only looked inside src/. I think we need to fix few things in the proposed implementation (most of my comments are superfluous and can be ignored for now):

We need to support pure callbacks
They would use mostly the same code, except the API would not have offset and output views, and they won't need the extra round of backward communication
For non-pure callbacks, we should allow a callback to output variable number of elements, which would necessitate some version of two-pass code.

aprokop · 2022-09-15T20:00:08Z

src/geometry/ArborX_HyperSphere.hpp

-struct ArborX::GeometryTraits::tag<ArborX::ExperimentalHyperGeometry::Sphere<DIM>>
+struct ArborX::GeometryTraits::tag<
+    ArborX::ExperimentalHyperGeometry::Sphere<DIM>>


Is this an intentional change? I think it should be reverted.

.jenkins/continuous.groovy

src/details/ArborX_DetailsDistributedTreeImpl.hpp

aprokop · 2022-12-16T20:06:26Z

src/details/ArborX_DetailsDistributedTreeImpl.hpp

+  // Execute the callback on the process owning the primitives.
+  OutputView remote_out(
+      Kokkos::view_alloc(space, Kokkos::WithoutInitializing,
+                         "ArborX::DistributedTree::query::remote_out"),
+      n_imports);
+  KokkosExt::reallocWithoutInitializing(space, indices, n_imports);
+  Kokkos::parallel_for(
+      "ArborX::DistributedTree::query::execute_callbacks",
+      Kokkos::RangePolicy<ExecutionSpace>(space, 0,
+                                          imported_queries_with_indices.size()),
+      KOKKOS_LAMBDA(int i) {
+        callback(imported_queries_with_indices(i).query,
+                 imported_queries_with_indices(i).primitive_index,
+                 [&](typename OutputView::value_type const &value) {
+                   remote_out(i) = value;
+                   indices(i) = imported_queries_with_indices(i).query_id;
+                 });
+      });


This seems wrong for a few reasons.

First, it assumes that callback will output a single element for each match. In practice, we don't know how many elements the callback will spit out per query. It could be 0, or it could be more than 1. So we need to do a two pass here, similar to the CrsGraphWrapperImpl. I'm not sure what is the easiest way to do it without massive code duplication.

Second, it assumes inline callback and not a post one. We could try to support post callback, but its meaning would have to be changed as we cannot execute it on all of the results coming from different ranks.

We could also just assume that the callback only returns a single element to get started, right?

We could. The thing is, this condition is undetectable, so the failure for a user would likely be an undefined behavior or a segfault. Which in the distributed setting would be tricky to debug.

I added an assertion.

aprokop · 2022-12-16T20:12:25Z

src/details/ArborX_DetailsDistributedTreeImpl.hpp

+  typename DeviceType::memory_space memory_space;
+  Kokkos::View<int *, DeviceType> destinations(
+      Kokkos::view_alloc(space, Kokkos::WithoutInitializing,
+                         "ArborX::DistributedTree::query::destinations"),
+      dest.size());
+  Kokkos::deep_copy(space, destinations, host_destinations);
+  Kokkos::View<int *, DeviceType> offsets(
+      Kokkos::view_alloc(space, Kokkos::WithoutInitializing,
+                         "ArborX::DistributedTree::query::offsets"),
+      off.size());
+  Kokkos::deep_copy(space, offsets, host_offsets);
+  auto const n_imports_back =
+      back_distributor.createFromSends(space, destinations, offsets);
+  KokkosExt::reallocWithoutInitializing(space, out, n_imports_back);
+  Kokkos::View<int *, DeviceType> query_ids(
+      Kokkos::view_alloc(space, Kokkos::WithoutInitializing,
+                         "ArborX::DistributedTree::query::nearest::query_ids"),
+      n_imports_back);
+


This could be simplified by using UnmanagedView from the distributor data pointers, and doing an immediate deep_copoy.

Isn't that what I'm doing already?

src/details/ArborX_DetailsDistributor.hpp

masterleinad · 2022-12-16T20:33:45Z

We need to support pure callbacks
They would use mostly the same code, except the API would not have offset and output views, and they won't need the extra round of backward communication

That's the same as #733, right?

aprokop · 2022-12-16T20:42:35Z

That's the same as #733, right?

No, that was for spatial queries. I need to think a bit about that one.

…allback

This reverts commit 3f66b82.

…allback

masterleinad force-pushed the distributed_knn_callback branch from 00d1ef2 to c74aa9e Compare September 9, 2022 19:55

masterleinad marked this pull request as ready for review September 15, 2022 15:07

aprokop added the enhancement New feature or request label Sep 22, 2022

dalg24 reviewed Sep 26, 2022

View reviewed changes

masterleinad added 22 commits October 11, 2022 13:36

Add DistributedTree nearest example

8223d33

Add DistributedTree intersects example

a6cf7e6

Add DistributedTree intersects with callback example

5cf9a5a

avoid global_mpi_rank

6ce6cd2

Make sure communicateResults is batched by ranks

a552d22

Restoring behavior without calling callback

8587a9d

Convert to lambda

3928d3f

Avoid casts

a637f27

Compiling

ff91ac3

Compiling and running for openmp with 1 MPI rank

6aee1b5

Almost there

1e6ba19

Fix up?

68bc0d5

Remove output

eda8f6b

Reimplement old funcionality with the new implementation

8379b23

Fix up

9857b23

Add example

484db22

Indentation

526d581

Delete other examples

c4ad321

Avoid global_mpi_rank

c666cfb

Move QueriesWithIndices out of function

82505da

Fix memory spaces

86b426a

Indentation

e0d0d79

masterleinad force-pushed the distributed_knn_callback branch from c47b028 to 9402639 Compare October 11, 2022 17:37

masterleinad added 2 commits October 11, 2022 13:46

Don't use lambdas

d26ce07

Don't use Kokkos::pair<int, int> for distributed tree tests

3f66b82

masterleinad force-pushed the distributed_knn_callback branch from 1375f7b to 3f66b82 Compare October 12, 2022 17:02

masterleinad added 3 commits October 13, 2022 20:44

Fix execution spaces

7ac6480

Fix warnings for SYCL

2d19d6e

Fix more warnings

e2a899e

masterleinad mentioned this pull request Oct 21, 2022

New user question: checking use case ECP-copa/Cabana#583

Closed

aprokop reviewed Dec 16, 2022

View reviewed changes

masterleinad added 12 commits January 10, 2023 15:06

Merge remote-tracking branch 'upstream/master' into distributed_knn_c…

74f4a30

…allback

Don't add flags for CUDA-11.0.03-Clang build

7452785

Fix compilation in ArborX_DetailsDistributedTreeImpl.hpp

85ccd16

Avoid spurious whiteline change

e6f43d5

Add ArborX::DistributedTree::query::nearest_callback profiling section

591b89e

Avoid Indices alias

0baa3fd

Rephrase strategy

3ffb9a7

postprocess_callback->execute_callback

901cd91

Assert that the callback is only called once per match

0601e4c

Use camel case

ec544e0

Intersection -> Match

071f409

Don't use ARBORX_ASSERT in device code

0d11bf1

masterleinad mentioned this pull request Jan 11, 2023

Allow named index-rank pairs in parallel tests #816

Closed

masterleinad added 4 commits January 11, 2023 17:09

Also set MPI options for the gcc-12 build

dac4018

Revert "Don't use Kokkos::pair<int, int> for distributed tree tests"

775e55b

This reverts commit 3f66b82.

Merge remote-tracking branch 'upstream/master' into distributed_knn_c…

fc59eca

…allback

Fix warning

498d407

masterleinad mentioned this pull request Aug 17, 2023

Add Moving Least Squares example #919

Closed

aprokop mentioned this pull request Apr 25, 2024

Distributed nearest callback #1075

Merged

		using PairIndexRank = Kokkos::pair<int, int>;
		using PairRankIndex = Kokkos::pair<int, int>;

Add DistributedTree Nearest query with callback #737

Are you sure you want to change the base?

Add DistributedTree Nearest query with callback #737

Uh oh!

Conversation

masterleinad commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masterleinad commented Sep 12, 2022

Uh oh!

masterleinad commented Sep 15, 2022

Uh oh!

masterleinad commented Sep 15, 2022

Uh oh!

masterleinad commented Sep 22, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aprokop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masterleinad commented Dec 16, 2022

Uh oh!

aprokop commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

masterleinad commented Sep 9, 2022 •

edited

Loading

aprokop left a comment •

edited

Loading