Repository navigation
fix(result): blank lost rows to None instead of three different markers (#262) - #263
Merged
Merged
Conversation
…rs (#262) A lost row used to carry one of three strings in its output cell — "[SKIPPED]" from the skip policy, "null" from a failed batch response, "" from an empty one — depending on which path lost it. None of them are NaN, so df.isna() reported a frame full of holes as complete, and a caller had to know all three magic strings to find the losses by value. Converge them on None, in one place. A single pass over the result blanks the output cells of every row recorded in `errors`, keyed off the error list rather than by matching cell strings. That keying is deliberate: * No collision. A model may legitimately answer "null" or ""; only rows Ondine actually recorded as lost are touched, so a real answer that looks like a marker is never blanked. * Recovery-aware. After auto-retry a formerly-failed row may hold a real value again; a cell is blanked only if it still carries a loss marker. The authority on which rows were lost stays `errors` / `is_complete`; the None in the cell is the visible shadow of that. Add `result.lost_row_indices` so slicing survivors from losses is one step. Consequence worth noting: a run where *every* row failed used to hand back a frame of "null" strings that the quality check counted as valid, so it returned success. Those cells are now None, correctly counted as zero valid output, so such a run trips the whole-run guard and fails loudly. Behavioral change: lost cells are now None/NaN, not "[SKIPPED]"/"null"/"". Callers detecting loss by those strings should use is_complete / errors / df.isna() instead. The ambiguous empty-but-successful response (a row that was not recorded as an error) is left untouched — it is data, not a tagged loss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSkipped output markers are normalized to ChangesLost-row output handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to This PR standardizes lost cells to None and adds lost-row indexing, but recovered retry results can still be marked as lost, causing valid rows to be discarded and completeness to be reported incorrectly; merge should wait for that correctness issue to be fixed or explicitly accepted. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ondine/api/pipeline.py`:
- Around line 1731-1743: The retry flow around _auto_retry_failed_rows() must
reconcile recovered rows with result.errors and row metrics before returning.
Remove recovered indices from lost_row_indices, update is_complete and
skipped/failed metrics consistently, and add a regression assertion verifying
recovered rows are absent from lost_row_indices.
In `@tests/unit/test_pipelined_streaming.py`:
- Line 133: Strengthen the assertions in the test around
pipeline.execute_stream_pipelined so each yielded chunk is verified to contain
exactly chunk_size rows and all expected input rows, rather than only checking
the number of result objects. Preserve the existing chunk-count assertion while
validating completeness and row counts for every result.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d480c8ee-e714-4697-92d3-dfffd2c10280
📒 Files selected for processing (8)
docs/guides/error-handling.mdondine/api/pipeline.pyondine/core/models.pytests/e2e/test_pipeline_conformance.pytests/e2e/test_router_conformance.pytests/integration/test_end_to_end.pytests/unit/test_pipelined_streaming.pytests/unit/test_silent_failure.py
Comment on lines
+1731
to
+1743
| This converges them on ``None``, in one place, keyed off ``errors`` | ||
| rather than by matching cell strings. Two reasons for that: | ||
|
|
||
| * **No collision.** A model may legitimately answer ``"null"`` or | ||
| ``""``; only rows Ondine actually recorded as lost are touched, so a | ||
| real answer that happens to look like a marker is never blanked. | ||
| * **Recovery-aware.** After auto-retry a formerly-failed row may hold a | ||
| real value again; a cell is only blanked if it still carries a loss | ||
| marker, so recovered rows keep their answer. | ||
|
|
||
| The authority on *which* rows were lost stays ``errors`` / | ||
| ``is_complete``; the ``None`` in the cell is the visible shadow of that, | ||
| not a second source of truth. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reconcile recovered rows with loss metadata.
_auto_retry_failed_rows() replaces recovered cells but leaves the original result.errors and skipped or failed metrics unchanged. This method then preserves the recovered value. A recovered row can therefore contain a valid answer while lost_row_indices still includes it and is_complete remains False.
Remove or reclassify errors for recovered rows. Update the row metrics before returning the retried result. Add a regression assertion that a recovered row is absent from lost_row_indices.
🤖 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 `@ondine/api/pipeline.py` around lines 1731 - 1743, The retry flow around
_auto_retry_failed_rows() must reconcile recovered rows with result.errors and
row metrics before returning. Remove recovered indices from lost_row_indices,
update is_complete and skipped/failed metrics consistently, and add a regression
assertion verifying recovered rows are absent from lost_row_indices.
|
|
||
| with patch("litellm.acompletion", side_effect=client.ainvoke): | ||
| results = list(pipeline.execute_stream_pipelined(chunk_size=chunk_size)) | ||
| results = list(pipeline.execute_stream_pipelined(chunk_size=chunk_size)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that each chunk contains all input rows.
Line 133 only proves that two chunk result objects were yielded. A regression that drops rows inside a chunk still passes.
Assert that each result has chunk_size rows and that it is complete.
Proposed test assertions
for i, chunk_result in enumerate(results):
assert chunk_result.success, f"Chunk {i} failed"
assert hasattr(chunk_result, "data"), f"Chunk {i} missing data"
+ assert len(chunk_result.to_list()) == chunk_size
+ assert chunk_result.is_complete🤖 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 `@tests/unit/test_pipelined_streaming.py` at line 133, Strengthen the
assertions in the test around pipeline.execute_stream_pipelined so each yielded
chunk is verified to contain exactly chunk_size rows and all expected input
rows, rather than only checking the number of result objects. Preserve the
existing chunk-count assertion while validating completeness and row counts for
every result.
The single-row retry branch returned raw text ("Processed_5"), which the retry
sub-pipeline's JSON batch parser cannot read, so row 5 never actually
recovered — the test only passed because the old failure marker was a non-null
string that satisfied notna(). With lost cells now None (#262) the masking is
gone; return the array shape the retry parses so the row genuinely recovers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Decision
Resolves the marker half of #254, tracked as #262: one representation (
None) on all three loss paths, materialized in a single pass keyed offerrors— not by matching cell strings.A lost row used to carry
[SKIPPED](skip policy),"null"(failed batch), or""(empty response) depending on which path lost it. None areNaN, sodf.isna()reported holes as complete and a caller had to know all three magic strings.Mechanism (why keyed off
errors)One pass blanks the output cells of every row recorded in
errors. Keying off the error list — not cell strings — buys two properties a blanket string-replace can't:"null"or""; only rows Ondine recorded as lost are touched.Authority on which rows were lost stays
errors/is_complete(from #260); theNonein the cell is its visible shadow. New:result.lost_row_indicesfor one-step survivor/loss slicing.Bonus correctness fix
A run where every row failed used to return a frame of
"null"strings that the quality check counted as valid →success=True. Those cells are nowNone→ zero valid output → the whole-run guard fires (same guard that already caught""/[SKIPPED]). Two tests unknowingly relying on that masking are corrected.Lost cells are now
None/NaN, not"[SKIPPED]"/"null"/"". Anyone detecting loss by those strings switches tois_complete/errors/df.isna(). Behaviorally breaking — I did not add aBREAKING CHANGE:footer (that would force release-please to a major bump); say the word if you want a major, otherwise it ships as a fix with a prominent note.The ambiguous empty-but-successful response (not recorded as an error) is deliberately left as-is — data, not a tagged loss.
Verification
null) verified to blank toNone,df.isna()truthful, survivors intactai assistance: i directed this work with help from claude code.
Summary by CodeRabbit
Bug Fixes
Noneinstead of the[SKIPPED]placeholder.nullresponses, and empty responses are preserved correctly.New Features
Documentation