Skip to content

fix(arrow): raise when the Arrow or Parquet output file cannot be opened - #535

Open
darkclad wants to merge 2 commits into
PolusAI:mainfrom
darkclad:main-arrow-open-error
Open

darkclad wants to merge 2 commits into
PolusAI:mainfrom
darkclad:main-arrow-open-error

Conversation

@darkclad

Copy link
Copy Markdown

Summary

When nyxus cannot open its Arrow IPC output file for writing, featurize(..., output_type="arrowipc") aborts the Python interpreter instead of raising:

arrow/result.cc:27: ValueOrDie called on an error: Unknown error: Uninitialized Result<T>
Fatal Python error: Aborted

ArrowIPCWriter's constructor dropped the Status returned by setup(). After a failed FileOutputStream::Open, writer_ stayed unassigned, and the first write() dereferenced it. ParquetWriter dropped the FileWriter::Open status the same way.

The open can fail when the target is a directory, the location is read-only, or Windows still has the file mapped for a live pyarrow table. On PR #529's Windows wheel jobs, the last case aborted the test run and cut off pytest's failure summary.

Changes

  • ParquetWriter and ArrowIPCWriter constructors throw std::runtime_error when setup() fails. The message names the output path and includes Arrow's error.
  • WriterFactory::create_writer catches that and returns the message through its existing error slot. Every caller of create_arrow_file already turns that error into a failed run, so:
    • featurize() raises RuntimeError;
    • the CLI exits non-zero instead of terminating.
  • ParquetWriter::setup opens the file through ARROW_ASSIGN_OR_RAISE instead of PARQUET_ASSIGN_OR_THROW, so both writers report a failed open the same way.
  • ArrowIPCWriter::setup now returns MakeFileWriter's status.

Example of the new error:

RuntimeError: Error occurred during dataset processing: error creating Arrow file: cannot set up the Arrow IPC writer for <dir>/NyxusFeatures.arrow: IOError: Failed to open local file '<dir>/NyxusFeatures.arrow'. Detail: [Windows error 5] Access is denied.

Tests

test checks without the fix
TEST_ARROW_IPC_UNOPENABLE_PATH_MECHANICS create_arrow_file on a path that is a directory returns failure naming the path; the same stream then writes and reads back a file at a writable path fails: creation reports success
TEST_ARROW_PARQUET_UNOPENABLE_PATH_MECHANICS same, for Parquet fails: the exception escapes create_arrow_file
test_arrow_unopenable_path_raises[arrowipc] featurize() raises RuntimeError naming the file, then writes to a writable path aborts the interpreter
test_arrow_unopenable_path_raises[parquet] same, for Parquet passes (pybind already converted the old Parquet exception)
test_arrow_ipc_mapped_file_raises (Windows only) the reported case: a live pyarrow table keeps the file mapped, featurize() raises, and the write succeeds once the table is released aborts the interpreter

A directory at the target path is the unopenable fixture because no platform opens a directory for writing.

Closes PN-279.

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.
@darkclad
darkclad force-pushed the main-arrow-open-error branch from a768584 to 20810ff Compare November 3, 2026 20:41

@vjaganat90 vjaganat90 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Nit, in a file this PR doesn't touch: tests/vetting/not_covered.md still counts the Arrow tests as they were before this PR.

  • Line 33: test_arrow_mechanics.h now has 4 test functions, not 2, so the section A total on line 20 becomes 107.
  • B.3: the USE_ARROW row should list TEST_ARROW_IPC_UNOPENABLE_PATH_MECHANICS and TEST_ARROW_PARQUET_UNOPENABLE_PATH_MECHANICS, which makes 15 cases.
  • B.4: could add that test_arrow_ipc_mapped_file_raises runs on Windows only.

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.
@darkclad

darkclad commented Nov 6, 2026

Copy link
Copy Markdown
Author

Diverted Updated the Arrow row, the USE_ARROW cases and the Windows-only note as suggested, but the totals are 113 and 73 rather than 107 and 15, because both headings were already stale before this PR. The section A tables summed to 111, and test_all.cc registers 73 flag-gated cases, so the OMEZARR_SUPPORT row now describes its 61 cases by name pattern. Done in 7c3a149.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants