From 20810ff9908b9b997cd037718d0f7d688657169a Mon Sep 17 00:00:00 2001 From: Demian Vladi Date: Thu, 29 Oct 2026 09:30:57 -0700 Subject: [PATCH 1/2] fix(arrow): raise when the Arrow or Parquet output file cannot be opened ArrowIPCWriter's constructor dropped the Status returned by setup(), so a failed FileOutputStream::Open (a directory at the target path, a read-only location, a file Windows keeps mapped for a live pyarrow table) left writer_ unassigned. The first write then called ValueOrDie on it and aborted the Python interpreter. ParquetWriter dropped the FileWriter::Open status the same way. Both constructors now throw when setup() fails, naming the output path and Arrow's message, and WriterFactory::create_writer returns that message as its error. Every caller of create_arrow_file already turns that error into a failed run, so featurize() raises RuntimeError and the CLI exits non-zero. ParquetWriter's open goes through the same Status path instead of PARQUET_ASSIGN_OR_THROW, and the IPC writer also checks MakeFileWriter's result. Tests: - TEST_ARROW_IPC_UNOPENABLE_PATH_MECHANICS and TEST_ARROW_PARQUET_UNOPENABLE_PATH_MECHANICS: create_arrow_file on a path that is a directory reports failure naming the path, and the same stream then writes a readable file to a writable path. Both fail without the fix (IPC returns success; Parquet lets the exception escape create_arrow_file). - test_arrow_unopenable_path_raises (arrowipc, parquet) and the Windows-only test_arrow_ipc_mapped_file_raises: featurize() raises RuntimeError, then writes the file once the path is usable. The arrowipc cases abort the interpreter without the fix. Closes PN-279. --- src/nyx/output_writers.cpp | 60 +++++++++++++++++++++-------------- tests/python/test_nyxus.py | 61 ++++++++++++++++++++++++++++++++++-- tests/test_all.cc | 8 +++++ tests/test_arrow_mechanics.h | 51 ++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 25 deletions(-) diff --git a/src/nyx/output_writers.cpp b/src/nyx/output_writers.cpp index 8052eef02..3b6ac0c75 100644 --- a/src/nyx/output_writers.cpp +++ b/src/nyx/output_writers.cpp @@ -7,6 +7,7 @@ #ifdef USE_ARROW #include +#include #include #include "helpers/fsystem.h" #include "helpers/helpers.h" @@ -32,7 +33,7 @@ arrow::Status ParquetWriter::setup(const std::vector& header) { schema_ = arrow::schema(fields); - PARQUET_ASSIGN_OR_THROW( + ARROW_ASSIGN_OR_RAISE( output_stream_, arrow::io::FileOutputStream::Open(output_file_) ); @@ -54,14 +55,15 @@ arrow::Status ParquetWriter::setup(const std::vector& header) { +// A writer whose setup failed has no usable output stream or file writer, so the +// constructor throws rather than return a half-built object; WriterFactory::create_writer +// turns the exception into its error message. ParquetWriter::ParquetWriter(const std::string& output_file, const std::vector& header) : output_file_(output_file) { auto status = this->setup(header); - if (!status.ok()) { - // Handle read error - std::cout << "Error writing setting up Arrow writer: " << status.ToString() << std::endl; - } + if (!status.ok()) + throw std::runtime_error("cannot set up the Parquet writer for " + output_file_ + ": " + status.ToString()); } arrow::Status ParquetWriter::write (const std::vector& features) { @@ -302,15 +304,19 @@ arrow::Status ArrowIPCWriter::setup(const std::vector& header) { writer_ = arrow::ipc::MakeFileWriter(output_stream_, schema_); - return arrow::Status::OK(); + return writer_.status(); } +// Throws on a failed setup for the same reason as ParquetWriter's constructor: write() and +// close() dereference writer_, and an unassigned arrow::Result aborts the process there. ArrowIPCWriter::ArrowIPCWriter(const std::string& output_file, const std::vector& header) : output_file_(output_file) { auto status = this->setup(header); + if (!status.ok()) + throw std::runtime_error("cannot set up the Arrow IPC writer for " + output_file_ + ": " + status.ToString()); } arrow::Status ArrowIPCWriter::write (const std::vector& features) @@ -531,32 +537,40 @@ arrow::Status ArrowIPCWriter::close() { std::tuple, std::optional> WriterFactory::create_writer(const std::string& output_file, const std::vector& header) { - if (Nyxus::ends_with_substr(output_file, ".parquet")) { + // the writers' constructors throw when the file cannot be opened for writing + // (a directory, a read-only location, a file another process holds mapped) + try { + + if (Nyxus::ends_with_substr(output_file, ".parquet")) { + + return { std::make_unique(output_file, header), std::nullopt }; + + } + else if (Nyxus::ends_with_substr(output_file, ".arrow") || Nyxus::ends_with_substr(output_file, ".feather")) { - return { std::make_unique(output_file, header), std::nullopt }; + return { std::make_unique(output_file, header), std::nullopt }; + } } - else if (Nyxus::ends_with_substr(output_file, ".arrow") || Nyxus::ends_with_substr(output_file, ".feather")) { + catch (const std::exception& e) { - return { std::make_unique(output_file, header), std::nullopt }; + return { nullptr, std::string{ e.what() } }; } - else { - fs::path path(output_file); + fs::path path(output_file); - auto error_msg = [&path]() { - if (path.has_extension()) - { - return "No writer option for extension \"" + path.extension().string() + "\". Valid options are \".parquet\" or \".arrow\"."; - } - else { - return std::string{ "No extension type was provided in the path." }; - } - }; + auto error_msg = [&path]() { + if (path.has_extension()) + { + return "No writer option for extension \"" + path.extension().string() + "\". Valid options are \".parquet\" or \".arrow\"."; + } + else { + return std::string{ "No extension type was provided in the path." }; + } + }; - return { nullptr, error_msg() }; - } + return { nullptr, error_msg() }; } #endif diff --git a/tests/python/test_nyxus.py b/tests/python/test_nyxus.py index 51a1874b9..7fe2e7ef4 100644 --- a/tests/python/test_nyxus.py +++ b/tests/python/test_nyxus.py @@ -723,9 +723,66 @@ def test_arrow_ipc_path(self): arrow_path = nyx.featurize(intens, seg, output_type="arrowipc") - assert arrow_path == 'NyxusFeatures.arrow' + assert arrow_path == 'NyxusFeatures.arrow' + + @pytest.mark.arrow + @pytest.mark.parametrize("output_type, ext", [("arrowipc", ".arrow"), ("parquet", ".parquet")]) + def test_arrow_unopenable_path_raises(self, tmp_path, output_type, ext): + ''' + An output file that cannot be opened for writing raises RuntimeError naming the + file, rather than aborting the interpreter. The target here is a directory, which + no platform opens for writing. The same Nyxus object then writes to a writable path. + ''' + + nyx = nyxus.Nyxus (["MEAN"]) + + # a directory output_path resolves to /NyxusFeatures + blocked = tmp_path/("NyxusFeatures" + ext) + blocked.mkdir() + + with pytest.raises(RuntimeError, match="NyxusFeatures" + ext.replace(".", r"\.")): + nyx.featurize(intens, seg, output_type=output_type, output_path=str(tmp_path)) + + writable = tmp_path/("writable" + ext) + out_path = nyx.featurize(intens, seg, output_type=output_type, output_path=str(writable)) + assert out_path == str(writable) + assert writable.is_file() + + if output_type == "parquet": + tbl = pq.read_table(out_path) + else: + with pa.memory_map(out_path, 'rb') as source: + tbl = pa.ipc.open_file(source).read_all() + assert tbl.num_rows == len(nyx.featurize(intens, seg)) + + @pytest.mark.arrow + @pytest.mark.skipif(sys.platform != "win32", reason="only Windows refuses to truncate a memory-mapped file") + def test_arrow_ipc_mapped_file_raises(self, tmp_path): + ''' + Featurizing to an Arrow IPC file that a live pyarrow table still maps fails to open + on Windows; that failure raises RuntimeError. Once the table is released, the same + path is written again. + ''' + + nyx = nyxus.Nyxus (["MEAN"]) + out_file = tmp_path/"mapped.arrow" + + arrow_path = nyx.featurize(intens, seg, output_type="arrowipc", output_path=str(out_file)) + source = pa.memory_map(arrow_path, 'rb') + tbl = pa.ipc.open_file(source).read_all() + source.close() + + with pytest.raises(RuntimeError, match=r"mapped\.arrow"): + nyx.featurize(intens, seg, output_type="arrowipc", output_path=str(out_file)) + + n_rows = tbl.num_rows + del tbl + + assert nyx.featurize(intens, seg, output_type="arrowipc", output_path=str(out_file)) == str(out_file) + with pa.memory_map(arrow_path, 'rb') as source: + assert pa.ipc.open_file(source).read_all().num_rows == n_rows + - def test_image_quality(self): ''' Test that ImageQuality class can be imported and will calculated features. diff --git a/tests/test_all.cc b/tests/test_all.cc index 45478c8b6..65b45dbe4 100644 --- a/tests/test_all.cc +++ b/tests/test_all.cc @@ -884,6 +884,14 @@ TEST(TEST_NYXUS, TEST_ARROW_PARQUET_MECHANICS) { test_arrow_parquet_mechanics(); } +TEST(TEST_NYXUS, TEST_ARROW_IPC_UNOPENABLE_PATH_MECHANICS) { + test_arrow_ipc_unopenable_path_mechanics(); +} + +TEST(TEST_NYXUS, TEST_ARROW_PARQUET_UNOPENABLE_PATH_MECHANICS) { + test_arrow_parquet_unopenable_path_mechanics(); +} + #endif //***** 3D shape ***** diff --git a/tests/test_arrow_mechanics.h b/tests/test_arrow_mechanics.h index 4cba04243..3b415c47f 100644 --- a/tests/test_arrow_mechanics.h +++ b/tests/test_arrow_mechanics.h @@ -320,3 +320,54 @@ void test_arrow_parquet_mechanics() { } } +// A target that exists as a directory cannot be opened for writing on any platform, so +// create_arrow_file must report the failure (naming the path) rather than hand back a writer +// whose first write aborts the process. The same stream then writes a file next to it, so +// the refusal is specific to the unopenable path. +void assert_arrow_unopenable_path_refused(Nyxus::SaveOption saveOption, const std::string& ext) +{ + auto temp = fs::temp_directory_path() / ("nyxus_unopenable" + ext); + fs::remove_all(temp); + fs::create_directories(temp); + + fs::path blocked = temp / ("NyxusFeatures" + ext); + fs::create_directory(blocked); + + auto arrow_stream = ArrowOutputStream(); + + auto [status, msg] = arrow_stream.create_arrow_file(saveOption, blocked.string(), std::get<0>(features888)); + ASSERT_FALSE(status) << "a writer was created for directory " << blocked.string(); + ASSERT_TRUE(msg.has_value()); + ASSERT_NE(msg->find(blocked.string()), std::string::npos) << "message does not name the path: " << *msg; + + auto [wstatus, wmsg] = arrow_stream.write_arrow_file(std::get<1>(features888)); + ASSERT_FALSE(wstatus); + + std::string writable = (temp / ("writable" + ext)).string(); + + auto [status1, msg1] = arrow_stream.create_arrow_file(saveOption, writable, std::get<0>(features888)); + ASSERT_TRUE(status1) << msg1.value_or(""); + + auto [status2, msg2] = arrow_stream.write_arrow_file(std::get<1>(features888)); + ASSERT_TRUE(status2) << msg2.value_or(""); + + auto [status3, msg3] = arrow_stream.close_arrow_file(); + ASSERT_TRUE(status3) << msg3.value_or(""); + + auto table = get_arrow_table(writable); + ASSERT_TRUE(table.ok()) << table.status().ToString(); + ASSERT_EQ((*table)->num_rows(), (int64_t) std::get<1>(features888).size()); + + fs::remove_all(temp); +} + +void test_arrow_ipc_unopenable_path_mechanics() +{ + assert_arrow_unopenable_path_refused(Nyxus::SaveOption::saveArrowIPC, ".arrow"); +} + +void test_arrow_parquet_unopenable_path_mechanics() +{ + assert_arrow_unopenable_path_refused(Nyxus::SaveOption::saveParquet, ".parquet"); +} + From 7c3a1496b5309727c144ea9779bc57d9d937cd56 Mon Sep 17 00:00:00 2001 From: Demian Vladi Date: Fri, 6 Nov 2026 10:37:00 -0800 Subject: [PATCH 2/2] docs(vetting): count the Arrow open-failure tests in not_covered.md test_arrow_mechanics.h has 4 test functions. Section A's total is the sum of its tables, 113 functions. B.3 lists the two new USE_ARROW cases and totals what test_all.cc registers under a build flag: 5 USE_ARROW, 61 OMEZARR_SUPPORT and 7 DICOM_SUPPORT, 73 cases. The OMEZARR_SUPPORT row describes its cases by name pattern. B.4 records that test_arrow_ipc_mapped_file_raises runs on Windows only. --- tests/vetting/not_covered.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/vetting/not_covered.md b/tests/vetting/not_covered.md index 4f9a15824..bbd3cfd68 100644 --- a/tests/vetting/not_covered.md +++ b/tests/vetting/not_covered.md @@ -17,7 +17,7 @@ Regenerate by re-deriving both sets; keep this file in step with each reorg wave --- -## A. Test files no registry row references — 31 files, 105 test functions +## A. Test files no registry row references — 31 files, 113 test functions Both totals are the sum of the two tables below, counting each distinct file once and skipping any row struck through as closed, so they can be recomputed from the tables. @@ -30,7 +30,7 @@ These assert no feature value, so they have no `(feature × config × oracle)` r | file | functions | why it has no row | |---|---:|---| | `test_3d_nifti_mechanics.h` | 2 | NIfTI loader geometry / data access | -| `test_arrow_mechanics.h` | 2 | Arrow + Parquet writer plumbing | +| `test_arrow_mechanics.h` | 4 | Arrow + Parquet writer plumbing, including an output path that cannot be opened | | `test_arrow_file_name_mechanics.h` | 1 | output-file naming rules | | `test_2d_glcm_mechanics.h` | 1 | guards the `GLCM_OFFSET` default (a setting, not a value) | | `test_2d_radial_mechanics.h` | 3 | contour frame, centre pixel and normalising radius — the wiring the radial goldens depend on. **Uncredited by decision, not by construction:** see the note below the table | @@ -202,15 +202,15 @@ Not dead, listed to avoid double-counting: `assert_gldzm_matrix_ibsi`, `assert_ngldm_matrix_ibsi_mode`, `assert_ngldm_matrix_nonibsi_mode` take no arguments and have no `TEST()` of their own, but each is called by a registered case — they are helpers wearing a `test_` prefix. -### B.3 Registered but excluded by a build flag — 13 cases +### B.3 Registered but excluded by a build flag — 73 cases Not dead; they run only where the feature is compiled in. A CPU/tiff-only build silently omits them, so a green local run is not a green matrix. | flag | cases | |---|---| -| `USE_ARROW` | `TEST_ARROW_IPC_MECHANICS`, `TEST_ARROW_PARQUET_MECHANICS`, `TEST_ARROW_FILE_NAMING_MECHANICS` | -| `OMEZARR_SUPPORT` | `TEST_2D_OMEZARR_TILELOADER_{GEOMETRY,CONTENT,MULTITILE,SIGNED_OFFSET_MAP,FLOAT_QUANTIZED_MAP,UNSIGNED_UNAFFECTED}_MECHANICS`, `TEST_2D_OMEZARR_RAW_{GEOMETRY,CONTENT,MULTITILE,SIGNED_SOURCE_DOMAIN,FLOAT_SOURCE_DOMAIN,UINT32_ACCESSOR_CLAMPS_NEGATIVE,UINT32_ACCESSOR_UNSIGNED_EXACT,UINT32_ACCESSOR_NONFINITE}_MECHANICS` | +| `USE_ARROW` | `TEST_ARROW_IPC_MECHANICS`, `TEST_ARROW_PARQUET_MECHANICS`, `TEST_ARROW_FILE_NAMING_MECHANICS`, `TEST_ARROW_IPC_UNOPENABLE_PATH_MECHANICS`, `TEST_ARROW_PARQUET_UNOPENABLE_PATH_MECHANICS` | +| `OMEZARR_SUPPORT` | the 61 `TEST_*OMEZARR*_MECHANICS` cases in `test_all.cc`; `TEST_OMEZARR_V3_ZSTD_MECHANICS` also needs `WITH_ZSTD` | | `DICOM_SUPPORT` | `TEST_2D_HU_LOADER_DICOM_{U16,I16}_PRESERVE_MECHANICS`, `TEST_2D_HU_LOADER_DICOM_CT_SMALL_{PRESERVE,BASELINE}_MECHANICS`, `TEST_2D_HU_LOADER_DICOM_FRACTIONAL_SLOPE_{ROUNDS_UNDER_PRESERVE_HU,TRUNCATES_WITHOUT_ROUND_OFFSET}_MECHANICS`, `TEST_2D_HU_DICOM_FRACTIONAL_SLOPE_LOAD_PATH_MECHANICS` | ### B.4 Python tests skipped at runtime @@ -220,7 +220,8 @@ so a green local run is not a green matrix. - `test_2d_morphology_invariant.py`, `test_2d_glcm_pyradiomics.py`, `test_2d_gldm_pyradiomics.py` — skip when the canonical ROI cannot be parsed out of `test_data.h`. **A skip here silently removes an oracle assertion**; these should fail rather than skip, since the fixture is committed. -- `test_nyxus.py` — one case is skipped on Python 3.12, one is `skip_ci`. +- `test_nyxus.py` — one case is skipped on Python 3.12, one is `skip_ci`, and `test_arrow_ipc_mapped_file_raises` + runs on Windows only, the one platform that refuses to truncate a memory-mapped file. ---