Update from_litellm type hints to properly return AsyncInstructor#1324
Merged
ivanleomk merged 3 commits into567-labs:mainfrom Mar 31, 2025
Merged
Update from_litellm type hints to properly return AsyncInstructor#1324ivanleomk merged 3 commits into567-labs:mainfrom
ivanleomk merged 3 commits into567-labs:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to acf2563 in 41 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. instructor/client.py:561
- Draft comment:
Usinginspect.iscoroutinefunctionmay not cover all asynchronous cases, such as coroutine objects. Consider usingasyncio.iscoroutinefunctionor checking forAwaitableto ensure accurate type inference. - Reason this comment was not posted:
Confidence changes required:80%
The PR correctly updates the type hints for thefrom_litellmfunction to distinguish between synchronous and asynchronous completions. However, the implementation offrom_litellmusesinspect.iscoroutinefunctionto determine if the function is asynchronous, which may not cover all cases, such as when the function is a coroutine but not defined withasync def. This could lead to incorrect type inference.
2. instructor/client.py:544
- Draft comment:
The overloads forfrom_litellmare inconsistent with the implementation. The first overload should be forCallable[..., Any]returningInstructor, and the second forCallable[..., Awaitable[Any]]returningAsyncInstructor. This matches the logic in the implementation whereinspect.iscoroutinefunctiondetermines the return type. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_QjN2oTmrAnnuQ7wU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
Author
|
Forgot to mention that the actual return types were correct, it was just the type hints that were incorrect. correctly identifies that acompletion is an async function. |
ivanleomk
approved these changes
Mar 31, 2025
Contributor
ivanleomk
left a comment
There was a problem hiding this comment.
Looks good to me, approved and merging in
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.
Type Checking was previously failing when attempting to create an AsyncInstructor because
litellm.acompletionis actually of typeCallable[..., Awaitable[Any]]and notAwaitable. This was causing the type hinter to match onCallable[..., Any]. With these changes, the type hints properly return AsyncInstructor.e.g.
Important
Update
from_litellmtype hints to correctly returnAsyncInstructorfor async completions.from_litellmoverloads to useCallable[..., Awaitable[Any]]for async completions andCallable[..., Any]for sync completions.from_litellmreturnsAsyncInstructorfor async completions andInstructorfor sync completions.This description was created by
for acf2563. It will automatically update as commits are pushed.