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

Remove Async Validation in process async #933

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Conversation

ivanleomk
Copy link
Collaborator

@ivanleomk ivanleomk commented Aug 19, 2024

This removes the use of async validation in the process step


🚀 This description was created by Ellipsis for commit 57150fe

Summary:

Removed async validation logic and updated related imports and checks, including test file updates.

Key points:

  • Removed async validation logic from process_response_async in instructor/process_response.py.
  • Deleted AsyncValidationError import and usage.
  • Removed async validation check for OpenAISchema in process_response_async.
  • Removed warning about async validators in process_response.
  • Removed async_field_validator and async_model_validator from instructor/__init__.py.
  • Updated import path for async_field_validator and async_model_validator in tests/llm/test_openai/schema/test_async_decorator.py.

Generated with ❤️ by ellipsis.dev

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 everything up to 4948dd9 in 13 seconds

More details
  • Looked at 38 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/process_response.py:147
  • Draft comment:
    Consider updating documentation or adding comments to inform users that async validation is no longer performed in process_response_async and process_response. This will help users understand the change and adjust their expectations accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The removal of async validation checks in process_response_async and process_response is consistent with the PR description. However, the removal of the warning log in process_response might lead to confusion for users who expect async validation to occur. A comment or documentation update might be necessary to inform users of this change.

Workflow ID: wflow_s074dbhwUVrPzrov


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

instructor/process_response.py Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Aug 19, 2024

Deploying instructor with  Cloudflare Pages  Cloudflare Pages

Latest commit: afb1dbc
Status: ✅  Deploy successful!
Preview URL: https://19b19964.instructor.pages.dev
Branch Preview URL: https://remove-async-validation.instructor.pages.dev

View logs

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. Incremental review on 3cf6dc5 in 17 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/__init__.py:43
  • Draft comment:
    The removal of async_field_validator and async_model_validator from the __all__ list is appropriate since their import statements have been removed, aligning with the PR's purpose.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The removal of async_field_validator and async_model_validator from the __all__ list is consistent with the removal of their import statements, which is appropriate given the PR's purpose.

Workflow ID: wflow_mi5Saksj4muhKHnb


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

instructor/__init__.py Show resolved Hide resolved
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! Incremental review on 57150fe in 23 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/llm/test_openai/schema/test_async_decorator.py:8
  • Draft comment:
    The import of async_field_validator and async_model_validator from instructor.validators should be removed as these functions are no longer used in the codebase according to the PR description.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is incorrect because the functions async_field_validator and async_model_validator are actively used in the code. The suggestion to remove them is not valid, as it would break the code functionality.
    I might be missing some context from the PR description, but based on the code provided, the functions are clearly used. The comment seems to be based on an incorrect assumption.
    Even if the PR description suggests these functions are unused, the code itself shows they are used, which takes precedence in this review.
    The comment should be deleted because it incorrectly suggests removing imports that are actively used in the code.
2. tests/llm/test_openai/schema/test_async_decorator.py:8
  • Draft comment:
    Function names should follow a consistent naming pattern. Consider using snake_case for all function names in this file. This issue is also present in other functions like test_openai_schema_serialization, test_openai_schema_float_and_bool, etc.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function names in the test file are inconsistent. Some functions use underscores while others use camel case. This violates the consistent naming pattern rule.

Workflow ID: wflow_9zFyRLg4aAnV1o3Y


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

@ivanleomk ivanleomk closed this Aug 21, 2024
@ivanleomk ivanleomk reopened this Aug 21, 2024
@ivanleomk
Copy link
Collaborator Author

ivanleomk commented Aug 21, 2024

hey all, this is a new PR i'm introducing to remove the usage of the async_validators inside the existing codebase itself. The initial implementation that I put forth in instructor 1.3.6 and 1.3.7 introduced modifications to the OpenAISchema.

For the next release, I'm aiming to remove all existing usage of this modification so that we get a stable release to support the new structured output mode + other features in instructor. Once we've verified that the changes have been validated, we can then work towards reintroducing this in the form of a mixin in the upcoming subsequent releases.

As such, for now the async_field_validator and the async_model_validators are going to be temporarily removed in the 1.4.0 release.

cc @jxnl on the purpose of this PR

@ivanleomk ivanleomk merged commit b338582 into main Aug 22, 2024
14 of 15 checks passed
@ivanleomk ivanleomk deleted the remove-async-validation branch August 22, 2024 04:07
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