Add partition_space to OpenMP #5105

Rombur · 2022-06-09T20:57:00Z

This is the last part of #4935

The PR does the following:

add a new constructor for OpenMP()
use the ExecutionSpace in impl_thread_pool_size()
use the new impl_thread_pool_size() where necessary
pass the ExecutionSpace by const reference instead of by copy in the default implementation of partition_space. This was already done in the backend implementations
add partition_space for the OpenMP backend. I had to modify a test because the new function emits a warning when the variadic arguments are different types. So you cannot mix int and double anymore. I don't think that's a big deal and the function is still in Experimental.

@dalg24 please do not merge until @crtrott has time to take a look. It has written the partition_space for all the other backends and I would like to have his feedback

dalg24 · 2022-06-09T21:00:57Z

I would never merge anything without him looking :)
I requested a review from him

core/src/Kokkos_Core.hpp

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

dalg24 · 2022-06-09T21:38:53Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+
+  int resources_used = 0;
+  for (unsigned int i = 0; i < weights.size() - 1; ++i) {
+    int instance_pool_size = (weights[i] / total_weight) * main_pool_size;


What if it is 0? Are we delegating this to the instance constructor?
If that is not legal (haven't yet looked at the constructor) should we raise a warning/an error here to produce better diagnostic for the user?

We should do something for. I just realized that there is also a problem if you pass an empty weights vector

The more I think about it, the more I am convinced that we should have a generic version that validates the input and then delegate to backend-specific implementations. That way we can guarantee uniformity. (Not asking you to do that as part of this PR.)

The other backends do not care about the inputs. They are thrown away. The only thing that matters is the number of weights

Do you also want to check for total_weight not to be zero? Maybe, just check weights[i] instead of instance_pool_size?

No I don't plan to check total_weight. The goal is to check that someone doesn't have two weights (1, 99) with main_pool_size = 3. You can do more checks on the input (like negative weight) but I don't believe it's worth the hassle.

masterleinad · 2022-06-16T16:02:42Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+
+  int resources_used = 0;
+  for (unsigned int i = 0; i < weights.size() - 1; ++i) {
+    int instance_pool_size = (weights[i] / total_weight) * main_pool_size;


Do you also want to check for total_weight not to be zero? Maybe, just check weights[i] instead of instance_pool_size?

masterleinad · 2022-06-16T16:04:31Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+    std::cout << "instance " << i << " " << instance_pool_size << " "
+              << weights[i] << " " << total_weight << " " << main_pool_size
+              << std::endl;


Do you intend to keep this output?

crtrott · 2022-06-16T17:11:40Z

Worked a bit with a testcode to figure out whats going on and what should be going on:

#include <Kokkos_Core.hpp>
#include <cmath>

thread_local int id;
int id_counter;

double global_counter;

void busyloop(int N) {
  double tmp = 3.0;
  for(int i=0; i<N; i++)
    tmp += std::pow(tmp,0.9);
  global_counter += tmp;
}

int main(int argc, char* argv[]) {
  Kokkos::initialize(argc, argv);
  {
    int N = argc > 1 ? atoi(argv[1]) : 1000000;
    int R = argc > 2 ? atoi(argv[2]) : 10;

    #pragma omp parallel
    {
       id = 100 + Kokkos::atomic_fetch_inc(&id_counter);
    }

    {
      Kokkos::Timer timer;
      int num_threads = Kokkos::OpenMP().concurrency();
      #pragma omp parallel num_threads(4)
      {
        printf("omp num id nesting level: %i %i %i %i %i\n",int(omp_get_num_threads()),int(omp_get_thread_num()),id,int(omp_get_nested()),int(omp_get_level()));
        #pragma omp parallel num_threads(num_threads/4)
        {
          printf("nested_omp num id nesting level: %i %i %i %i %i\n",int(omp_get_num_threads()),int(omp_get_thread_num()),id,int(omp_get_nested()),int(omp_get_level()));
          busyloop(N);
        }
      }
      printf("time: %lf\n",timer.seconds());
    }

    auto instances = Kokkos::Experimental::partition_space(Kokkos::OpenMP(),0.25,0.25,0.25,0.25);
    {
      Kokkos::Timer timer;
      int num_threads = Kokkos::OpenMP().concurrency();
      #pragma omp parallel num_threads(4)
      {
        auto exec = instances[omp_get_thread_num()];
        printf("omp num id nesting level: %i %i %i %i %i %i\n",int(omp_get_num_threads()),int(omp_get_thread_num()),id,int(omp_get_nested()),int(omp_get_level()),exec.concurrency());
        Kokkos::parallel_for(Kokkos::RangePolicy<>(exec,0,num_threads/4),[=](int i)
        {
          printf("nested_omp num id nesting level: %i (%i %i %i) %i %i\n",int(omp_get_num_threads()),int(omp_get_thread_num()),i,id,int(omp_get_nested()),int(omp_get_level()));
          busyloop(N);
          //Kokkos::parallel_for(Kokkos::RangePolicy<>(exec,0,num_threads/4),[=](int i)
          //{});
        });
      }
      printf("time: %lf\n",timer.seconds());
    }

  }
  Kokkos::finalize();
}

Rombur · 2022-06-22T20:13:59Z

@crtrott I have fixed concurrency. By default, the behavior is unchanged but you can pass an execution space if you have use partition_space. I have added a mutex and locks before accessing m_pool data. I don't think that the code can hang if you launch kernels from different threads. The kernels will be executed serially similar to what is done in the other backends.

Rombur · 2022-07-06T17:49:27Z

@crtrott ping

Rombur · 2022-07-06T20:50:25Z

Retest this please

masterleinad · 2022-08-17T19:04:29Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

-  // static
-  return OpenMP::in_parallel()
+inline int OpenMP::impl_thread_pool_size(OpenMP const& exec_space) noexcept {
+  return OpenMP::in_parallel(exec_space)
             ? omp_get_num_threads()
             : (Impl::t_openmp_instance


In which cases do we actually need to use t_openmp_instance? Is that only necessary for partition_master?

Yes, it's only for partition_master

masterleinad · 2022-08-17T19:07:56Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+    std::cout << "instance " << i << " " << instance_pool_size << " "
+              << weights[i] << " " << total_weight << " " << main_pool_size
+              << std::endl;


Suggested change

std::cout << "instance " << i << " " << instance_pool_size << " "

<< weights[i] << " " << total_weight << " " << main_pool_size

<< std::endl;

masterleinad · 2022-08-17T19:09:17Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+  // Unpack the arguments and create the weight vector. Note that if some of the
+  // types are not same, you will get a narrowing warning.


Suggested change

// Unpack the arguments and create the weight vector. Note that if some of the

// types are not same, you will get a narrowing warning.

// Unpack the arguments and create the weight vector. Note that if not all of the

// types are the same, you will get a narrowing warning.

crtrott

Provided a PR here Rombur#2 fixes some issues exposing others.

dalg24 · 2022-08-29T20:04:16Z

core/src/Kokkos_OpenMP.hpp

@@ -135,8 +137,7 @@ class OpenMP {
      int requested_partition_size = 0);
 #endif

-  // use UniqueToken
-  static int concurrency();
+  static int concurrency(OpenMP const& = OpenMP());


I understand why you had to implement it this way but I feel like this needs a broader design discussion.

Why can't this be a non-static member function as it kind of is supposed to be?

I did that for consistency. The function exists in several backends and it is always static.

OK we need an immediate follow up on this: this is ONLY static in the other guys because someone suggested that it is the same for every instance of currently existing types: it should NOT be static.

dalg24 · 2022-08-29T20:05:38Z

core/src/Kokkos_OpenMP.hpp

@@ -147,13 +148,12 @@ class OpenMP {
  /// \brief Free any resources being consumed by the default execution space
  static void impl_finalize();

-  inline static int impl_thread_pool_size() noexcept;
+  inline static int impl_thread_pool_size(OpenMP const& = OpenMP()) noexcept;


Any particular reason to prefer this to a non-static member function?

I did that for consistency. The function exists in several backends and it is always static.

same as above

dalg24 · 2022-08-29T20:18:49Z

core/unit_test/TestExecSpacePartitioning.hpp

+  ASSERT_NE(exec1.impl_internal_space_instance(),
+            exec2.impl_internal_space_instance());


Would need to be updated after #5398

crtrott · 2022-08-30T14:53:24Z

core/src/Kokkos_OpenMP.hpp

@@ -135,8 +137,7 @@ class OpenMP {
      int requested_partition_size = 0);
 #endif

-  // use UniqueToken
-  static int concurrency();
+  static int concurrency(OpenMP const& = OpenMP());


Why can't this be a non-static member function as it kind of is supposed to be?

crtrott · 2022-08-30T14:53:57Z

core/src/Kokkos_OpenMP.hpp


-  inline static int impl_thread_pool_size(int depth);
+  inline static int impl_thread_pool_size(int depth, OpenMP const& = OpenMP());


why not make this non-static?

I did that for consistency. The function exists in several backends and it is always static.

same as above

core/unit_test/TestExecSpacePartitioning.hpp

core/src/OpenMP/Kokkos_OpenMP_WorkGraphPolicy.hpp

dalg24 · 2022-10-27T13:34:17Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

-int OpenMP::impl_thread_pool_rank() noexcept {
-  // FIXME_OPENMP We are forced to use t_openmp_instance because the function is
-  // static
+inline int OpenMP::impl_thread_pool_rank() noexcept {


Did you mean to drop the KOKKOS_INLINE_FUNCTION annotation? If so we probably should drop the KOKKOS_IF_ON_{HOST,DEVICE}

dalg24 · 2022-10-27T13:43:10Z

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp

+    if (OpenMP::in_parallel() &&
+        !(omp_get_nested() && (omp_get_level() == 1))) {


We check this condition a bunch of times. Did you consider encapsulating it in a function?

The code has significantly changed since the review.

PhilMiller · 2022-10-27T18:33:07Z

core/src/OpenMP/Kokkos_OpenMP_WorkGraphPolicy.hpp

@@ -74,7 +74,11 @@ class ParallelFor<FunctorType, Kokkos::WorkGraphPolicy<Traits...>,

 public:
  inline void execute() {
-#pragma omp parallel num_threads(OpenMP::impl_thread_pool_size())
+    // Work around NVHPC 22.5 ICE
+    int pool_size = OpenMP::impl_thread_pool_size();


Why not [[maybe_unused]] instead of the workaround below?

Sure, I'll change it. The PR started 6 months ago so we couldn't use C++17

masterleinad · 2022-11-30T15:59:54Z

core/src/OpenMP/Kokkos_OpenMP_Instance.cpp

@@ -194,10 +193,7 @@ void OpenMPInternal::resize_thread_data(size_t pool_reduce_bytes,

    memory_fence();


Do you still need the memory fences here and below after serializing the loop?

Also, can you please comment on the necessity to serialize the loop in the first place?

Do you still need the memory fences here and below after serializing the loop?

I think we want to keep this memory_fence but we can get rid of the one in the loop.

Also, can you please comment on the necessity to serialize the loop in the first place?

The problem is when you don't have nested parallelism, you only get rank=0. That's why I am doing the loop in serial to make sure that I really perform the loop.

masterleinad · 2022-11-30T16:14:18Z

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp

+inline bool in_serial() {
+  return (OpenMP::in_parallel() &&
+          !(omp_get_nested() && (omp_get_level() == 1)));
+}


Why does in_serial require in_parallel? Should this function rather be called is_nested or execute_in_serial (also compared to what it's replacing below)?

The problem is not that it's nested. It's that you want to execute the code in parallel but the loop require nested parallelism which was not enable. I don't think that it should be called is_nested because that's not the issue itself. I can change it to execute_in_serial if you prefer.

masterleinad

Looks OK to me (assuming that there will be some cleanup for the static member functions later).

Rombur · 2022-12-05T20:33:10Z

After the rebase on #5601, we are hitting #5651. There is also an error with HIP but it is spurious.

crtrott · 2022-12-07T00:35:05Z

I see about 2-3% performance regression for LAMMPS with in.lj with size 4,2,2. This is on SKX with GCC 11.1 using 12 threads. 2.27s vs 2.32s.

Ran test where I just commented out the lock stuff: and that on its own closes the gap. I guess we can't do anything about that really.

crtrott · 2022-12-07T05:19:11Z

I did some more experiments and implemented a different locking scheme here. Looks like mutex lock implies stronger memory fencing than we need: Rombur#4, and if we relax that a bit we recover the perf.

crtrott · 2022-12-07T17:34:18Z

core/src/OpenMP/Kokkos_OpenMP_Instance.cpp

+                      desul::MemoryScopeDevice());
+}
+
+void release_lock() {}


what is this?

copy/paste error

Co-authored-by: Christian Trott <crtrott@sandia.gov>

dalg24 · 2022-12-07T18:47:54Z

core/src/OpenMP/Kokkos_OpenMP_Instance.cpp

+  int lock = desul::atomic_compare_exchange(&m_pool_mutex, 0, 1,
+                                            desul::MemoryOrderAcquire(),
+                                            desul::MemoryScopeDevice());
+  while (lock == 1)
+    lock = desul::atomic_compare_exchange(&m_pool_mutex, 0, 1,
+                                          desul::MemoryOrderAcquire(),
+                                          desul::MemoryScopeDevice());


Suggested change

int lock = desul::atomic_compare_exchange(&m_pool_mutex, 0, 1,

desul::MemoryOrderAcquire(),

desul::MemoryScopeDevice());

while (lock == 1)

lock = desul::atomic_compare_exchange(&m_pool_mutex, 0, 1,

desul::MemoryOrderAcquire(),

desul::MemoryScopeDevice());

while (1 == desul::atomic_compare_exchange(&m_pool_mutex, 0, 1,

desul::MemoryOrderAcquire(),

desul::MemoryScopeDevice())) {

// nothing

}

dalg24 · 2022-12-07T18:53:35Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

+  void acquire_lock();
+
+  void release_lock();


Comment that these are related to the pool mutex or update the name to reflect how they relate to the thread pool

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp

crtrott · 2022-12-08T00:31:07Z

Here is the Serial fix corresponding to this for the Intel ICE: #5671

dalg24

(not formatted not tested)

dalg24 · 2022-12-08T01:31:23Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

@@ -36,14 +36,18 @@

 #include <omp.h>

+#include <mutex>


Suggested change

#include <mutex>

dalg24 · 2022-12-08T01:35:43Z

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp

@@ -329,7 +333,7 @@ class UniqueToken<OpenMP, UniqueTokenScope::Global> {
  /// \brief acquire value such that 0 <= value < size()
  KOKKOS_INLINE_FUNCTION
  int acquire() const noexcept {
-    KOKKOS_IF_ON_HOST((return Kokkos::Impl::t_openmp_hardware_id;))
+    KOKKOS_IF_ON_HOST((return omp_get_thread_num();))


dalg24 · 2022-12-08T01:48:50Z

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp

    KOKKOS_PRAGMA_IVDEP_IF_ENABLED
-    for (auto iwork = m_policy.begin(); iwork < m_policy.end(); ++iwork) {
+    for (auto iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {


Suggested change

for (auto iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {

for (typename Policy::index_type iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {

dalg24 · 2022-12-08T01:49:10Z

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp

    KOKKOS_PRAGMA_IVDEP_IF_ENABLED
-    for (auto iwork = m_policy.begin(); iwork < m_policy.end(); ++iwork) {
+    for (auto iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {


Suggested change

for (auto iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {

for (typename Policy::index_type iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {

This is for the MDRangePolicy specialization of ParallelFor and ParallelReduce. Removing the extra policy object fixes classic Intel compiler ICE.

dalg24 requested a review from crtrott June 9, 2022 20:59

dalg24 reviewed Jun 9, 2022

View reviewed changes

core/src/Kokkos_Core.hpp Show resolved Hide resolved

dalg24 reviewed Jun 9, 2022

View reviewed changes

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp Show resolved Hide resolved

core/src/OpenMP/Kokkos_OpenMP_Instance.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Jun 9, 2022

View reviewed changes

cz4rs self-requested a review June 10, 2022 18:18

masterleinad reviewed Jun 16, 2022

View reviewed changes

PhilMiller assigned masterleinad Aug 17, 2022

masterleinad requested changes Aug 17, 2022

View reviewed changes

Rombur force-pushed the new_partition_space branch from cda2ca9 to 638578d Compare August 17, 2022 19:43

masterleinad mentioned this pull request Aug 26, 2022

Guard t_openmp_instance with KOKKOS_ENABLE_DEPRECATED_CODE_3 #5396

Merged

masterleinad previously approved these changes Aug 26, 2022

View reviewed changes

crtrott requested changes Aug 29, 2022

View reviewed changes

dalg24 reviewed Aug 30, 2022

View reviewed changes

crtrott requested changes Aug 30, 2022

View reviewed changes

Rombur force-pushed the new_partition_space branch 2 times, most recently from 38385c4 to 5d409bb Compare October 26, 2022 17:59

dalg24 mentioned this pull request Oct 26, 2022

Avoid static/extern thread_local #5601

Merged

dalg24 reviewed Oct 27, 2022

View reviewed changes

Rombur force-pushed the new_partition_space branch from 2fcfe3b to 8a6ef43 Compare October 27, 2022 17:44

PhilMiller reviewed Oct 27, 2022

View reviewed changes

Rombur force-pushed the new_partition_space branch from 8b43366 to 01c47f6 Compare October 28, 2022 17:28

masterleinad requested a review from crtrott November 30, 2022 15:56

masterleinad reviewed Nov 30, 2022

View reviewed changes

masterleinad approved these changes Nov 30, 2022

View reviewed changes

Rombur added 2 commits December 5, 2022 18:09

Fix reviewer's comments

b9f3e22

Add FIXME for UniqueToken

8cd5fdb

Rombur force-pushed the new_partition_space branch from 668551a to 8cd5fdb Compare December 5, 2022 18:12

crtrott requested changes Dec 7, 2022

View reviewed changes

Rombur force-pushed the new_partition_space branch from 20d8f0d to ab2984c Compare December 7, 2022 18:09

Replace std::lock_guard with our own lock

98e5b70

Co-authored-by: Christian Trott <crtrott@sandia.gov>

Rombur force-pushed the new_partition_space branch from ab2984c to 98e5b70 Compare December 7, 2022 18:10

dalg24 reviewed Dec 7, 2022

View reviewed changes

nliber approved these changes Dec 7, 2022

View reviewed changes

Fix reviewer's comments

bd4049e

crtrott force-pushed the new_partition_space branch from c322f35 to f983dda Compare December 7, 2022 22:44

cz4rs reviewed Dec 7, 2022

View reviewed changes

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp Show resolved Hide resolved

cz4rs reviewed Dec 7, 2022

View reviewed changes

core/src/OpenMP/Kokkos_OpenMP_Parallel.hpp Outdated Show resolved Hide resolved

crtrott force-pushed the new_partition_space branch from f983dda to c534f25 Compare December 7, 2022 23:45

crtrott approved these changes Dec 8, 2022

View reviewed changes

crtrott mentioned this pull request Dec 8, 2022

Fix #5651: Intel Classic Compiler ICE #5671

Merged

dalg24 reviewed Dec 8, 2022

View reviewed changes

Remove an unneccesary policy member in OpenMP Impl

27ed831

This is for the MDRangePolicy specialization of ParallelFor and ParallelReduce. Removing the extra policy object fixes classic Intel compiler ICE.

crtrott force-pushed the new_partition_space branch from c534f25 to 27ed831 Compare December 8, 2022 01:59

crtrott merged commit e4cce6e into kokkos:develop Dec 8, 2022

Rombur mentioned this pull request Dec 8, 2022

CHANGELOG: 4.0 #5439

Closed

This was referenced Dec 8, 2022

Fix default memory order for sycl::atomic_ref #5672

Merged

Fix default memory order for sycl::atomic_ref desul/desul#79

Merged

Rombur deleted the new_partition_space branch December 14, 2022 16:30

ndellingwood mentioned this pull request Dec 20, 2022

Nightly test failures with Cuda builds running on device id != 0 #5713

Closed

Rombur mentioned this pull request Feb 1, 2023

Make OpenMP::concurrency and impl_thread_pool_size non-static #5836

Merged

	std::cout << "instance " << i << " " << instance_pool_size << " "
	<< weights[i] << " " << total_weight << " " << main_pool_size
	<< std::endl;

		// Unpack the arguments and create the weight vector. Note that if some of the
		// types are not same, you will get a narrowing warning.

		ASSERT_NE(exec1.impl_internal_space_instance(),
		exec2.impl_internal_space_instance());


		inline static int impl_thread_pool_size(int depth);
		inline static int impl_thread_pool_size(int depth, OpenMP const& = OpenMP());

		if (OpenMP::in_parallel() &&
		!(omp_get_nested() && (omp_get_level() == 1))) {

		@@ -194,10 +193,7 @@ void OpenMPInternal::resize_thread_data(size_t pool_reduce_bytes,

		memory_fence();

	for (auto iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {
	for (typename Policy::index_type iwork = 0; iwork < m_mdr_policy.m_num_tiles; ++iwork) {

Add partition_space to OpenMP #5105

Add partition_space to OpenMP #5105

Conversation

Rombur commented Jun 9, 2022

dalg24 commented Jun 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Jun 16, 2022

Rombur commented Jun 22, 2022

Rombur commented Jul 6, 2022

Rombur commented Jul 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

crtrott 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

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

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

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

Rombur commented Dec 5, 2022

crtrott commented Dec 7, 2022 • edited

crtrott commented Dec 7, 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

crtrott commented Dec 8, 2022

dalg24 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

crtrott commented Dec 7, 2022 •

edited