Add Apache Arrow stream writers #147

JesseMckinzie · 2023-09-20T13:37:16Z

Update Apache Arrow writers for Arrow IPC and Parquet formats to write in streams to make Arrow writing fully scalable
Update Python API and CLI to use new writers
Add unit tests for updated get_arrow_table method in the Python API

src/nyx/main_nyxus.cpp

sameeul · 2023-09-29T13:46:53Z

src/nyx/output_2_buffer.cpp

@@ -27,6 +27,7 @@

 namespace Nyxus
 {
+


just a white-space change?

This was left behind after removing a function I implemented

sameeul · 2023-09-29T13:55:16Z

src/nyx/environment.h

 	std::string arrow_output_type = "";
+	ArrowOutputStream  arrow_stream;
+	std::shared_ptr<ApacheArrowWriter> arrow_writer;


initialize to nullptr?

sameeul · 2023-09-29T13:59:52Z

src/nyx/output_writers.h

@@ -16,9 +20,20 @@
 #include <memory>

 #include "helpers/helpers.h"
+#include "globals.h"


Not clear why we need this here.

This is no longer needed

sameeul · 2023-09-29T14:20:11Z

src/nyx/scan_fastloader_way.cpp

 // Sanity
 #ifdef _WIN32
 #include<windows.h>
 #endif

+#include <chrono>


Do we need this?

sameeul · 2023-09-29T14:24:23Z

src/nyx/scan_fastloader_way.cpp

+				if (ok == false)
+				{
+					std::cout << "save_features_2_csv() returned an error code" << std::endl;
+					return 2;
+				}


This need to be updated to refer to proper function call. May be something like

if (!ok){ if(save2csv){...} else {...} }

sameeul · 2023-09-29T14:29:07Z

src/nyx/scan_fastloader_way.cpp

+				if (!status.ok()) {
+                    // Handle read error
+                    auto err = status.ToString();
+                    throw std::runtime_error("Error writing Arrow file: " + err);


Should we throw? This can be performance degrading if we are in a situation where we keep throwing. Are we catching this in the caller code? If not, may be just ignore/log and move on?
Since this function already return error codes if anything goes wrong, we should just use the same error reporting mechanism instead of throw.

sameeul · 2023-09-29T14:49:02Z

src/nyx/main_nyxus.cpp

+		use_arrow, // 'true' to save to csv
+		theEnvironment.useCsv,


Update the arg comment :-)

sameeul · 2023-09-29T15:01:29Z

src/nyx/output_writers_old.h

@@ -0,0 +1,166 @@
+


is this a required file?

sameeul · 2023-10-02T13:40:55Z

src/nyx/scan_fastloader_way.cpp

+		#ifdef USE_ARROW
+			if (arrow_output) {
+
+				auto features = Nyxus::get_feature_values();


I guess this is an extra call that we don't need?

friskluft · 2023-10-09T08:26:45Z

src/nyx/arrow_output_stream.cpp

+        throw std::invalid_argument("The arrow file path must end in \".arrow\"");
+    }
+
+    if (!(arrow_file_type_upper == "ARROW" || arrow_file_type_upper == "ARROWIPC" || arrow_file_type_upper == "PARQUET")) {


How a user can specify via arrow_file_type_upper that the desired format is ".feather" ?

friskluft · 2023-10-09T08:33:55Z

src/nyx/globals.h

@@ -32,7 +32,7 @@ namespace Nyxus

 	bool scanFilePairParallel(const std::string& intens_fpath, const std::string& label_fpath, int num_fastloader_threads, int num_sensemaker_threads, int filepair_index, int tot_num_filepairs);
 	std::string getPureFname(const std::string& fpath);
-	int processDataset(const std::vector<std::string>& intensFiles, const std::vector<std::string>& labelFiles, int numFastloaderThreads, int numSensemakerThreads, int numReduceThreads, int min_online_roi_size, bool save2csv, const std::string& csvOutputDir);
+	int processDataset(const std::vector<std::string>& intensFiles, const std::vector<std::string>& labelFiles, int numFastloaderThreads, int numSensemakerThreads, int numReduceThreads, int min_online_roi_size, bool save2csv, bool arrow_output, const std::string& csvOutputDir);


Wouldn't having overloaded function processDataset() for the separate cases of CSV and Apache output types be less confusing than having a single overparametered processDataset() ?

friskluft · 2023-10-09T08:38:07Z

src/nyx/main_nyxus.cpp

+#ifdef USE_ARROW
+	use_arrow = theEnvironment.arrow_output_type == "ARROW" || theEnvironment.arrow_output_type == "PARQUET";
+#endif
+


This piece of logic is OK but is just somewhat not elegant to put such a low-level stuff in main(). Again, if we overload processDataset() as mentioned earlier, we would be able to call it separately with Apache-related and CSV parameters.

friskluft · 2023-10-09T08:42:55Z

src/nyx/scan_fastloader_way.cpp

+				return 2;
+			}
+		}
+	#endif


