From 7fd444f15390c61f6f59a9fd97ad470bad686df0 Mon Sep 17 00:00:00 2001 From: Kor de Jong Date: Mon, 16 Oct 2023 09:08:34 +0200 Subject: [PATCH 1/2] Refactor --- .../algorithm/create_partitioned_array.hpp | 4 +-- .../test/create_partitioned_array_test.cpp | 5 +-- .../python/source/numpy/from_numpy.cpp | 34 +++---------------- 3 files changed, 10 insertions(+), 33 deletions(-) diff --git a/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp b/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp index 4c50f6685..02118f312 100644 --- a/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp +++ b/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp @@ -750,7 +750,7 @@ namespace lue { auto const& ondp = std::get<0>(policies.outputs_policies()).output_no_data_policy(); - // A single instance of this class are used to instantiate all new partitions. This + // A single instance of this class is used to instantiate all new partitions. This // happens asynchronously. A partition client (a future to the server instance) // is returned immediately, but the server instance itself is not created yet. // To be able to create the server instance later on, when the @@ -765,7 +765,7 @@ namespace lue { array_shape, offset, partition_shape, - buffer_handle = _buffer_handle, + buffer_handle = _buffer_handle, // Copy, increases reference count grab_buffer = _grab_buffer, no_data_value = _no_data_value]() { diff --git a/source/framework/algorithm/test/create_partitioned_array_test.cpp b/source/framework/algorithm/test/create_partitioned_array_test.cpp index 743ed3399..cf039fdcb 100644 --- a/source/framework/algorithm/test/create_partitioned_array_test.cpp +++ b/source/framework/algorithm/test/create_partitioned_array_test.cpp @@ -327,6 +327,7 @@ BOOST_AUTO_TEST_CASE(use_case_2) using BufferHandle = std::shared_ptr; BufferHandle buffer_handle = std::make_shared(nr_values); + BOOST_CHECK_EQUAL(buffer_handle.use_count(), 1); std::iota(buffer_handle->begin(), buffer_handle->end(), 0); // Create a partitioned array, passing in the buffer @@ -352,9 +353,9 @@ BOOST_AUTO_TEST_CASE(use_case_2) auto const& partitions = array.partitions(); lue::wait_all(partitions); - // buffer_handle and the functor containing a copy of buffer_handle + // buffer_handle and the functor contain a copy of buffer_handle // At least once this test failed (use_count was 3). Could be this specific test is wrong. - // BOOST_CHECK_EQUAL(buffer_handle.use_count(), 2); + BOOST_CHECK_EQUAL(buffer_handle.use_count(), 2); auto const [nr_partitions0, nr_partitions1] = array.partitions().shape(); diff --git a/source/framework/python/source/numpy/from_numpy.cpp b/source/framework/python/source/numpy/from_numpy.cpp index f781127d4..3bdce7c4d 100644 --- a/source/framework/python/source/numpy/from_numpy.cpp +++ b/source/framework/python/source/numpy/from_numpy.cpp @@ -18,10 +18,7 @@ namespace lue::framework { using Array = pybind11::array_t; template - using Object = std::tuple, pybind11::buffer_info>; - - template - using ReferenceCountedObject = std::shared_ptr>; + using ReferenceCountedObject = std::shared_ptr>; } // Anonymous namespace @@ -30,18 +27,6 @@ namespace lue::framework { namespace lue::detail { - // template< - // typename T> - // class ArrayTraits< - // pybind11::array_t> - // { - - // public: - - // using Element = T; - - // }; - template class ArrayTraits> { @@ -111,20 +96,10 @@ namespace lue::framework { // created. This is prevented by copying array instances around. This increases // the reference count of the underlying pybind11 object. - // The reference counted object is a shared pointer of: - // - The array passed in - // - The pybind11::buffer_info instance of the array - // This second thing provides us with a pointer to the buffer array elements. Obtaining - // it should only be done once, for all partitions. Therefore we do that here and glue - // it to the array. A shared pointer to this tuple is passed around in the create_array - // function. We don't use the reference counting of the array itself, apart from - // the one copy we make when we create the tuple. - using Policies = lue::policy::create_partitioned_array::DefaultValuePolicies; using Functor = lue::InstantiateFromBuffer, 2>; - ReferenceCountedObject object{ - std::make_shared>(std::make_tuple(array, array.request()))}; + ReferenceCountedObject object{std::make_shared>(array)}; return pybind11::cast(lue::create_partitioned_array( Policies{}, @@ -133,7 +108,7 @@ namespace lue::framework { Functor{ object, // Copy: increments reference count [](ReferenceCountedObject const& object) -> Element* - { return static_cast(std::get<1>(*object).ptr); }, + { return const_cast(object->data()); }, no_data_value})); } @@ -154,7 +129,8 @@ namespace lue::framework { "array"_a.noconvert(), pybind11::kw_only(), "partition_shape"_a = std::optional{}, - "no_data_value"_a = std::optional{}); + "no_data_value"_a = std::optional{}, + pybind11::return_value_policy::move); } }; From 81f7f1a6ca29b0848e487376acf40e2b380f029d Mon Sep 17 00:00:00 2001 From: Kor de Jong Date: Thu, 6 Feb 2025 10:50:22 +0100 Subject: [PATCH 2/2] Fix from_numpy crash --- .../algorithm/create_partitioned_array.hpp | 40 +-- .../python/source/numpy/from_numpy.cpp | 46 +-- .../framework/python/test/from_numpy_test.py | 264 ++++++++---------- source/framework/python/test/to_numpy_test.py | 46 ++- 4 files changed, 190 insertions(+), 206 deletions(-) diff --git a/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp b/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp index 02118f312..c207d5947 100644 --- a/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp +++ b/source/framework/algorithm/include/lue/framework/algorithm/create_partitioned_array.hpp @@ -750,14 +750,14 @@ namespace lue { auto const& ondp = std::get<0>(policies.outputs_policies()).output_no_data_policy(); - // A single instance of this class is used to instantiate all new partitions. This - // happens asynchronously. A partition client (a future to the server instance) - // is returned immediately, but the server instance itself is not created yet. - // To be able to create the server instance later on, when the - // InstantiateFromBuffer instance is already gone, we still need to be able to - // access to the buffer. Because of that, we pass a copy of the BufferHandle - // instance and the GrabBuffer function to the lambda. Copying the BufferHandle + // A single instance of this class is used to instantiate all new partitions. This happens + // asynchronously. A partition client (a future to the server instance) is returned + // immediately, but the server instance itself is not created yet. To be able to create the + // server instance later on, when the InstantiateFromBuffer instance is already gone, we still + // need to be able to access to the buffer. Because of that, we pass a copy of the + // BufferHandle instance and the GrabBuffer function to the lambda. Copying the BufferHandle // instance increases the reference count and keeps the underlying buffer alive. + return hpx::async( [locality_id, @@ -769,8 +769,8 @@ namespace lue { grab_buffer = _grab_buffer, no_data_value = _no_data_value]() { - // Create a partition instance and copy the relevant cells from - // the input buffer to the partition instance + // Create a partition instance and copy the relevant cells from the input buffer to + // the partition instance // A 1D array of elements to copy Element const* buffer = grab_buffer(buffer_handle); @@ -863,10 +863,7 @@ namespace lue { template - concept PartitionInstantiator = requires(Functor functor) - { - functor.instantiate_per_locality; - }; + concept PartitionInstantiator = requires(Functor functor) { functor.instantiate_per_locality; }; /*! @@ -1004,7 +1001,8 @@ namespace lue { // create array, fill with fill value -------------------------------------------------------------------- template - requires std::is_arithmetic_v auto create_partitioned_array( + requires std::is_arithmetic_v + auto create_partitioned_array( Shape const& array_shape, Shape const& partition_shape, Scalar const& fill_value) -> PartitionedArray @@ -1018,7 +1016,8 @@ namespace lue { template - requires std::is_arithmetic_v auto create_partitioned_array( + requires std::is_arithmetic_v + auto create_partitioned_array( Shape const& array_shape, Shape const& partition_shape, Element const fill_value) -> PartitionedArray @@ -1028,17 +1027,18 @@ namespace lue { template - requires std::is_arithmetic_v auto create_partitioned_array( - Shape const& array_shape, - Scalar const& fill_value) -> PartitionedArray + requires std::is_arithmetic_v + auto create_partitioned_array(Shape const& array_shape, Scalar const& fill_value) + -> PartitionedArray { return create_partitioned_array(array_shape, default_partition_shape(array_shape), fill_value); } template - requires std::is_arithmetic_v auto create_partitioned_array( - Shape const& array_shape, Element const fill_value) -> PartitionedArray + requires std::is_arithmetic_v + auto create_partitioned_array(Shape const& array_shape, Element const fill_value) + -> PartitionedArray { return create_partitioned_array( array_shape, default_partition_shape(array_shape), Scalar{fill_value}); diff --git a/source/framework/python/source/numpy/from_numpy.cpp b/source/framework/python/source/numpy/from_numpy.cpp index 3bdce7c4d..b9990b0ef 100644 --- a/source/framework/python/source/numpy/from_numpy.cpp +++ b/source/framework/python/source/numpy/from_numpy.cpp @@ -15,10 +15,7 @@ namespace lue::framework { namespace { template - using Array = pybind11::array_t; - - template - using ReferenceCountedObject = std::shared_ptr>; + using ReferenceCountedObject = std::shared_ptr; } // Anonymous namespace @@ -45,7 +42,7 @@ namespace lue::framework { template auto from_numpy_py( - pybind11::array_t const& array, + pybind11::array_t const& array, std::optional const& partition_shape, std::optional const& no_data_value) -> pybind11::object { @@ -84,31 +81,37 @@ namespace lue::framework { static_partition_shape = default_partition_shape(static_array_shape); } - // The goal is to create a partitioned array, given a Numpy array. We want to do - // this asynchronously. So, even though the function returns an array immediately, - // the partitions might not be ready to use yet. - // This is nice, because other code can already attach continuations to the - // partitions. Or do something else entirely. - // This is a bit dangerous as well: - // - Modifying the Numpy array while partitions are still being created - // results in rubish in the resulting array. Don't do this. - // - The Numpy array must not be deleted while array partitions are still being - // created. This is prevented by copying array instances around. This increases - // the reference count of the underlying pybind11 object. + // The goal is to create a partitioned array, given a Numpy array. We want to do this + // asynchronously. So, even though the function returns an array immediately, the partitions might + // not be ready to use yet. + // + // This is nice, because other code can already attach continuations to the partitions. Or do + // something else entirely. + // + // The problem is that we can't easily use the Python API in threads. This requires obtaining and + // releasing the GIL from/to the thread running the Python interpreter. + // A solution for this is to just copy the data here, and then asynchronously create the + // partitions using Policies = lue::policy::create_partitioned_array::DefaultValuePolicies; using Functor = lue::InstantiateFromBuffer, 2>; - ReferenceCountedObject object{std::make_shared>(array)}; + // The source array buffer + pybind11::buffer_info source_array_buffer_info{array.request()}; + Element const* source_buffer{static_cast(source_array_buffer_info.ptr)}; + + // The target array buffer + auto const nr_elements(lue::nr_elements(static_array_shape)); + ReferenceCountedObject copy_buffer(std::make_shared(nr_elements)); + std::copy(source_buffer, source_buffer + nr_elements, copy_buffer.get()); return pybind11::cast(lue::create_partitioned_array( Policies{}, static_array_shape, static_partition_shape, Functor{ - object, // Copy: increments reference count - [](ReferenceCountedObject const& object) -> Element* - { return const_cast(object->data()); }, + std::move(copy_buffer), + [](ReferenceCountedObject const& buffer) -> Element* { return buffer.get(); }, no_data_value})); } @@ -129,8 +132,7 @@ namespace lue::framework { "array"_a.noconvert(), pybind11::kw_only(), "partition_shape"_a = std::optional{}, - "no_data_value"_a = std::optional{}, - pybind11::return_value_policy::move); + "no_data_value"_a = std::optional{}); } }; diff --git a/source/framework/python/test/from_numpy_test.py b/source/framework/python/test/from_numpy_test.py index e3fcb744d..0c92b1059 100644 --- a/source/framework/python/test/from_numpy_test.py +++ b/source/framework/python/test/from_numpy_test.py @@ -13,143 +13,127 @@ def tearDownModule(): class FromNumPyTest(lue_test.TestCase): - pass - - # TODO gh484 - # @lue_test.framework_test_case - # def test_array(self): - - # array_shape = (60, 40) - # nr_cells = 60 * 40 - # partition_shape = (10, 10) - # dtype = np.int32 - # numpy_array = np.arange(nr_cells, dtype=dtype).reshape(array_shape) - - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - - # self.assertEqual(lue_array.dtype, dtype) - # self.assertEqual(lue_array.shape, array_shape) - # self.assertEqual(lfr.minimum(lue_array).future.get(), 0) - # self.assertEqual(lfr.maximum(lue_array).future.get(), nr_cells - 1) - - # @lue_test.framework_test_case - # def test_mark_no_data(self): - - # array_shape = (60, 40) - # partition_shape = (10, 10) - # dtype = np.int32 - # numpy_array = np.full(array_shape, 5, dtype=dtype) - # numpy_array[10, 10] = 999 - # numpy_array[20, 20] = 999 - # numpy_array[30, 30] = 999 - - # lue_array = lfr.from_numpy( - # numpy_array, partition_shape=partition_shape, no_data_value=999 - # ) - - # self.assertEqual(lfr.minimum(lue_array).future.get(), 5) - # self.assertEqual(lfr.maximum(lue_array).future.get(), 5) - - # # TODO Verify that the array contains three no-data elements - - # @lue_test.framework_test_case - # def test_small_array(self): - - # array_shape = (1, 1) - # partition_shape = (1, 1) - # dtype = np.int32 - # numpy_array = np.arange(1, dtype=dtype).reshape(array_shape) - - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - - # self.assertEqual(lue_array.dtype, dtype) - # self.assertEqual(lue_array.shape, array_shape) - # self.assertEqual(lfr.minimum(lue_array).future.get(), 0) - # self.assertEqual(lfr.maximum(lue_array).future.get(), 0) - - # @lue_test.framework_test_case - # def test_dtype(self): - # """ - # The element type of the numpy array determines the element type of the LUE array. The - # element type of the no-data value must be convertible to the element type of the - # numpy array. - # """ - # array_shape = (60, 40) - # partition_shape = (10, 10) - - # for input_type in [ - # np.uint8, - # np.uint32, - # np.uint64, - # ]: - - # input_dtype = np.dtype(input_type) - # numpy_array = np.full(array_shape, 5, dtype=input_dtype) - - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - # self.assertEqual(lue_array.dtype, input_dtype) - - # lue_array = lfr.from_numpy( - # numpy_array, partition_shape=partition_shape, no_data_value=9 - # ) - # self.assertEqual(lue_array.dtype, input_dtype) - - # self.assertRaises( - # TypeError, - # lfr.from_numpy, - # numpy_array, - # partition_shape=partition_shape, - # no_data_value=-9, - # ) - # self.assertRaises( - # TypeError, - # lfr.from_numpy, - # numpy_array, - # partition_shape=partition_shape, - # no_data_value=9.9, - # ) - - # for input_type in [ - # np.int32, - # np.int64, - # ]: - - # input_dtype = np.dtype(input_type) - # numpy_array = np.full(array_shape, 5, dtype=input_dtype) - - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - # self.assertEqual(lue_array.dtype, input_dtype) - - # lue_array = lfr.from_numpy( - # numpy_array, partition_shape=partition_shape, no_data_value=9 - # ) - # self.assertEqual(lue_array.dtype, input_dtype) - - # lue_array = lfr.from_numpy( - # numpy_array, partition_shape=partition_shape, no_data_value=-9 - # ) - # self.assertEqual(lue_array.dtype, input_dtype) - - # self.assertRaises( - # TypeError, - # lfr.from_numpy, - # numpy_array, - # partition_shape=partition_shape, - # no_data_value=9.9, - # ) - - # for input_type in [ - # np.float32, - # np.float64, - # ]: - - # input_dtype = np.dtype(input_type) - # numpy_array = np.full(array_shape, 5, dtype=input_dtype) - - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - # self.assertEqual(lue_array.dtype, input_dtype) - - # lue_array = lfr.from_numpy( - # numpy_array, partition_shape=partition_shape, no_data_value=9.9 - # ) - # self.assertEqual(lue_array.dtype, input_dtype) + @lue_test.framework_test_case + def test_array(self): + + array_shape = (60, 40) + nr_cells = 60 * 40 + partition_shape = (10, 10) + dtype = np.int32 + numpy_array = np.arange(nr_cells, dtype=dtype).reshape(array_shape) + + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + + self.assertEqual(lue_array.dtype, dtype) + self.assertEqual(lue_array.shape, array_shape) + self.assertEqual(lfr.minimum(lue_array).future.get(), 0) + self.assertEqual(lfr.maximum(lue_array).future.get(), nr_cells - 1) + + @lue_test.framework_test_case + def test_mark_no_data(self): + + array_shape = (60, 40) + partition_shape = (10, 10) + dtype = np.int32 + numpy_array = np.full(array_shape, 5, dtype=dtype) + numpy_array[10, 10] = 999 + numpy_array[20, 20] = 999 + numpy_array[30, 30] = 999 + + lue_array = lfr.from_numpy( + numpy_array, partition_shape=partition_shape, no_data_value=999 + ) + + self.assertEqual(lfr.minimum(lue_array).future.get(), 5) + self.assertEqual(lfr.maximum(lue_array).future.get(), 5) + + # TODO Verify that the array contains three no-data elements + + @lue_test.framework_test_case + def test_small_array(self): + + array_shape = (1, 1) + partition_shape = (1, 1) + dtype = np.int32 + numpy_array = np.arange(1, dtype=dtype).reshape(array_shape) + + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + + self.assertEqual(lue_array.dtype, dtype) + self.assertEqual(lue_array.shape, array_shape) + self.assertEqual(lfr.minimum(lue_array).future.get(), 0) + self.assertEqual(lfr.maximum(lue_array).future.get(), 0) + + @lue_test.framework_test_case + def test_dtype(self): + """ + The element type of the numpy array determines the element type of the LUE array. The + element type of the no-data value must be convertible to the element type of the + numpy array. + """ + array_shape = (60, 40) + partition_shape = (10, 10) + + for input_type in lfr.unsigned_integral_element_types: + input_dtype = np.dtype(input_type) + numpy_array = np.full(array_shape, 5, dtype=input_dtype) + + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + self.assertEqual(lue_array.dtype, input_dtype) + + lue_array = lfr.from_numpy( + numpy_array, partition_shape=partition_shape, no_data_value=9 + ) + self.assertEqual(lue_array.dtype, input_dtype) + + self.assertRaises( + TypeError, + lfr.from_numpy, + numpy_array, + partition_shape=partition_shape, + no_data_value=-9, + ) + self.assertRaises( + TypeError, + lfr.from_numpy, + numpy_array, + partition_shape=partition_shape, + no_data_value=9.9, + ) + + for input_type in lfr.signed_integral_element_types: + input_dtype = np.dtype(input_type) + numpy_array = np.full(array_shape, 5, dtype=input_dtype) + + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + self.assertEqual(lue_array.dtype, input_dtype) + + lue_array = lfr.from_numpy( + numpy_array, partition_shape=partition_shape, no_data_value=9 + ) + self.assertEqual(lue_array.dtype, input_dtype) + + lue_array = lfr.from_numpy( + numpy_array, partition_shape=partition_shape, no_data_value=-9 + ) + self.assertEqual(lue_array.dtype, input_dtype) + + self.assertRaises( + TypeError, + lfr.from_numpy, + numpy_array, + partition_shape=partition_shape, + no_data_value=9.9, + ) + + for input_type in lfr.floating_point_element_types: + input_dtype = np.dtype(input_type) + numpy_array = np.full(array_shape, 5, dtype=input_dtype) + + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + self.assertEqual(lue_array.dtype, input_dtype) + + lue_array = lfr.from_numpy( + numpy_array, partition_shape=partition_shape, no_data_value=9.9 + ) + self.assertEqual(lue_array.dtype, input_dtype) diff --git a/source/framework/python/test/to_numpy_test.py b/source/framework/python/test/to_numpy_test.py index 4e33a70dc..54f97e1f4 100644 --- a/source/framework/python/test/to_numpy_test.py +++ b/source/framework/python/test/to_numpy_test.py @@ -129,17 +129,16 @@ def test_numpy_roundtrip(self): dtype = np.dtype(element_type) numpy_array = np.arange(nr_cells, dtype=dtype).reshape(array_shape) - # TODO gh484 - # lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) - # numpy_array = lfr.to_numpy(lue_array) - - # self.assertEqual(numpy_array.dtype, dtype) - # np.testing.assert_array_equal( - # numpy_array, - # np.arange(nr_cells, dtype=dtype).reshape(array_shape), - # err_msg="Error in case type is {}".format(dtype), - # verbose=True, - # ) + lue_array = lfr.from_numpy(numpy_array, partition_shape=partition_shape) + numpy_array = lfr.to_numpy(lue_array) + + self.assertEqual(numpy_array.dtype, dtype) + np.testing.assert_array_equal( + numpy_array, + np.arange(nr_cells, dtype=dtype).reshape(array_shape), + err_msg="Error in case type is {}".format(dtype), + verbose=True, + ) @lue_test.framework_test_case def test_numpy_roundtrip_result_of_multiple_operations(self): @@ -152,16 +151,15 @@ def test_numpy_roundtrip_result_of_multiple_operations(self): dtype = np.dtype(element_type) numpy_array = np.arange(nr_cells, dtype=dtype).reshape(array_shape) - # TODO gh484 - # lue_array = ( - # lfr.from_numpy(numpy_array, partition_shape=partition_shape) + 5 - # ) - # numpy_array = lfr.to_numpy(lue_array) - - # self.assertEqual(numpy_array.dtype, dtype) - # np.testing.assert_array_equal( - # numpy_array, - # np.arange(nr_cells, dtype=dtype).reshape(array_shape) + 5, - # err_msg="Error in case type is {}".format(dtype), - # verbose=True, - # ) + lue_array = ( + lfr.from_numpy(numpy_array, partition_shape=partition_shape) + 5 + ) + numpy_array = lfr.to_numpy(lue_array) + + self.assertEqual(numpy_array.dtype, dtype) + np.testing.assert_array_equal( + numpy_array, + np.arange(nr_cells, dtype=dtype).reshape(array_shape) + 5, + err_msg="Error in case type is {}".format(dtype), + verbose=True, + )