fix: narrow the parquet encoder's bare except so real failures are not reported as a missing extra - #398
Open
tschm wants to merge 3 commits into
Conversation
`_encode_frame_as_parquet` wrapped the optional-dependency lookup and the pandas->arrow conversion in one `except Exception` and returned `None`. `None` means "parquet is unavailable, use the default encoding", which is right when pyarrow is not installed and wrong for everything else the handler was catching: an install present but broken against its numpy, or a dtype, index or duplicate column name pyarrow will not take. In those cases the caller has pyarrow installed, asked for `frame_encoding= "parquet"`, and silently got npy — and the reason to choose parquet is that other tools can read the blob, so the loss surfaces much later. The two are now separate handlers. The dependency lookup catches `ImportError`, which is what `loman._extras.require` raises, and still falls back without a word: nothing was promised and nothing is broken. The conversion keeps catching `Exception` — pyarrow raises ArrowInvalid, ArrowNotImplementedError, ValueError and TypeError across the frames it cannot take, and narrowing would mean a new error type failing a save that used to succeed — but warns with `FrameEncodingFallbackWarning`, naming the frame's shape and the underlying error. Falling back rather than propagating is deliberate, and matches the behaviour `test_unrepresentable_frame_falls_back` already locked: a save must not fail because an optional codec has a limitation. What was wrong was the silence, not the fallback. This is the same shape as the existing `UnserializableFunctionWarning` — the save succeeded, and here is what it could not do. Tests: `test_unrepresentable_frame_falls_back` now asserts the warning; `test_broken_pyarrow_is_not_reported_as_an_absent_extra` makes `Table.from_pandas` raise `ImportError`, the one type the absent-extra path is allowed to swallow, so the test pins the distinction on where the failure came from rather than its type; `test_a_frame_parquet_can_take_warns_about_nothing` and the existing no-pyarrow test assert silence where silence is correct. Against the old single handler the two new assertions fail; with the fix all four pass. 1543 tests pass, coverage 98.94%, `ty check src` reports the same single pre-existing warning as master, and every pre-commit hook passes. Closes janushendersonassetallocation#392. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_encode_frame_as_parquetwrapped the optional-dependency lookup and the pandas→arrow conversion in a singleexcept Exceptionand returnedNone.Nonemeans "parquet is unavailable, use the default encoding" — right when pyarrow is not installed, and wrong for everything else that handler caught:Table.from_pandasrefusing a frame it genuinely cannot convert — unsupported dtype, duplicated or non-representable index, mixed-type object columns.In each of those the caller has pyarrow installed, asked for
frame_encoding="parquet", and silently got npy. Since the documented reason to choose parquet is that other tools can read the blob, the loss surfaces much later as "why can nothing else read my saved computation?".Change
Two handlers instead of one:
ImportErroris whatloman._extras.requireraises, so that is what the dependency path narrows to. The conversion keeps catchingExceptiondeliberately: pyarrow raisesArrowInvalid,ArrowNotImplementedError,ValueErrorandTypeErroracross the frames it will not take, and narrowing here would mean a future pyarrow error type failing a save that used to succeed.Why it warns rather than propagates
The issue allowed either. Propagating would have broken behaviour this repo already locked deliberately —
test_unrepresentable_frame_falls_backsaves a frame with duplicate column names and asserts the save succeeds, on the stated grounds that "a save must not fail because an optional codec has a limitation". What was wrong was the silence, not the fallback.FrameEncodingFallbackWarning(UserWarning)names the frame's shape and the underlying error. That is the same shape as the existingUnserializableConstantWarning/UnserializableFunctionWarning: the save succeeded, and here is what it could not do.Done when
Noneis returned from this path only when the optional pyarrow dependency cannot be resolvedTable.from_pandasfailure is reported rather than presented as an absent extraTests
test_unrepresentable_frame_falls_back— duplicate column names, a real pyarrow failure; now also asserts the warning, keeping the fallback assertionstest_broken_pyarrow_is_not_reported_as_an_absent_extra— makesTable.from_pandasraiseImportError, the very type the absent-extra path is allowed to swallow, so the test pins the distinction on where the failure came from rather than what type it istest_a_frame_parquet_can_take_warns_about_nothingand the existing no-pyarrow test — assert silence where silence is correct, so the warning keeps meaning somethingMutation-checked: with the warning class in place but the old single handler restored, both new assertions fail; with the fix, all four pass.
1543 tests pass, coverage 98.94%,
ty check srcreports the same single pre-existing warning as master, and every pre-commit hook passes.Closes #392.
🤖 Generated with Claude Code