fix: harden hail_init credentials check and DuckDB index metadata#97
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- hail_init now raises FileNotFoundError when the GCS credentials path is provided but does not exist, instead of silently skipping it and failing later with an opaque GCS auth error; extracted into _export_gcs_credentials (#76). - write_metadata_tables writes its five metadata tables in one transaction so an interrupted init leaves no partially populated index that a later append/finalize would read as corrupt metadata (#87). - read_and_validate_pops_legends rejects an empty table-pair list with a clear ValueError instead of a raw IndexError (#87). - finalize_duckdb_index logs a warning when the sequences table is empty (#87). Closes #76 Closes #87 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5d55ad9 to
53f59f9
Compare
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Action performedReview finished.
|
## Summary Pure style/convention cleanup (#82) — no behavior change. Kept separate from the functional infra PR (#97) per the "never mix formatting and functional" rule. ## Changes - **Keyword-only `hail_init`** — added the leading `*` and updated its four call sites (`extract_gnomad_afs`, `extract_gnomad_single_afs`, `extract_sample_metadata`, `gnomad_hail_table_test_data`) to pass `gcs_credentials_path` by keyword. - **`Raises:` blocks** added to `extract_gnomad_afs` and `extract_gnomad_single_afs` (the two extract tools that actually raise `ValueError`). - **Builtin generics in `main.py`** — `list[...]` and `collections.abc.Callable` instead of `typing.List` / `typing.Callable`. ## Scope notes (the original nit was slightly off) - `gnomad_hail_table_test_data` **already** led with `*`, so only its call site changed. - `extract_sample_metadata` raises nothing, so it gets no `Raises:` block (only its call site changed). ## Conflict-free with #97 This PR touches only `hail_init`'s **signature line** (the `*`); PR #97 touched `hail.py`'s **body + docstring**. Different hunks, so they merge cleanly in sequence regardless of order. ## Verification - `uv run --directory divref poe fix-and-check-all` — 180 passed; ruff (check + format) and mypy clean. Closes #82 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Small infrastructure-hardening fixes across
hail.pyand the DuckDB index builders.Changes
#76 — fail fast on a missing credentials file. In GCS mode,
hail_initpreviously setGOOGLE_APPLICATION_CREDENTIALSonlyif path.exists(), so a provided-but-missing file was silently ignored and surfaced later as an opaque GCS auth error. It now raisesFileNotFoundErrorwith a clear remediation message. Extracted into_export_gcs_credentials(also keepshail_initunder the complexity limit).#87 — DuckDB index guards.
write_metadata_tableswrites its five metadata tables in a single transaction, so an interruptedinitleaves no partially populated index (which a laterappend/finalizewould read as corrupt metadata).read_and_validate_pops_legendsrejects an emptytable_pairslist with a clearValueErrorinstead of a rawIndexError.finalize_duckdb_indexlogs a warning when thesequencestable is empty, rather than silently producing a zero-row index.Tests
test_hail.py—hail_initraisesFileNotFoundErrorfor a missing credentials file andValueErrorwhen none is provided (no Hail context needed).test_read_and_validate_pops_legends_empty_raises— empty list →ValueError.test_finalize_warns_on_empty_sequences— emptysequencestable → warning (pure DuckDB).Verification
uv run --directory divref poe fix-and-check-all— 184 passed; ruff (check + format) and mypy clean.Note on overlap with the upcoming style PR
This PR touches only
hail.py's body + docstring. Thehail_initkeyword-only (*) change stays in the house-rule PR (#82) together with its call-site updates — a different hunk, so the two merge cleanly in sequence.Closes #76
Closes #87
🤖 Generated with Claude Code