Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: hotfix create_with_completion failing for AdapterBase, ParallelBase, IterableBase and PartialBase #1103

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ivanbelenky
Copy link
Contributor

@ivanbelenky ivanbelenky commented Oct 21, 2024

Describe your changes

The change solve create_with_completion failing with AdapterBase, ParallelBase, IterableBase and PartialBase types. Every single instructor.create_fn call is wrapped with a Creation object. The idea for this wrapper is to be as simple as a container for both the raw response from the LM client, and the processed response by instructor.

T = TypeVar("T", bound=Union[BaseModel, "Iterable[Any]", "Partial[Any]"])


class Creation(Generic[T]):
    raw: Any # probably should collapse to the ChatCompletion model
    processed: T

    def __init__(self, raw: Any, processed: T) -> None:
        self.raw, self.processed = raw, processed

Then retry_sync and retry_async return types were altered from T_Model to Creation[T_Model].

For further traceability in instructor calls, a runtime accessible Inspector|Monitor could be set up. Capturing calls as a postfix for every single create_fn call.

Related Issue


Important

Fixes create_with_completion failures by introducing Creation class to wrap responses, updating return types in instructor/client.py, instructor/patch.py, and instructor/retry.py.

  • Behavior:
    • Fixes create_with_completion failing for AdapterBase, ParallelBase, IterableBase, and PartialBase by introducing Creation class in instructor/client.py.
    • Creation class wraps raw and processed responses.
    • Updates retry_sync and retry_async in instructor/retry.py to return Creation[T_Model].
  • Functions:
    • Modifies create, create_partial, create_iterable, and create_with_completion in Instructor and AsyncInstructor classes to use Creation.
    • Updates new_create_sync and new_create_async in instructor/patch.py to handle Creation.
  • Misc:
    • Adds Generic import in instructor/client.py for Creation class.
    • Adjusts type hints and return types across affected functions.

This description was created by Ellipsis for 1f30dce. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 1f30dce in 35 seconds

More details
  • Looked at 300 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. instructor/client.py:28
  • Draft comment:
    Consider adding type annotations for the raw and processed attributes in the Creation class to improve code clarity and type safety.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The Creation class is missing type annotations for its attributes raw and processed. This can lead to confusion and potential misuse of the class.
2. instructor/client.py:37
  • Draft comment:
    Update the type of create_fn to Callable[..., Creation[T]] to ensure consistency with the constructor and type safety.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The create_fn attribute in the Instructor class is defined as Callable[..., Any], but it should be Callable[..., Creation[T]] to match the constructor and ensure type consistency.
3. instructor/client.py:368
  • Draft comment:
    Update the type of create_fn to Callable[..., Creation[T]] to ensure consistency with the constructor and type safety.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The create_fn attribute in the AsyncInstructor class is defined as Callable[..., Any], but it should be Callable[..., Creation[T]] to match the constructor and ensure type consistency.
4. instructor/retry.py:108
  • Draft comment:
    Update the return type in the docstring to Creation[T_Model] | None for consistency with the function's return type.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The retry_sync and retry_async functions have been updated to return Creation[T_Model] | None, but the docstring return type still mentions T_Model | None. This should be updated for consistency.
5. instructor/retry.py:189
  • Draft comment:
    Update the return type in the docstring to Creation[T_Model] | None for consistency with the function's return type.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The retry_sync and retry_async functions have been updated to return Creation[T_Model] | None, but the docstring return type still mentions T_Model | None. This should be updated for consistency.
6. instructor/client.py:27
  • Draft comment:
    The introduction of the Creation class is a change in the library code. Ensure that the documentation is updated to reflect this change.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The Creation class is introduced in the instructor/client.py file, which is a change in the library code. However, there is no indication that the documentation has been updated to reflect this change. According to the principles, if library code changes, the documentation should be updated accordingly.
7. instructor/patch.py:16
  • Draft comment:
    The introduction of the Creation class is a change in the library code. Ensure that the documentation is updated to reflect this change.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The Creation class is introduced in the instructor/patch.py file, which is a change in the library code. However, there is no indication that the documentation has been updated to reflect this change. According to the principles, if library code changes, the documentation should be updated accordingly.
8. instructor/retry.py:9
  • Draft comment:
    The introduction of the Creation class is a change in the library code. Ensure that the documentation is updated to reflect this change.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The Creation class is introduced in the instructor/retry.py file, which is a change in the library code. However, there is no indication that the documentation has been updated to reflect this change. According to the principles, if library code changes, the documentation should be updated accordingly.

Workflow ID: wflow_siD89ALUCSdoBjXz


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@jxnl
Copy link
Collaborator

jxnl commented Oct 28, 2024

will this break for folks who access _raw_response or type inference?

@ivanbelenky
Copy link
Contributor Author

Hey @jxnl good questions

  • access to _raw_response will not break, I did not eliminate it's assignment on process_response
  • not sure I know what you mean in the context of these changes by type inference. But I guess the answer is no, mainly because the changes are only wrapping responses from internal create_fn calls with a Creation object. If you can describe a test case scenario, I am happy to add it to the tst suite as well.

@conradlee
Copy link

Hey @ivanbelenky great that you tackled this problem. At the moment my code is starting to get polluted with all sorts of 'dummy models` that look like

class ListOfMyModel(BaseModel):
    items: list[BaseModel]

and the addition of this boilerplate seems to very much against the ethos of this library.
Any chance you could review this @jxnl ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants