Skip to content

fix(reader): ArrowReader silently returns an empty dataset for a .parquetbundle path - #431

Open
jcoludar wants to merge 1 commit into
mainfrom
fix/arrowreader-silent-empty
Open

fix(reader): ArrowReader silently returns an empty dataset for a .parquetbundle path#431
jcoludar wants to merge 1 commit into
mainfrom
fix/arrowreader-silent-empty

Conversation

@jcoludar

Copy link
Copy Markdown
Collaborator

What happened

We handed ArrowReader a .parquetbundle — the documented distribution format — to inspect a dataset. It returned successfully:

reader = ArrowReader(Path("phosphatase.parquetbundle"))
reader.get_protein_ids()        # []
reader.get_projection_names()   # []
reader.get_format_version()     # 1

No exception, no warning, not even a DEBUG log. We only noticed because the file was 189 KB and "0 proteins" did not square with that.

Why

ArrowReader expects a directory of loose .parquet files. Every probe in _load_data is an .exists() test with an empty-frame fallback:

protein_annotations_path = self.data_path / "selected_annotations.parquet"
if protein_annotations_path.exists():
    ...
else:
    self._protein_annotations_df = pd.DataFrame(columns=["protein_id"])

For a file path, data.parquetbundle/selected_annotations.parquet never exists, so all three frames fall back to empty and _build_data_structure iterates zero rows. format_version is never assigned, so get_format_version() returns its default of 1.

The try/except around the block only fires on a read error. A path that does not exist is never read, so it never triggers.

The same silence covers two more inputs:

input proteins projections format_version raised
.parquetbundle file 0 [] 1 nothing
directory that does not exist 0 [] 1 nothing
directory with no ProtSpace tables 0 [] 1 nothing
extracted directory (control) 3 ['UMAP_2'] 1

What this changes

Adds _validate_data_path(), called as the first statement of _load_databefore the try, so its ValueError is not re-wrapped by the handler into a misleading "Error loading Arrow data" message.

  • a .parquetbundle file → ValueError naming extract_bundle_to_dir(), which is what every existing caller already does
  • any other file → ValueError
  • a missing directory → FileNotFoundError
  • a directory with none of the three core tables → ValueError

The check is "at least one core file", not "all three". Directories carrying only selected_annotations.parquet are legitimate, and test_bundle_version.py builds exactly those — requiring all three would break it.

Why raise rather than auto-extract

Both existing resolvers already convert bundle → directory before constructing the reader (main.detect_data_type, and detect_data_format in add_annotation_style.py, which returns "parquet" only for path.is_dir()), and both raise on bad input. Extracting inside __init__ would also mint an uncleaned mkdtemp as a hidden side effect of construction. The directory-only contract looks deliberate, so this makes it explicit instead of changing it.

Blast radius

Checked every caller. All CLI/dash paths (app.py, add_annotation_style.py) extract bundles first; the rest construct from a dict and never reach this code. No caller relies on the empty fallback.

One behaviour change worth flagging: main.detect_data_type accepts any directory containing any *.parquet, so a directory holding only e.g. statistics.parquet previously produced a silent empty viewer and will now raise. That seems like the right outcome, but it is a change.

Verification

  • Full suite: 806 passed, 2 skipped.
  • test_bundle_version.py, test_settings_converter.py, test_display_decode.py, test_bundle_settings.py: 61 passed.
  • Targeted acceptance test: bundle path → ValueError; missing directory → FileNotFoundError; directory with only selected_annotations.parquet → still loads (2 proteins).

Related

While tracing this we noticed the same swallow-and-continue shape twice more in this file: _build_data_structure does except json.JSONDecodeError: pass on a corrupt info_json (projection parameters vanish silently), and _load_visualization_state logs a settings-load failure at DEBUG only, invisible at default verbosity. test_settings_converter.py records that the latter has already caused a real regression once — "ArrowReader swallowed, so every legend colour/shape vanished silently". Left alone here to keep this PR to one change; happy to follow up.

Every probe in _load_data is an .exists() test with an empty-frame
fallback, so ArrowReader(Path) reported 0 proteins / 0 projections /
format_version 1 and raised nothing for:

  - a .parquetbundle FILE (the documented distribution format)
  - a directory that does not exist
  - a directory containing no ProtSpace tables

The try/except only fires on a read error, and a path that does not
exist is never read, so it never triggered.

Add _validate_data_path(), called before the try so its ValueError is
not re-wrapped. The bundle case names extract_bundle_to_dir() in the
message, since that is what the existing callers already do.

The check is "at least one core file" rather than "all three":
directories carrying only selected_annotations.parquet are legitimate
and test_bundle_version.py builds exactly those.
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.

1 participant