REVIEW: [BUG] Make pyspark an optional extra on overture-schema-pyspark

[sethfitz]

The proposed upstream PR body is a file in Files changed — .pr-review/BODY.md. Comment on it line by line like code. It lives only on this review branch; pyspark-optional-extra is clean and is what gets submitted.

Decisions that are yours

Attribution. The commit carries Co-authored-by: Adam Lastowka and the body opens by naming OvertureMaps#660 as the origin. If you'd rather this land as a review comment on his PR than as a superseding PR, nothing here is wasted — the branch works either way.

The probe vs. the alternatives. Three forms were on the table. Per-site guards at each from pyspark... import (your first suggestion) — I argued against: 22 sites, 21 unreachable, and the generated tree would need a renderer change. A sentinel try: import pyspark (what OvertureMaps#660 landed on) — works, but misreports an installed-but-broken pyspark as "not installed" unless it inspects e.name, which is the construct you objected to. The find_spec probe avoids the branch entirely. Detail is in the body's "It catches nothing" paragraph.

The option nobody argued: no guard at all. No module named 'pyspark' from a package named overture-schema-pyspark is not cryptic, and the guard costs ~10 lines, 3 tests, and a find_spec on every import. I'd still ship it — the extra is what makes that failure routine — but it was foreclosed by OvertureMaps#660's framing rather than weighed. Easy to drop if you disagree; the extras change stands alone.

Tests, stated honestly

Three tests added, one of which is a real RED/GREEN gate. Verified by reverting __init__.py alone to origin/main and re-running:

Test	Unguarded __init__	With probe
test_missing_pyspark_names_the_extra	FAIL	pass
test_unrelated_missing_dependency_surfaces_its_own_error	pass	pass
test_package_imports_when_pyspark_is_installed	pass	pass

The second is not idle — it fails against a broad-except implementation, which is the regression it exists to forbid — but it does not prove this fix. Worth knowing before "three new tests" reads as three proofs.

Verification

Check	Result
make check TESTMON= (pre-commit gate)	6201 passed
uv sync --locked --all-packages --all-extras	lockfile accepted
Bare venv, no pyspark → import	actionable message naming [spark]
Bare venv, no pyspark, no shapely → import	No module named 'shapely', unmasked
pyspark present, py4j removed → import	No module named 'py4j', unmasked
Leftover empty pyspark/ dir on path	falls through to bare error (known, shared with every alternative)

What I deliberately left out

The lockfile is hand-edited, not relocked. OvertureMaps#660's uv.lock carried unrelated relock churn (exclude-newer, an exceptiongroup marker); this edits only the overture-schema-pyspark entry, so the diff is 7 lines. uv sync --locked accepts it.

No README/docs change. Nothing in the repo documents installing this package, so there was no install line to update. If it's documented somewhere outside this repo, [spark] needs to reach it.

Before submitting

Your fork's main was
stale; I fast-forwarded sethfitz/schema:main to origin/main so this review PR diffs cleanly. Upstream needs exactly one change-type label — change type - minor 🤏 fits (non-breaking packaging change).

[sethfitz]

Submitted upstream as schema#681. Staging complete; review branch kept for provenance.