Conversation
Wrap list/Iterable results in ListResponse so create_with_completion can always return the raw provider response. Also ensure list[T] is treated like Iterable[T] during response model preparation to avoid misclassification as a simple type. Co-authored-by: jason <jason@jxnl.co>
|
Cursor Agent can help with this pull request. Just |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
instructor | f64c381 | Commit Preview URL Branch Preview URL |
Jan 17 2026, 05:43 PM |
|
@cursoragent fix ci please |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 971c9db in 1 minute and 41 seconds. Click for details.
- Reviewed
314lines of code in5files - Skipped
0files when reviewing. - Skipped posting
7draft 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/response_list.py:17
- Draft comment:
ListResponse is implemented as a list subclass with an extra attribute. Consider documenting or overriding key mutating methods (e.g. iadd, add, pop) if preserving the _raw_response is important when the list is modified. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
2. instructor/processing/response.py:240
- Draft comment:
In process_response_async, the raw response (an async generator) is stored in _raw_response. Ensure this behavior is documented so users know that the original async stream isn’t consumed elsewhere. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
3. instructor/processing/response.py:388
- Draft comment:
Similar wrapping of IterableBase responses into a ListResponse is duplicated in this function. Consider extracting a helper to reduce duplication and adhere to DRY. - Reason this comment was not posted:
Confidence changes required:60%<= threshold85%None
4. instructor/utils/core.py:600
- Draft comment:
In prepare_response_model, the check for list/Iterable types uses 'if origin is Iterable or origin is list:'. Consider using 'if origin in (Iterable, list):' for clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:60%<= threshold85%None
5. tests/test_list_response_wrapper.py:92
- Draft comment:
Tests for ListResponse are comprehensive. Consider adding tests to check that mutations (e.g. appending items) do not inadvertently affect the _raw_response attribute if such behavior is important. - Reason this comment was not posted:
Confidence changes required:40%<= threshold85%None
6. instructor/processing/response.py:265
- Draft comment:
Typo found in the debug message: 'Returning takes from IterableBase' likely should be 'Returning tasks from IterableBase'. - Reason this comment was not posted:
Comment was on unchanged code.
7. instructor/processing/response.py:390
- Draft comment:
Typo: The debug message reads "Returning takes from IterableBase". It seems like 'takes' should be corrected to 'tasks'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_dpDshUfM7QLJUTfv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@cursoragent fix ty and rugg and test and update changelog |
|
CI is fixed now — the failures were from (1) streaming I pushed a follow-up that:
Latest PR runs are green (ty + tests). |
Widen internal response processor return types and tighten TypedDict name handling. Adjust wrapper tests to satisfy ty-tests type bounds. Co-authored-by: jason <jason@jxnl.co>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 70e7263 in 2 minutes and 22 seconds. Click for details.
- Reviewed
70lines 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. instructor/processing/response.py:177
- Draft comment:
The return type was changed to 'Any' to accommodate ListResponse wrapping. Consider updating the docstring to clarify that batch extractions for list response models now return a specialized ListResponse. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 85% The comment is about the return type change toAnyand suggests updating the docstring. However, looking at the existing docstrings (lines 204-210 for async and lines 326-332 for sync), they already document the different return types including "If response_model is IterableBase with stream=True: returns list of models". The docstrings don't explicitly mentionListResponseby name, but they do describe when lists are returned. The comment seems to be asking for more specificity aboutListResponsebeing the wrapper type. However, this is an implementation detail that may not need to be in the public-facing docstring. The return type change toAnyis likely because the function can return many different types (ChatCompletion, ListResponse, model instances, etc.), and the docstring already describes these cases. The comment is somewhat speculative about what "should" be documented. The docstrings already document the various return types in detail. The comment is asking for a very specific implementation detail (mentioningListResponseby name) to be added, but this might be overly specific for a docstring. The existing documentation already explains when lists are returned. This could be seen as a minor documentation enhancement rather than a necessary fix. While the docstrings do mention returning lists, they don't explicitly mentionListResponsewhich is a specific wrapper type that carries_raw_response. However, this level of detail might be too implementation-specific for the docstring. The comment is also not pointing to a clear error or omission - it's a suggestion for enhancement. Given the rule that comments should only be made when there's clearly a code change required, this seems more like a "nice to have" rather than a "must fix". The comment is suggesting a documentation enhancement rather than pointing out a clear error. The docstrings already document the return types for different scenarios. While mentioningListResponsespecifically could add clarity, it's not a required fix and the comment is somewhat speculative about what documentation is needed.
2. instructor/processing/response.py:292
- Draft comment:
Similarly, the return type for process_response was updated to 'Any'. It would be helpful to update the docstring to inform consumers that list responses now come wrapped in a ListResponse. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 85% The comment is about documentation accuracy. The return type was changed toAny, which is less specific. The docstring currently says "If IterableBase: List of extracted model instances" but the actual implementation returns aListResponseobject (which is list-like but not a plain list). However, this is a suggestion to update documentation, not a code change. The rules state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." Documentation suggestions are borderline - they're not code changes. Also, the docstring already mentions what happens with IterableBase, just not the specific type name. The comment is asking for more precision in the docstring, which is a minor documentation improvement rather than a critical issue. The comment is technically correct - the docstring could be more precise about theListResponsetype. However, this is a documentation suggestion rather than a code change requirement. The existing docstring does describe the behavior, just not with the exact type name. Is this important enough to warrant a comment? While documentation accuracy is valuable, the rules explicitly state not to comment unless there's a clear code change required. This is a documentation enhancement suggestion, not a bug or code issue. The docstring already describes the behavior adequately, even if it could be more precise about the specific wrapper type. This comment should be deleted. It's a documentation suggestion rather than a required code change. The docstring already describes the behavior for IterableBase responses, and asking for more precision about theListResponsewrapper type is not critical enough to warrant a review comment.
3. instructor/utils/core.py:611
- Draft comment:
The refactored TypedDict handling using getattr for 'name' and 'annotations' improves type-safety and clarity. This defensive coding is a neat improvement. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the changes made, which violates the rule against making purely informative comments.
4. tests/test_list_response_wrapper.py:47
- Draft comment:
The tests now consistently use DummyCompletion to simulate provider responses, ensuring that the _raw_response attribute is preserved and that ListResponse is correctly wrapping the output. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states what has been done in the code without offering any critique or guidance.
Workflow ID: wflow_jgi8jtFfcoCPeP67
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Restore generator/async-generator returns for Iterable streaming so create_iterable works. Only wrap non-streaming Iterable outputs in ListResponse. Also refine list[T] handling to only treat list[BaseModel] as IterableModel, keeping list[int|str] on the simple-type path, and adjust ModelAdapter wrapping for ty. Co-authored-by: jason <jason@jxnl.co>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0d7025d in 1 minute and 25 seconds. Click for details.
- Reviewed
145lines 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. instructor/processing/response.py:232
- Draft comment:
In the async streaming branch, the code now directly returns the generator from from_streaming_response_async without wrapping it in a ListResponse, so the _raw_response attribute is not preserved. Please confirm that this trade‑off is acceptable, given that non‑streaming cases do attach _raw_response. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
2. instructor/processing/response.py:379
- Draft comment:
The sync non‑streaming branch correctly wraps IterableBase responses in a ListResponse to preserve the _raw_response attribute. It would be helpful to document the behavioral difference between streaming (raw response not preserved) and non‑streaming cases for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold85%None
3. instructor/utils/core.py:609
- Draft comment:
The refactored prepare_response_model now distinguishes between Iterable and list types appropriately. Consider extracting the inner BaseModel check (lines 609–625) into a helper function for improved readability and to avoid code duplication. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. tests/test_list_response_wrapper.py:56
- Draft comment:
The tests for non‑streaming and streaming IterableBase responses are comprehensive. It might be beneficial to add tests for simple type lists (e.g., list[int] or list[str]) to ensure they follow the simple type path rather than being wrapped as IterableModels. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
Workflow ID: wflow_H6LNZpAS325mkhZs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Document the fix for list[BaseModel] create_with_completion raw response crash (#1303). Co-authored-by: jason <jason@jxnl.co>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed dd58774 in 44 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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. CHANGELOG.md:11
- Draft comment:
Consider ensuring consistent terminology for the response models across documentation. The PR description refers to 'List[Model]' (or Object) while the changelog uses 'list[BaseModel]'. Aligning these can reduce confusion. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%The comment suggests aligning terminology between the PR description and the changelog. However, it violates the rule against asking the PR author to update the PR description. The focus should be on the code changes, not the PR description or changelog.
Workflow ID: wflow_3Feq2ongFfiG4UxV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@cursoragent merge main, resolve conflicts and fix any ci and update changelog |
What I did
CI status
Changelog
|
# Conflicts: # CHANGELOG.md # instructor/dsl/response_list.py # instructor/processing/response.py # instructor/utils/core.py Co-authored-by: jason <jason@jxnl.co>
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 82f4d53 in 1 minute and 54 seconds. Click for details.
- Reviewed
437lines of code in10files - 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/__init__.py:114
- Draft comment:
Good use of a try/except block to conditionally import the optional vertexai client. This prevents import failures if the dependency is misconfigured. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the use of a try/except block without offering any constructive criticism or guidance.
2. tests/test_list_response.py:19
- Draft comment:
The tests effectively verify that the ListResponse subclass correctly preserves the raw response and handles slicing, as well as wrapping IterableBase tasks. Good coverage! - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the tests are effective, which is not necessary for the PR author to know.
3. instructor/processing/response.py:260
- Draft comment:
Typographical error: The log message says "Returning takes from IterableBase" which is likely a typo. Consider replacing "takes" with "tasks". - Reason this comment was not posted:
Comment was on unchanged code.
4. instructor/processing/response.py:378
- Draft comment:
Typographical error: The debug message says "Returning takes from IterableBase", which likely should be "Returning tasks from IterableBase" to match the context (since it's processing model.tasks). - Reason this comment was not posted:
Comment was on unchanged code.
5. tests/test_list_response.py:43
- Draft comment:
Typo alert: In the comment on line 43, "to keeptyhappy." might be a typo. Consider changing it to "to keep type checking happy." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 85% This comment is about a typo/wording issue in a code comment, not about actual code logic. The rules state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." While the suggestion might make the comment slightly clearer, this is a very minor stylistic issue in a comment (not code). The current wording "to keeptyhappy" is understandable in context - it's informal but clear enough. This doesn't require a code change, it's just a suggestion to reword a comment for clarity. This falls under "obvious or unimportant" - it's not affecting functionality or correctness of the code itself. The comment could be seen as improving code clarity and documentation quality. "ty" might be confusing to some readers, and making comments more explicit could be valuable for maintainability. This is a test file, so clarity in explaining why certain patterns are used could be helpful. While clarity is good, the rules explicitly state not to comment unless there's clearly a code change required. This is about wording in a comment, not actual code. The current phrasing is understandable enough in context (especially for developers familiar with type checking). This is too minor and doesn't meet the bar for requiring action. This comment should be deleted. It's about minor wording in a code comment, not actual code logic or functionality. It doesn't meet the threshold of requiring a clear code change, and falls under "obvious or unimportant" suggestions.
Workflow ID: wflow_xLzS02cDzoPQBoXI
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@cursoragent merge main, resolve issues, and fix ruff/ci |
|
Co-authored-by: jason <jason@jxnl.co>
Co-authored-by: jason <jason@jxnl.co>
|
@cursoragent review |
|
I reviewed the PR end-to-end after merging
This should be ready to merge. |
fix: Prevent AttributeError for List[Model] batch extractions
Describe your changes
This PR resolves an
AttributeError: 'list' object has no attribute 'rawresponse'that occurred during batch extractions when the response model wasList[Object].The issue stemmed from
list[Model]being treated as a simple type, resulting in a plainlistbeing returned, which lacks the_raw_responseattribute expected bycreate_with_completion().This fix re-implements the solution from the unmerged PR #1870:
ListResponse, alistsubclass that carries the_raw_response.list[T]andIterable[T]response models are correctly processed asIterableModels.process_responseandprocess_response_asyncto wrap list-like outputs inListResponse, preserving the_raw_response.Issue ticket number and link
567-263
#1303
Checklist before requesting a review
Linear Issue: 567-263
Important
Fixes
AttributeErrorin batch extractions by introducingListResponseto preserve_raw_responsein list outputs and updates response processing functions accordingly.AttributeErrorincreate_with_completion()forlist[T]response models by introducingListResponseto preserve_raw_response.process_responseandprocess_response_asyncto wrap list outputs inListResponse.ListResponseinresponse_list.pyto handle list-like responses with_raw_response.test_list_response_wrapper.pyto testListResponseand ensure correct handling of iterable models.prepare_response_modelincore.pyto treatlist[Model]asIterableModel.This description was created by
for 82f4d53. You can customize this summary. It will automatically update as commits are pushed.
Note
Ensures list-based extractions retain the raw provider response and streaming behavior is consistent across iterable and partial models.
ListResponse(aliasResponseList) to carry_raw_responsefor list outputs; exported indsl/__init__.pyprocess_response/process_response_asyncto:IterableBase; streamPartialBaseappropriatelyIterableBaseresults inListResponsewith_raw_response_raw_responseforParallelBase; return.contentforAdapterBaseAnyprepare_response_modelto treatlist[Model]/Iterable[T]asIterableModel, keep scalar lists viaModelAdapter, avoid type-expression subscripting, and hardenTypedDict->BaseModelconversionCHANGELOG.mdWritten by Cursor Bugbot for commit f64c381. Configure here.