feat: Collection of Minor Changes after review

[znorgaard]

Summary

A collection of small correctness, quality, and consistency changes across the divref toolkit and the Snakemake workflows. The bulk is non-behavioral (docstrings, typing, refactors, added tests), with two behavioral changes called out below. All checks pass (uv run poe check-all: format, lint, mypy, tests; snakefmt clean).

Behavioral changes (worth a closer look)

• fix(haplotype): break sort ties for co-located SNP + insertion — get_haplo_sequence / haplo_coordinates sorted component variants by (position, len(ref)), which ties a SNP and an insertion at the same position. The tie was resolved by input order, and when the insertion applied first the SNP was silently dropped from the emitted sequence (a PASS row in the FASTA/DuckDB deliverable). Added a tertiary tiebreak on alt length so a substitution always composes before a pure insertion, plus a reversed-order regression test.
• feat(fasta): order FASTA output by genomic position — create_divref_fasta now emits rows ORDER BY start, sequence_id, making per-chromosome FASTA output deterministic/checksummable across runs. Same content, stable byte-ordering.

Toolkit refactors & hardening

• refactor(duckdb) — extract a shared sequences_table_exists helper; reuse it in finalize, append_contig, and create_divref_fasta (dedups the repeated information_schema check).
• refactor(append-contig) — reuse the helper; switch the per-chunk insert to INSERT ... BY NAME (guards against column-order drift); eq=False on _RemapArrays; document the 0-based half-open coordinate convention at the DuckDB boundary.
• refactor(extract) — single-source the HGDP+1KG table URIs from defaults; fix stale "sample metadata" docstrings; validate the .ht output path before starting Hail; type freq_threshold as 0.0.
• refactor(hail) — use pathlib for connector-JAR path handling; validate the credentials path with is_file().
• fix(haplotype-compat) — parse_variants_string raises an informative ValueError naming the offending token; corrected the classify_pair / REASONS documentation.
• refactor — replace to_hashable_items with frozenset(d.items()); drop the unused n_groups from _compute_locus_groups's return.

Observability

• chore(compute-haplotypes) — log the count of samples dropped for an undefined population; document all five output tables; add comments for the biallelic call_stats and row_idx int32-downcast invariants.

Tests

• test(hail): parametrized sub-1GB Spark memory guards.
• FASTA genomic-ordering regression test (fixtures updated to the real schema); removed an unused test helper.
• Reversed-order SNP+insertion regression test (guards the fix above).

Style & consistency

• style — modern typing (PEP 695 type alias, collections.abc imports), lazy %s logging, Final constants with a provenance comment for the population set.

Workflows, CI & docs

• refactor(workflows) — curl --fail over wget (macOS-safe, fails loudly); chromosomes config-schema pattern; comment on the forced .fai name; fix an implicit string concat; correct the credentials comment.
• CI/pixi — lock the pixi env in the Snakemake-linting action (--locked); pixi lock-file fixes.
• README — note the gcloud CLI prerequisite for GCS runs.

Not in this PR (tracked follow-ups)

Larger items surfaced in review, deliberately left for separate PRs:

• remap_divref pass: extract helpers to drop the C901 complexity, and a pandas→polars streaming rewrite.
• extract_gnomad_single_afs: an in_sites_table override + happy-path test for the default v4.1 JOINT path (currently untested), and a shared pop_freq_indices helper.
• Schema single-sourcing (index-path/table/column-name constants; shared output-partition constant).
• Decisions pending: %.5f gnomAD-AF truncation, .fai naming convention, no_apply_filters → apply_filters, and softening the append_contig memory-bound docstrings.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added validation for required sequence data before FASTA generation and index finalization.
  • FASTA output now follows deterministic genomic ordering.
  • Added clearer validation for chromosome names and malformed variant input.
  • Haplotype ordering now consistently handles substitutions and insertions.

• Bug Fixes

  • Improved cloud credential and Spark connector file validation.
  • DuckDB sequence imports now align columns by name.
  • Workflow downloads now report failures more clearly.

• Documentation

  • Clarified authentication prerequisites, configuration-file usage, checkpoints, and workflow behavior.

[coderabbitai]

Warning

Review limit reached

@ameynert, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fbdcb41d-43da-4aae-80a0-c4ba8c4e389a

📥 Commits

Reviewing files that changed from the base of the PR and between d7444a6 and c21561b.

⛔ Files ignored due to path filters (1)

• pixi.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• .github/workflows/python_package.yml
• .github/workflows/workflow_linting.yml

📝 Walkthrough

Walkthrough

This PR updates DuckDB index and FASTA handling, haplotype ordering and frequency extraction, Hail path validation, type declarations, workflow validation, dependency constraints, logging, and related documentation and tests.

Changes

DivRef consistency and workflow updates

