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

Revert "chore: Include types to instructor.patch" #437

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

jxnl
Copy link
Collaborator

@jxnl jxnl commented Feb 15, 2024

Reverts #422


Ellipsis 🚀 This PR description was created by Ellipsis for commit c5db786.

Summary:

This PR reverts a previous change that included types to instructor.patch, and updates type hints, imports, and related files for improved type safety and clarity.

Key points:

  • Reverted previous changes that included types to instructor.patch
  • Modified type hints and imports in instructor/patch.py
  • Updated .github/workflows/mypy.yml and tests/test_patch.py to reflect these changes

Generated with ❤️ by ellipsis.dev

@jxnl
Copy link
Collaborator Author

jxnl commented Feb 15, 2024

@savarin the openai api call tests are all failing due to missing optionals.

@jxnl jxnl enabled auto-merge (squash) February 15, 2024 19:19
@jxnl jxnl disabled auto-merge February 15, 2024 19:21
@jxnl jxnl merged commit e579144 into main Feb 15, 2024
10 of 12 checks passed
@jxnl jxnl deleted the revert-422-include-types-6 branch February 15, 2024 19:21
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.

Changes requested. Reviewed entire PR up to commit c5db786

Reviewed 429 lines of code across 3 files in 3 minute(s) and 13 second(s).

See details
  • Skipped files: 0
  • Confidence threshold: 85%
  • Drafted 6 additional comments.
  • Workflow ID: wflow_ayL8NT6raMkBpLS8
View 6 draft comments

These comments were drafted by Ellipsis, but were filtered out of the final review. They're included here so you can see our internal thought process and help you configure your ellipsis.yaml.

Drafted 6 comments under confidence threshold

Filtered comment at instructor/patch.py:60

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function handle_response_model has been changed from Tuple[Union[Type[OpenAISchema], ParallelBase], Dict[str, Any]] to Union[Type[OpenAISchema], dict]. This could potentially cause issues if the function is expected to return a tuple elsewhere in the codebase.

Filtered comment at instructor/patch.py:168

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function process_response has been changed from Union[OpenAISchema, List[OpenAISchema]] to Union[T_Model, T]. This could potentially cause issues if the function is expected to return a specific type elsewhere in the codebase.

Filtered comment at instructor/patch.py:213

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function process_response_async has been changed from Union[OpenAISchema, List[OpenAISchema], Dict[str, Any], Generator[Any, None, None]] to T. This could potentially cause issues if the function is expected to return a specific type elsewhere in the codebase.

Filtered comment at instructor/patch.py:225

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function retry_async has been changed from Union[OpenAISchema, List[OpenAISchema], Dict[str, Any], Generator[Any, None, None]] to T. This could potentially cause issues if the function is expected to return a specific type elsewhere in the codebase.

Filtered comment at instructor/patch.py:327

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function retry_sync has been changed from Union[OpenAISchema, List[OpenAISchema]] to no return type. This could potentially cause issues if the function is expected to return a specific type elsewhere in the codebase.

Filtered comment at instructor/patch.py:444

Confidence changes required: 66%

Commentary: The PR is reverting a previous change that included types to instructor.patch. The changes seem to be mostly about type annotations and refactoring. There are no obvious logical bugs or security issues. However, there are some potential issues with the type annotations and the removal of certain imports.

The return type of the function patch has been changed from Union[OpenAI, AsyncOpenAI, None] to Union[OpenAI, AsyncOpenAI]. This could potentially cause issues if the function is expected to return a specific type elsewhere in the codebase.


Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

@@ -37,6 +34,13 @@
from .function_calls import Mode, OpenAISchema, openai_schema
Copy link
Contributor

Choose a reason for hiding this comment

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

The type variable T is defined twice in this file. Please remove the duplicate definition.

@savarin
Copy link
Contributor

savarin commented Feb 15, 2024

@savarin the openai api call tests are all failing due to missing optionals.

Got it, will take a look.

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.

2 participants