[REVIEW] Serializer for mdspans #1173

hcho3 · 2023-01-25T00:43:55Z

Closes #1017

Implement a serializer for mdspan using the NumPy format. Row- and column-major layouts are supported.

TODOs

Revise IVF to use the new mdspan serializer
Add gtest to cover serializing device mdspans
Implement serializer for scalars
Rename header to raft/core/serialize.hpp
Use device_resources instead of handle_t
Use 64-byte alignment
Serialize scalars as 0D NumPy array
Add version field to ANN serialization
Test mdspan serializer in the Python layer

codecov-commenter · 2023-01-25T07:12:24Z

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (65fdf53) compared to base (afece4f).
Patch has no changes to coverable lines.

Additional details and impacted files

@@              Coverage Diff              @@
##           branch-23.02    #1173   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

cpp/include/raft/core/mdspan_serializer.hpp

cpp/include/raft/spatial/knn/detail/ivf_flat_build.cuh

hcho3 · 2023-01-26T01:52:32Z

@cjnolet I introduced a few macros to replace global constant variables, as the latter caused a link error due to symbol duplication. Is this okay? Macros are not hygienic and will not be scoped by namespaces. The alternative is to add new source file(s). What's the policy like for adding source files? Is RAFT meant to be mostly header only?

cjnolet · 2023-01-26T02:10:07Z

Macros are not hygienic and will not be scoped by namespaces

I think what you've done should be okay since they are at least prefixed w/ RAFT_NUMPY thus lowering the potential for collision.

What's the policy like for adding source files? Is RAFT meant to be mostly header only?

RAFT's APIs are all header-only and the source files are used for 1) template specializations that users can link against to avoid recompiling expensive headers, and 2) runtime APIs that can be accessed on host (and by pylibraft). This means the serializer code needs to be header-only at its core but if we find that the functions are expensive to compile, we can always add an explicit template specialization for them.

hcho3 · 2023-01-27T18:07:54Z

Can I get another round of review?

achirkin

I'm excited finally to get this in raft! A few minor things in the in the comments.

Also, I'm concerned a little bit regarding the naming. Atm we use serialize_xx, deserialize_xx and also load,save. This looks a bit inconsistent. Would you consider changing all of these to serialize_xx/deserialize_xx? Or even just serialize/deserialize (and rely on overload machinery)?

cpp/include/raft/core/detail/mdspan_numpy_serializer.hpp

cpp/include/raft/core/serialize.hpp

cpp/include/raft/spatial/knn/detail/ivf_pq_build.cuh

cpp/include/raft/core/mdspan.hpp

…izer

hcho3 · 2023-01-31T05:49:25Z

Would you consider changing all of these to serialize_xx/deserialize_xx? Or even just serialize/deserialize (and rely on overload machinery)?

I'm in favor of serialize_xx/deserialize_xx, because using the same serialize name for both scalars and mdspans may confuse the compiler and cause a surprising behavior in overload resolution. That is, when we declare

template <typename T>
void serialize(std::ostream& os, const T& value)

to serialize scalars, the function may be accidentally called with an mdspan (by setting T=mdspan<...>).

I have renamed all functions to serialize_xxx / deserialize_xxx.

…izer

achirkin

LGTM!

cjnolet

I’m pre-approving so we don’t need to hold up the release for these but just a couple small things we should probably open a follow-on for (in 23.04)

cjnolet · 2023-01-27T16:52:41Z

python/pylibraft/pylibraft/test/test_mdspan_serializer.py

+@pytest.mark.parametrize("dtype", ["float32", "float64", "int32", "uint32"])
+def test_mdspan_serializer(dtype):
+    X = np.random.random_sample((2, 3)).astype(dtype)
+    run_roundtrip_test_for_mdspan(X)


Can we also test this for fortran_order=True?

cjnolet · 2023-01-27T16:55:59Z

python/pylibraft/pylibraft/common/mdspan.pyx

+    Py_buffer* PyMemoryView_GET_BUFFER(PyObject* mview)
+
+
+def run_roundtrip_test_for_mdspan(X, fortran_order=False):


Could we move this to the testing namespace? maybe something like test/serializer_helper.pyx?

divyegala

Pre-approving, the PR looks really great! Just one comment, could you please file an issue or add a TODO for my comment to be tackled later?

divyegala · 2023-02-01T20:35:16Z

cpp/include/raft/core/serialize.hpp

