feat/update partial streaming #1920
Open
+49
−34
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 15541d5 in 1 minute and 6 seconds. Click for details.
- Reviewed
151lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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. CONTRIBUTING.md:244
- Draft comment:
The evals directory link has been updated from /tests/llm/test_openai/evals to /tests/llm. Please confirm that this new URL is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%The comment is asking for confirmation on a change in a directory link, which is not allowed according to the rules. It doesn't provide a specific suggestion or point out a clear issue with the change.
2. instructor/dsl/partial.py:247
- Draft comment:
The chunk processing now checks for None and converts non-string chunks. To adhere to DRY principles, consider extracting this conversion (including logging or handling of conversion exceptions) into a helper function shared by both sync and async methods. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
3. instructor/dsl/partial.py:264
- Draft comment:
Joining all buffered chunks with ''.join(chunk_buffer) instead of using only the first element improves robustness. Verify that appending to the cumulative 'potential_object' as done here is the desired behavior. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. tests/dsl/test_partial.py:95
- Draft comment:
Defining global constants for 'partial_chunks' and 'expected_model_from_chunks' improves readability and maintainability in the tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative, as it praises the use of global constants for readability and maintainability. It does not provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_LozxKpaLSwA6Pnqn
You can customize 
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 6472406 in 2 minutes and 27 seconds. Click for details.
- Reviewed
151lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6draft 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. CONTRIBUTING.md:241
- Draft comment:
Updated evals directory link. Please ensure that this new location aligns with the repository structure and that any related documentation is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold85%The comment is asking the PR author to ensure that the new location aligns with the repository structure and that related documentation is updated. This violates the rule against asking the author to ensure things. However, it does touch on documentation updates, which is relevant if the change involves library code. Since the comment is not specific about library code changes, it seems to be more of a general request for confirmation, which is not allowed.
2. instructor/dsl/partial.py:247
- Draft comment:
Good addition of type and None checks for the chunk. Consider adding an inline comment to clarify that non-string chunks are coerced to string to ensure robust streaming input processing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% The code is fairly self-explanatory - it's checking if something is a string, and if not, converting it to a string. The comment suggestion is purely informative and doesn't suggest any code change. According to the rules, "Do NOT comment unless there is clearly a code change required. Do not make purely informative comments." This comment is asking to add documentation/explanation, not to change the logic. The code itself is straightforward enough that a developer can understand what's happening. The phrase "to ensure robust streaming input processing" is somewhat vague and doesn't add much value beyond what the code already shows. This appears to be a purely informative comment that doesn't require any code change. However, the rules also mention "Extremely Complicated Code Needs Comments" as an additional rule to check. Could this code be considered complicated enough to warrant a comment? The try-except with str() conversion and continue logic might benefit from explanation about why we're being defensive here. While the code does have some defensive programming with try-except, it's not "extremely complicated" - it's a straightforward type coercion with error handling. The pattern is common and self-documenting. The suggested comment doesn't add significant clarity beyond what the code already expresses. The rule states "Do NOT comment unless there is clearly a code change required" and this is purely asking for documentation, not a code change. This comment should be deleted. It's a purely informative suggestion to add documentation, which violates the rule that comments should only be made when there is clearly a code change required. The code is straightforward enough to understand without additional inline comments.
3. instructor/dsl/partial.py:255
- Draft comment:
The condition 'if len(chunk_buffer) < 2:' delays processing until at least two chunks are buffered. Please add an inline comment explaining why two chunks are needed (e.g., to avoid processing incomplete data). - Reason this comment was not posted:
Comment was on unchanged code.
4. instructor/dsl/partial.py:263
- Draft comment:
Using ''.join(chunk_buffer) for concatenation is a good fix. Note that potential_object is cumulatively appended over iterations—ensure this is the intended behavior for streaming partial responses. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold85%The comment is asking the author to ensure that the cumulative appending ofpotential_objectis intended. This is a request for confirmation, which is against the rules. However, it does point out a specific behavior that might be unintended, which could be useful for the author to consider. The comment is not making a suggestion or asking for a test, so it doesn't fully align with the examples of good comments. Overall, it seems to be more of a request for confirmation than a suggestion or identification of an issue.
5. instructor/dsl/partial.py:289
- Draft comment:
The async version now mirrors the synchronous improvements. Consider adding a similar inline comment about accumulating at least two chunks before processing for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% The comment is suggesting to add an inline comment explaining thelen(chunk_buffer) < 2check. However, looking at the actual code, neither the sync nor async version has this comment currently. The suggestion mentions "for consistency" but there's nothing to be consistent with. The logic itself is somewhat self-explanatory - it's accumulating chunks before processing. This seems like it would be adding documentation where it's not strictly necessary. The code change is clear enough: it buffers chunks and processes them when there are at least 2. This is a suggestion to add a comment, not pointing out a bug or code quality issue. According to the rules, comments should not be purely informative and should only be made if there's clearly a code change required. Perhaps the logic of "accumulate at least two chunks" is not immediately obvious to future maintainers, and adding a comment could help explain the rationale behind this buffering strategy. The comment might prevent confusion about why we're not processing single chunks immediately. While the rationale might not be immediately obvious, the code itself is fairly readable -if len(chunk_buffer) < 2: continueis self-documenting to some degree. More importantly, this comment is purely informative and doesn't suggest a required code change. The rules explicitly state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." Adding inline comments is a style preference, not a required change. This comment suggests adding documentation but doesn't identify any code issue or required change. It's a style suggestion about adding an inline comment, which violates the rule that comments should only be made when there's clearly a code change required. The comment should be deleted.
6. tests/dsl/test_partial.py:110
- Draft comment:
Test now iterates through models and asserts against expected output. It may be beneficial to also assert that the total number of yielded models matches the expected count to catch any off-by-one errors. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
Workflow ID: wflow_Ljs5C4o5gR30GRGh
You can customize
Contributor
Author
|
@jxnl On the failing tests: I'm getting import module errors even though PR changes are non related. Can you point me in the direction for next steps? |
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.
Important
Update
model_from_chunksfunctions to handle non-string chunks and improve buffer handling, with corresponding test updates.model_from_chunksandmodel_from_chunks_asyncinpartial.pyto handle non-string chunks by converting them to strings or skipping them if conversion fails.chunk_bufferand processing when buffer length is at least 2.test_partial_with_whitespaceandtest_async_partial_with_whitespaceintest_partial.pyto usepartial_chunksandexpected_model_from_chunksfor validation.CONTRIBUTING.mdto correct the link to the evals directory.This description was created by
for 6472406. You can customize this summary. It will automatically update as commits are pushed.