Core type system #697

magnatelee · 2023-04-21T06:37:10Z

This PR adds a common type system to Legate core. The type system supports primitive types, fixed size array types, and struct types. Struct types can optionally be aligned, which is often what the C++ counterpart expects. As a part of the refactoring, the ugly handling of tuples in legate::Scalar has been improved with the new fixed-size array type so everything is under the same type system.

magnatelee · 2023-04-21T06:41:12Z

@manopapad @jjwilke @bryevdv this PR is fairly big, so I thought it'd be helpful to have more than one pair of eyes on the code.

bryevdv

Great improvement

src/core/legate_c.h

legate/core/runtime.py

src/core/utilities/deserializer.inl

src/core/type/type_info.h

src/core/runtime/runtime.cc

legate/core/types.pyi

jjwilke

LGTM. A couple nits. Main question is on Store lifetimes related to reference_wrapper and std::unique_ptr

jjwilke · 2023-04-24T22:37:16Z

examples/hello/editable-install.sh

@@ -1,5 +1,5 @@
 legate_root=`python -c 'import legate.install_info as i; from pathlib import Path; print(Path(i.libpath).parent.resolve())'`
 echo "Using Legate at $legate_root"
 cmake -S . -B build -D legate_core_ROOT=$legate_root
-cmake --build build
+cmake --build build -j 8


I would call --parallel instead of -j 8

src/core/utilities/deserializer.inl

jjwilke · 2023-04-24T23:03:36Z

src/core/mapping/mapping.cc