Layer / File(s)	Summary
Runtime contracts and validation divref/divref/alias.py, divref/divref/defaults.py, divref/divref/hail.py, divref/pyproject.toml, divref/tests/test_hail.py	Type aliases and constants use updated declarations; Hail credentials and connector paths require files; output validation and dependency constraints are updated.
Haplotype and frequency processing divref/divref/haplotype.py, divref/divref/haplotype_compat.py, divref/divref/tools/extract_gnomad_*.py, divref/tests/test_haplotype.py	Variant ordering gains an alternate-allele-length tie-breaker, malformed variant tokens receive clearer errors, and population metadata lookup uses frozenset keys.
Haplotype computation contracts and reporting divref/divref/tools/compute_haplotypes.py, divref/divref/tools/gnomad_hail_table_test_data.py	Locus-group returns, checkpoint documentation, missing-population reporting, and computation comments are updated.
Sequence-table detection and index writes divref/divref/duckdb_index.py, divref/divref/tools/append_contig_to_duckdb_index.py, divref/divref/tools/finalize_duckdb_index.py, divref/divref/tools/init_duckdb_index.py	Sequence-table detection is centralized, DuckDB inserts use name-based alignment, and index path construction is adjusted.
Validated and ordered FASTA streaming divref/divref/tools/create_divref_fasta.py, divref/tests/tools/test_create_divref_fasta.py	FASTA generation validates the required table, streams Arrow batches, orders by genomic start and sequence ID, and updates test fixtures.
Workflow checks and input handling .github/workflows/workflow_linting.yml, pixi.toml, workflows/*, README.md, divref/divref/main.py	Linting uses locked Pixi execution, chromosome inputs are schema-validated, downloads use curl with failure handling, command logging is parameterized, and workflow instructions are clarified.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• fg-labs/divref-wf#39: Related FASTA generation and DuckDB sequence handling.
• fg-labs/divref-wf#62: Related sequence-table detection and DuckDB index workflow changes.
• fg-labs/divref-wf#64: Related haplotype ordering and tie-break behavior.

Suggested reviewers: ameynert

Poem

I hopped through indexes, tidy and bright,
Sorted each sequence in genomic light.
The haplotypes twirled, the workflows locked,
Bad paths and chromosomes now get checked.
Thump, thump—clean data down the lane!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is related to the PR, but it is too generic to clearly summarize the main changes or distinguish this patch from other review follow-ups.	Use a more specific title that names the primary changes, such as deterministic haplotype/FASTA ordering and DuckDB/Hail/workflow updates.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 97.44% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

divref/divref/defaults.py (1)

28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use a tuple to fully enforce immutability.

While Final prevents reassignment of POPULATIONS, the underlying list remains mutable. As per coding guidelines, prefer immutable data. Consider defining this as a tuple (e.g., tuple[str, ...]) and updating downstream function signatures that currently expect a list[str].

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@divref/divref/defaults.py` at line 28, Change the POPULATIONS constant from a
mutable list to an immutable tuple, using an appropriate tuple type annotation.
Update downstream functions that consume POPULATIONS, including their parameter
annotations, to accept the tuple type without changing behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Line 33: Update the sentence introducing Hail-dependent tools to replace the
comma splice with clear wording, preserving the requirement that users log in
and install the gcloud CLI first.

---

Nitpick comments:
In `@divref/divref/defaults.py`:
- Line 28: Change the POPULATIONS constant from a mutable list to an immutable
tuple, using an appropriate tuple type annotation. Update downstream functions
that consume POPULATIONS, including their parameter annotations, to accept the
tuple type without changing behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 38a3c1ca-0de0-4a63-bce5-3c7f768146bf

📥 Commits

Reviewing files that changed from the base of the PR and between e1a4fc2 and e712a72.

📒 Files selected for processing (27)

• .github/workflows/workflow_linting.yml
• CONTRIBUTING.md
• README.md
• divref/divref/alias.py
• divref/divref/defaults.py
• divref/divref/duckdb_index.py
• divref/divref/hail.py
• divref/divref/haplotype.py
• divref/divref/haplotype_compat.py
• divref/divref/main.py
• divref/divref/tools/append_contig_to_duckdb_index.py
• divref/divref/tools/compute_haplotypes.py
• divref/divref/tools/create_divref_fasta.py
• divref/divref/tools/extract_gnomad_afs.py
• divref/divref/tools/extract_gnomad_single_afs.py
• divref/divref/tools/finalize_duckdb_index.py
• divref/divref/tools/gnomad_hail_table_test_data.py
• divref/divref/tools/init_duckdb_index.py
• divref/tests/test_hail.py
• divref/tests/test_haplotype.py
• divref/tests/test_main.py
• divref/tests/tools/test_create_divref_fasta.py
• pixi.toml
• workflows/compare_divref_gnomad.smk
• workflows/config/config_schema.yml
• workflows/create_test_data.smk
• workflows/generate_divref.smk

💤 Files with no reviewable changes (1)

• pixi.toml

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the sentence structure.

Replace the comma splice with clearer wording, for example: “Log in before running any Hail-dependent tools; the gcloud CLI must be installed first:”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 33, Update the sentence introducing Hail-dependent tools
to replace the comma splice with clear wording, preserving the requirement that
users log in and install the gcloud CLI first.

[ameynert]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.