File scan_fastloader_way.cpp is getting cluttered with #ifdef USE_ARROW code. Which is OK for this PR but I would immediately fix in a subsequent PR to keep the image scan logic free of low-level file format stuff. (Principle of separation of concerns.)

sameeul · 2023-10-10T14:14:25Z

src/nyx/arrow_output_stream.h

+    std::shared_ptr<ApacheArrowWriter> create_arrow_file(const std::string& arrow_file_type,
+                                                         const std::string& arrow_file_path,
+                                                         const std::vector<std::string>& header);
+    std::shared_ptr<arrow::Table> get_arrow_table(const std::string& file_path, arrow::Status& table_status);


This function call should change in a subsequent PR not to pass a ref to retrieve an error/status code. Instead the caller code will check for nullptr on the return value.

JesseMckinzie added 8 commits September 20, 2023 07:25

Add Apache Arrow stream writers

f40c692

Update Apache Arrow writers to use stream writers

63000b8

Add unit tests for get_arrow_table

142c80d

Update name of Gabor parameter

0bb41eb

Update main_nyxus.cpp

a5b3d2b

Update get_arrow_table methods

3d121a1

Remove old arrow writers

1e69ff4

Update new_bindings_py.cpp

1f1ff92

sameeul reviewed Sep 21, 2023

View reviewed changes

src/nyx/main_nyxus.cpp Outdated Show resolved Hide resolved

JesseMckinzie and others added 10 commits September 21, 2023 07:27

Update main_nyxus.cpp

13a397a

Fix build on windows

83efd66

Link arrow to GTests

64870fc

Update get_arrow_file methods

b80dc70

Update arrow writers

8dead40

Update write method for arrow output

5fcbfad

Add arrow writers to gtests

9a6bd2e

Update arrow writers

bbb808a

Update arrow writers

40d972c

Update test_nyxus.py

6c6bd97

sameeul requested a review from friskluft September 29, 2023 13:17

sameeul reviewed Sep 29, 2023

View reviewed changes

src/nyx/output_writers_old.h Outdated

@@ -0,0 +1,166 @@

Copy link

Member

sameeul Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a required file?

JesseMckinzie added 8 commits September 29, 2023 15:00

Initial writer to be null

28e6370

Update output_2_buffer.cpp

870dbfb

Remove old unneeded headers

355df10

Update error handling for arrow output

f882c60

Update main_nyxus.cpp

4f3547b

Update output_writers.cpp

2d17221

Update output_writers.h

012921e

Update arrow error handling for processing montage

16db4c1

Remove previous implementation of Arrow writers

6ae187a

sameeul reviewed Oct 2, 2023

View reviewed changes

JesseMckinzie added 3 commits October 2, 2023 07:42

Remove old method for alling get_feature_values

19434dc

Update Arrow writer error handling

4d100df

Add arrow_table field to ArrowOutputStream

d67b72c

friskluft approved these changes Oct 9, 2023

View reviewed changes

sameeul reviewed Oct 10, 2023

View reviewed changes

sameeul approved these changes Oct 10, 2023

View reviewed changes

sameeul merged commit bfa2543 into PolusAI:main Oct 10, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Apache Arrow stream writers #147

Add Apache Arrow stream writers #147

JesseMckinzie commented Sep 20, 2023

sameeul Sep 29, 2023

JesseMckinzie Sep 29, 2023

sameeul Sep 29, 2023

sameeul Sep 29, 2023

JesseMckinzie Sep 29, 2023

sameeul Sep 29, 2023

sameeul Sep 29, 2023

sameeul Sep 29, 2023

sameeul Sep 29, 2023

sameeul Sep 29, 2023

sameeul Oct 2, 2023

friskluft Oct 9, 2023

friskluft Oct 9, 2023

friskluft Oct 9, 2023

friskluft Oct 9, 2023

sameeul Oct 10, 2023

Add Apache Arrow stream writers #147

Add Apache Arrow stream writers #147

Conversation

JesseMckinzie commented Sep 20, 2023

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