+
+template <typename ElementType, typename Extents, typename LayoutPolicy, typename AccessorPolicy>
+inline void serialize_mdspan(
+  const raft::device_resources&,


Can we use the base-class raft::resources for all these functions? device_resources is not needed with host_mdspan overloads

cjnolet · 2023-02-02T11:30:41Z

/merge

Closes rapidsai#1017 Implement a serializer for mdspan using the NumPy format. Row- and column-major layouts are supported. TODOs - [x] Revise IVF to use the new mdspan serializer - [x] Add gtest to cover serializing device mdspans - [x] Implement serializer for scalars - [x] Rename header to `raft/core/serialize.hpp` - [x] Use `device_resources` instead of `handle_t` - [x] Use 64-byte alignment - [x] Serialize scalars as 0D NumPy array - [x] Add version field to ANN serialization - [x] Test mdspan serializer in the Python layer Authors: - Philip Hyunsu Cho (https://github.com/hcho3) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - Artem M. Chirkin (https://github.com/achirkin) - Corey J. Nolet (https://github.com/cjnolet) - Divye Gala (https://github.com/divyegala) - Ray Douglass (https://github.com/raydouglass) URL: rapidsai#1173

hcho3 added 2 commits January 24, 2023 14:04

Implement mdspan serializer

7b6e4ba

Implement deserializer

8f012d5

hcho3 requested review from a team as code owners January 25, 2023 00:43

github-actions bot added CMake cpp labels Jan 25, 2023

cjnolet assigned hcho3 Jan 25, 2023

cjnolet requested review from tfeher, achirkin, cjnolet and divyegala January 25, 2023 20:07

cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 25, 2023

cjnolet and others added 5 commits January 25, 2023 15:07

Merge branch 'branch-23.02' into mdspan_serializer

4e00e4a

Update ANN to use the new mdspan serializer

9d4b332

Fix copyright year

71d1b0a

Solve link error due to duplicated symbols

7391313

Add gtest to test serializing device_mdspan

03efe03

hcho3 changed the title ~~[WIP] Serializer for mdspans~~ [REVIEW] Serializer for mdspans Jan 26, 2023

cjnolet reviewed Jan 26, 2023

View reviewed changes

cpp/include/raft/core/mdspan_serializer.hpp Outdated Show resolved Hide resolved

cjnolet reviewed Jan 26, 2023

View reviewed changes

cpp/include/raft/spatial/knn/detail/ivf_flat_build.cuh Outdated Show resolved Hide resolved

Set up Pytest for mdspan serialization

65fdf53

hcho3 requested a review from a team as a code owner January 27, 2023 06:41

github-actions bot added the python label Jan 27, 2023

achirkin requested changes Jan 28, 2023

View reviewed changes

cjnolet and others added 2 commits January 28, 2023 14:46

Merge branch 'branch-23.02' into mdspan_serializer

30b4238

Merge remote-tracking branch 'origin/branch-23.02' into mdspan_serial…

3f7481d

…izer

hcho3 added 4 commits January 30, 2023 22:03

Rename load/save to (de)serialize

30733f3

Don't export dextents from raft

4c025de

Make serialization_version a constexpr

0196f00

Implement serializer for managed_mdspan

1cd63a8

achirkin mentioned this pull request Jan 31, 2023

Serialization for commonly used types #770

Closed

hcho3 added 5 commits January 31, 2023 10:04

Fix build error

a91c64a

Merge remote-tracking branch 'origin/branch-23.02' into mdspan_serial…

42eccbc

…izer

Revert spurious copyright year updates

cf83274

Revert spurious copyright year updates

3cf5a79

Add static_assert about size

3bd6f13

achirkin approved these changes Feb 1, 2023

View reviewed changes

Merge branch 'branch-23.02' into mdspan_serializer

dca29a9

cjnolet approved these changes Feb 1, 2023

View reviewed changes

divyegala approved these changes Feb 1, 2023

View reviewed changes

Setting parallel_level-8

9bb0043

cjnolet requested a review from a team as a code owner February 1, 2023 21:43

raydouglass approved these changes Feb 2, 2023

View reviewed changes

rapids-bot bot merged commit aa14ed4 into rapidsai:branch-23.02 Feb 2, 2023

hcho3 deleted the mdspan_serializer branch February 2, 2023 17:03

hcho3 mentioned this pull request Feb 3, 2023

[FEA] Follow-up task for mdspan serializer #1233

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Serializer for mdspans #1173

[REVIEW] Serializer for mdspans #1173

hcho3 commented Jan 25, 2023 •

edited

Loading

codecov-commenter commented Jan 25, 2023 •

edited

Loading

hcho3 commented Jan 26, 2023

cjnolet commented Jan 26, 2023 •

edited

Loading

hcho3 commented Jan 27, 2023

achirkin left a comment

hcho3 commented Jan 31, 2023 •

edited

Loading

achirkin left a comment

cjnolet left a comment

cjnolet Jan 27, 2023

cjnolet Jan 27, 2023

divyegala left a comment

divyegala Feb 1, 2023

cjnolet commented Feb 2, 2023

		Py_buffer* PyMemoryView_GET_BUFFER(PyObject* mview)


		def run_roundtrip_test_for_mdspan(X, fortran_order=False):

[REVIEW] Serializer for mdspans #1173

[REVIEW] Serializer for mdspans #1173

Conversation

hcho3 commented Jan 25, 2023 • edited Loading

codecov-commenter commented Jan 25, 2023 • edited Loading

Codecov Report

hcho3 commented Jan 26, 2023

cjnolet commented Jan 26, 2023 • edited Loading

hcho3 commented Jan 27, 2023

achirkin left a comment

Choose a reason for hiding this comment

hcho3 commented Jan 31, 2023 • edited Loading

achirkin left a comment

Choose a reason for hiding this comment

cjnolet left a comment

Choose a reason for hiding this comment

cjnolet Jan 27, 2023

Choose a reason for hiding this comment

cjnolet Jan 27, 2023

Choose a reason for hiding this comment

divyegala left a comment

Choose a reason for hiding this comment

divyegala Feb 1, 2023

Choose a reason for hiding this comment

cjnolet commented Feb 2, 2023

hcho3 commented Jan 25, 2023 •

edited

Loading

codecov-commenter commented Jan 25, 2023 •

edited

Loading

cjnolet commented Jan 26, 2023 •

edited

Loading

hcho3 commented Jan 31, 2023 •

edited

Loading