fix(streaming): skip model validators during partial streaming#1994
Merged
jxnl merged 4 commits into567-labs:mainfrom Jan 13, 2026
Merged
fix(streaming): skip model validators during partial streaming#1994jxnl merged 4 commits into567-labs:mainfrom
jxnl merged 4 commits into567-labs:mainfrom
Conversation
Model validators (mode="after") can fail during streaming when they
reference fields that haven't arrived yet. This commit adds automatic
wrapping of model validators to skip them during streaming.
Changes:
- Pass context={"partial_streaming": True} during streaming validation
- Wrap model validators to check context and skip during streaming
- Add PartialLiteralMixin for Literal/Enum types (uses partial_mode="on")
- Add comprehensive tests for validator behavior during streaming
The validators run normally during final validation (without streaming
context), ensuring data integrity while allowing smooth streaming.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 19ba756 in 1 minute and 51 seconds. Click for details.
- Reviewed
751lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/dsl/partial.py:49
- Draft comment:
PartialLiteralMixin is currently empty and acts solely as a marker for switching partial_mode. Consider adding a comment or future extension note if custom behavior is intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
2. instructor/dsl/partial.py:75
- Draft comment:
The context {'partial_streaming': True} is passed repeatedly in multiple validator calls. Consider defining a constant or helper to DRY this pattern. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 75% vs. threshold = 85% This comment is about code that was changed in the diff - the context parameter was added in multiple places. The suggestion to extract this into a constant is a reasonable DRY refactor. It's actionable (create a constant likePARTIAL_STREAMING_CONTEXT = {"partial_streaming": True}at module level). The additional rules explicitly state "Code should be DRY (Dont Repeat Yourself)" and "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This comment is both actionable and clear. However, I need to consider if this is "obvious or unimportant" - a simple dict with one key-value pair being repeated 4 times is borderline, but given the explicit DRY rule, this seems worth keeping. This is a very minor refactor for a simple dictionary that's only repeated 4 times. The value of extracting this is marginal - it's just one key-value pair, and having it inline might actually be more readable. The comment might be considered too nitpicky for such a small pattern. While it's a small refactor, the additional rules explicitly prioritize DRY principles, and this is a clear case of repetition that was introduced in this PR. The pattern appears 4 times, and if the context structure ever needs to change (e.g., adding more keys), having it as a constant would make that easier. The comment is actionable and follows the stated priorities. Keep this comment. It's a valid, actionable DRY suggestion that aligns with the explicit rules provided. The pattern is repeated 4 times in the changes, and extracting it to a constant would improve maintainability.
3. instructor/dsl/partial.py:197
- Draft comment:
In create_streaming_safe_validator, the looping over original validators assumes order. While Python 3.7+ maintains dict insertion order, a comment clarifying assumptions re ordering of validators might improve maintainability. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
4. instructor/dsl/partial.py:236
- Draft comment:
The approach of setting subsequent validators to no-op to ensure parent validators are overridden is clever. Adding an inline comment to explain why no-ops are inserted (to avoid duplicate execution) may benefit future maintainers. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
5. tests/dsl/test_partial.py:104
- Draft comment:
Debug print statements in test_partial_with_whitespace add noise. Consider removing or guarding them since tests ideally should avoid extraneous output. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_gSLJagE5U8NOMlU2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
…ming After streaming completes, validate the final object against the original model to enforce required fields. This ensures: - Required fields are validated at the end of streaming - Model validators run without streaming context - Incomplete responses from LLMs trigger retry mechanism Changes: - Store original model reference in Partial model (_original_model) - Add final validation at the end of all streaming methods - Update tests to provide complete data for final validation - Add comprehensive tests for final validation behavior Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ory fields Fields with default_factory (e.g., List[str] = Field(default_factory=list)) were failing final validation because the partial model sets them to None, and None is not valid for the original model's field type. Fix: Use exclude_none=True in model_dump() during final validation so fields with None values are excluded and the original model uses its default values instead. Co-Authored-By: Claude Opus 4.5 <[email protected]>
5 tasks
jxnl
added a commit
that referenced
this pull request
Jan 13, 2026
Co-Authored-By: Claude Opus 4.5 <[email protected]>
3 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.
Summary
PartialLiteralMixinfor handling Literal/Enum types during streamingFixes #1993
Changes
Model Validator Skipping During Streaming
context={"partial_streaming": True}during all streaming validation calls@model_validator(mode="after")validators to check context and skip during streamingFinal Validation After Streaming
_original_model)PartialLiteralMixin
partial_modefrom"trailing-strings"to"on"Noneinstead of failing validationImplementation Details
mode="wrap"validator to intercept validationValidationInfo.contextTest plan
test_model_validator_skipped_during_streaming- validators skipped with streaming contexttest_model_validator_runs_when_complete- validators run without streaming contexttest_multiple_model_validators- multiple validators all wrapped correctlytest_validators_run_without_streaming_context- final validation behaviortest_final_validation_catches_missing_required_fields- required fields enforcedtest_final_validation_runs_model_validators- validators run at endtest_streaming_yields_partial_objects_before_final_validation- streaming still works🤖 Generated with Claude Code