@@ -220,7 +220,7 @@ void StoreMapping::populate_layout_constraints(
 {
  StoreMapping mapping{};
  mapping.policy = InstanceMappingPolicy::default_policy(target, exact);
-  mapping.stores.push_back(store);
+  mapping.stores.emplace_back(store);


I'm a bit uncomfortable with the change here. Store has changed from a lightweight, copyable handle to a move-only class. The reference_wrapper stuff here has potential lifetime issues. I'm sure the current implementation is correct, but I don't love creating a vector of references that returns from a function.

Can these type objects just be statics? They are going to get reused quite a bit, yeah? Would it be better to move from unique_ptr<Type> to Type* where Type is an object with static lifetime?

Note that we don't want to pass around pointers in the distributed setting, so Type* needs to be deserialized somehow in the mapper. We can certainly make a singleton type object for each distinct type, but then we need a centralized registry of Type* objects for the deserializer (so it can recover a Type* from its serialized form) , which would then introduce synchronization between the control code registering types and every deserialization attempt. I don't actually want to introduce this. Though I don't like how it's set up, I feel it's reasonable because store mappings cannot exist without stores nor can they outlive them.

jjwilke · 2023-04-24T23:20:22Z

src/core/runtime/runtime.inl

-  Legion::Runtime::perform_registration_callback(
-    detail::invoke_legate_registration_callback<CALLBACK>, true /*global*/);
+  auto runtime = Runtime::get_runtime();
+  if (runtime->is_in_callback())


What would cause multiple callbacks to get invoked recursively? Also - can we guarantee that perform_registration_callback will actually invoke the callback? Doesn't it sometimes defer the callback?

If Legion defers the callback until later, it might invoke the callback when runtime->is_in_callback is false?

Callbacks don't get deferred with Legion::Runtime::perform_registration_callback; it's add_registration_callback that defers them.

jjwilke

LGTM

manopapad · 2023-05-04T04:58:34Z

legate/core/_lib/types.pyx

+    ) except+
+
+
+cdef Dtype from_ptr(Type* ty):


Would it be possible to use forwarding references here, similar to https://github.com/cython/cython/blob/master/tests/run/cpp_forwarding_ref.pyx, to avoid having to release() then reset() the unique_ptrs?

manopapad · 2023-05-04T05:15:58Z

legate/core/_lib/types.pyx

+    def serialize(self, buf) -> None:
+        buf.pack_32bit_int(self.code)
+        buf.pack_32bit_int(self.uid)
+        buf.pack_32bit_int(self.num_elements())


Technically buf.pack_32bit_uint, but I doubt it matters.

manopapad · 2023-05-04T05:21:18Z

legate/core/_lib/types.pyx

+        buf.pack_32bit_int(self.code)
+        num_fields = self.num_fields()
+        buf.pack_32bit_int(self.uid)
+        buf.pack_32bit_int(num_fields)


Similarly technically buf.pack_32bit_uint

manopapad · 2023-05-04T05:28:34Z

src/core/data/scalar.h

@@ -49,11 +49,10 @@ class Scalar {
   * @brief Creates a shared `Scalar` with an existing allocation. The caller is responsible
   * for passing in a sufficiently big allocation.
   *
-   * @param tuple If true, the allocation contains a tuple of scalars.
   * @param code Type code of the scalar(s)


Update argument doc

manopapad · 2023-05-04T05:32:12Z

src/core/data/scalar.h

   */
-  bool is_tuple() const { return tuple_; }
+  const Type* type() const { return type_.get(); }


Could probably return a const Type& here, like Store.type() does, to avoid exposing the pointer.

refactored in nv-legate/legate#697

magnatelee added 17 commits April 17, 2023 10:46

Rename _Dtype to Dtype, as it's now going to be a user-facing type

fafd6b0

Refactor type codes as part of the prep for the core type system

dbfdf4c

Cython module for type codes and rename float types

bed226d

Merge branch 'branch-23.05' into core-type-system

86fa2b3

Start using data type objects

bf758d7

Bind C++ data type classes to Python

56064df

Doxygen for data type classes

3645796

Finish a complete loop for the new type system

c3cfbea

Remaining logic for custom reduction operators

0b47f32

Alignment for struct types

0e455f3

Clean up abstractions for legate data interface

47a91ab

Implement a proper class hierarchy for dtypes in Python

7606e78

Replace a hacky tuple type for scalars with a proper array type

86a4c10

Fix for string scalars

1a44e67

Add legate data interface for stores

a4a33fe

Fix tutorial examples

27298c8

Merge branch 'branch-23.05' into core-type-system

83eb13e

magnatelee added the category:improvement PR introduces an improvement and will be classified as such in release notes label Apr 21, 2023

magnatelee requested review from manopapad, jjwilke and bryevdv April 21, 2023 06:37

magnatelee mentioned this pull request Apr 21, 2023

Using the new core type system nv-legate/cupynumeric#903

Merged

magnatelee added 2 commits April 21, 2023 08:58

Fix for installed header files

87a69b9

IO example doesn't require pyarrow

86c1e9a

bryevdv reviewed Apr 21, 2023

View reviewed changes

magnatelee added 2 commits April 21, 2023 18:11

Merge branch 'branch-23.05' into core-type-system

69bc7fa

Address comments from @bryevdv

0a3105b

magnatelee requested a review from bryevdv April 22, 2023 22:58

Fix for CPU-only build

72a2d38

bryevdv approved these changes Apr 24, 2023

View reviewed changes

jjwilke requested changes Apr 24, 2023

View reviewed changes

magnatelee added 2 commits May 2, 2023 16:13

Merge branch 'branch-23.05' into core-type-system

9d4e7fe

Address comments from @jjwilke

822fb0e

magnatelee requested a review from jjwilke May 3, 2023 20:39

jjwilke approved these changes May 3, 2023

View reviewed changes

magnatelee merged commit 149fa50 into nv-legate:branch-23.05 May 4, 2023

magnatelee deleted the core-type-system branch May 4, 2023 00:12

manopapad reviewed May 4, 2023

View reviewed changes

manopapad mentioned this pull request May 4, 2023

Follow-ups from recent type system changes #713

Merged

madsbk added a commit to madsbk/kvikio that referenced this pull request May 10, 2023

use Type::Code

fe74c4f

refactored in nv-legate/legate#697

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core type system #697

Core type system #697

magnatelee commented Apr 21, 2023

magnatelee commented Apr 21, 2023

bryevdv left a comment

jjwilke left a comment

jjwilke Apr 24, 2023

magnatelee May 3, 2023

jjwilke Apr 24, 2023

magnatelee May 3, 2023

jjwilke Apr 24, 2023

magnatelee May 3, 2023

jjwilke left a comment

manopapad May 4, 2023

manopapad May 4, 2023

manopapad May 4, 2023

manopapad May 4, 2023

manopapad May 4, 2023

Core type system #697

Core type system #697

Conversation

magnatelee commented Apr 21, 2023

magnatelee commented Apr 21, 2023

bryevdv left a comment

Choose a reason for hiding this comment

jjwilke 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

jjwilke 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