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

feat(instructor): introduce new client with support for different providers and sync/async operations #546

Merged
merged 31 commits into from
Apr 1, 2024

Conversation

jxnl
Copy link
Owner

@jxnl jxnl commented Mar 29, 2024

Ellipsis 🚀 This PR description was created by Ellipsis for commit 7b55741.

Summary:

This PR enhances the instructor module, introduces new protocols and function overloading, updates classes and functions, adds a new test file, updates the documentation, adds a new blog post, removes instructor.Mode.MD_JSON from modes list, adds new dependencies to poetry.lock, and updates test configurations.

Key points:

  • Enhanced instructor module with support for different providers and sync/async operations.
  • Introduced new protocols and function overloading in instructor/patch.py.
  • Updated Provider enum in utils.py and Instructor and AsyncInstructor classes in client.py.
  • Added new test file tests/test_new_client.py.
  • Updated documentation and added a new blog post in docs/blog/posts/version-1.md.
  • Changed client parameter type in llm_validator function in instructor/dsl/validators.py.
  • Removed instructor.Mode.MD_JSON from modes list in tests/llm/test_openai/util.py.
  • Added new dependencies to poetry.lock.
  • Updated .github/workflows/test.yml to exclude 'docs' from the test run.
  • Updated tests/llm/test_openai/docs/test_hub.py to include 'contact' in the list of excluded sources for testing.

Generated with ❤️ by ellipsis.dev

@jxnl jxnl marked this pull request as ready for review March 29, 2024 14:43
@jxnl jxnl requested review from lukevs and shanktt March 29, 2024 14:44
@ellipsis-dev ellipsis-dev bot changed the title ... feat(instructor): introduce new client with support for different providers and sync/async operations Mar 29, 2024
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 the entire pull request up to 17ee49a
  • Looked at 536 lines of code in 3 files
  • Took 1 minute and 13 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:30:
  • Assessed confidence : 50%
  • Comment:
    The 'ANYSCALE' and 'TOGETHER' providers are defined in the Provider Enum but are not used anywhere in the code. If these are not intended to be used, consider removing them to avoid confusion.
  • Reasoning:
    The new client.py file introduces a new class 'Instructor' and 'AsyncInstructor' which seem to be wrappers around the OpenAI and Anthropic clients. The classes provide methods for creating, creating partial, creating iterable, and creating with response. The methods seem to be handling the kwargs properly and the assertions for the provider not supporting certain features are in place. The 'from_openai' and 'from_anthropic' functions are overloaded to handle both synchronous and asynchronous clients. The test cases in test_new_client.py seem to be covering all the new methods introduced in the Instructor classes. However, I noticed that the 'ANYSCALE' and 'TOGETHER' providers are defined in the Provider Enum but are not used anywhere in the code. This could potentially be a future plan or an oversight.

Workflow ID: wflow_u8sAUQs1gVUFX2eF


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link

cloudflare-pages bot commented Mar 29, 2024

Deploying instructor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7b55741
Status: ✅  Deploy successful!
Preview URL: https://931e05b5.instructor.pages.dev
Branch Preview URL: https://v1-attempt2.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.

👍 Looks good to me!

  • Performed an incremental review on e8798b3
  • Looked at 14 lines of code in 1 files
  • Took 2 minutes and 27 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:15:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    The Instructor and AsyncInstructor classes have a lot of repeated code. The methods create, create_partial, create_iterable, and create_with_response are almost identical in both classes. This violates the DRY (Don't Repeat Yourself) principle. We should consider refactoring this to avoid code duplication.
  • Reasoning:
    The Instructor and AsyncInstructor classes have a lot of repeated code. The methods create, create_partial, create_iterable, and create_with_response are almost identical in both classes. This violates the DRY (Don't Repeat Yourself) principle. We should consider refactoring this to avoid code duplication.

Workflow ID: wflow_uy3pzmDr2DCjnIOs


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on c54bf2b
  • Looked at 132 lines of code in 1 files
  • Took 1 minute and 32 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:68:
  • Assessed confidence : 80%
  • Comment:
    The removal of *args from the method signatures could potentially break existing code that uses positional arguments. Please ensure that these methods are not called with positional arguments anywhere in the codebase.
  • Reasoning:
    The PR removed the *args parameter from several methods in the Instructor and AsyncInstructor classes. This could potentially break existing code that uses positional arguments when calling these methods. However, it's not clear from the PR description why this change was made. I'll need to check the rest of the codebase to see if these methods are called with positional arguments anywhere.

Workflow ID: wflow_P5UiIUtbMzdVUWaY


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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.

  • Performed an incremental review on 79d265a
  • Looked at 144 lines of code in 2 files
  • Took 2 minutes and 51 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 0 additional comments because they didn't meet confidence threshold of 85%.

Workflow ID: wflow_HlPA1ijP3KJjfoy3


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.



class Instructor:
client: openai.OpenAI | anthropic.Anthropic | None
Copy link
Contributor

Choose a reason for hiding this comment

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

The client parameter is now optional and can be None. Please ensure that all usages of client in the Instructor and AsyncInstructor classes are properly checked to avoid NoneType errors.

**kwargs,
) -> Instructor | AsyncInstructor:

is_async = inspect.isawaitable(completion)
Copy link
Contributor

Choose a reason for hiding this comment

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

The inspect.isawaitable function checks if an object is an instance of an awaitable class, not if a function returns an awaitable object. This could lead to incorrect behavior if a synchronous function that returns an awaitable object is passed as the completion parameter. Consider using inspect.iscoroutinefunction instead.

@@ -59,6 +71,13 @@ def patch(
) -> InstructorChatCompletionCreate: ...


@overload
Copy link
Contributor

Choose a reason for hiding this comment

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

The overload of the patch function that takes an Awaitable as the create parameter returns InstructorChatCompletionCreate, which is a protocol for synchronous functions. This could lead to type errors if an asynchronous function is passed as the create parameter. Consider returning AsyncInstructorChatCompletionCreate instead.

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.

  • Performed an incremental review on 66c53e8
  • Looked at 89 lines of code in 1 files
  • Took 1 minute and 55 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:163:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    The provider attribute is not defined in the AsyncInstructor class and is not passed as an argument to its constructor. This could lead to an AttributeError when the create_partial and create_iterable methods are called on an instance of AsyncInstructor. Consider adding a provider attribute to the AsyncInstructor class and passing it as an argument to its constructor.
  • Reasoning:
    The create_partial and create_iterable methods in both Instructor and AsyncInstructor classes have assertions that check if the provider is not Anthropic. However, the provider attribute is not defined in the AsyncInstructor class and is not passed as an argument to its constructor. This could lead to an AttributeError when these methods are called on an instance of AsyncInstructor.

Workflow ID: wflow_jwzh2EF0J5FYysis


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.


def from_anthropic(
client: anthropic.Anthropic | anthropic.AsyncAnthropic,
mode: instructor.Mode = instructor.Mode.ANTHROPIC_JSON,
Copy link
Contributor

Choose a reason for hiding this comment

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

The from_anthropic function has two overloads but the actual function implementation does not match the overloads. The overloads suggest that the mode parameter can have different default values depending on the type of the client parameter. However, in the actual function implementation, the mode parameter has a single default value regardless of the client type. Consider updating the function implementation to match the overloads.

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!

  • Performed an incremental review on f281a64
  • Looked at 28 lines of code in 1 files
  • Took 1 minute and 52 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:91:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    The PR introduces significant changes to the instructor module, including new classes and methods. However, the PR description does not mention any updates to the documentation or tests to reflect these changes. Please ensure that the documentation and tests are updated accordingly.
  • Reasoning:
    The PR introduces new classes and methods in the instructor module, removes the *args parameter from several methods, includes a new test file for validation, introduces a new protocol, and overloads a function to support Awaitable. The changes seem to be well implemented and there are no clear violations of best practices, logical bugs, performance bugs, or security bugs. However, the PR description does not mention any updates to the documentation or tests to reflect these changes. I will comment on this.

Workflow ID: wflow_QsUMqfbhJ283Oad9


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on 4d45d7e
  • Looked at 80 lines of code in 1 files
  • Took 4 minutes and 2 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. README.md:39:
  • Assessed confidence : 0%
  • Comment:
    The changes in the README.md file are correct and consistent with the changes made in the code. The examples provided are clear and easy to understand.
  • Reasoning:
    The README.md file has been updated to reflect the changes in the code. The examples provided in the README.md file are consistent with the changes made in the code. The examples are clear and easy to understand. The changes made in the README.md file are correct and do not require any modifications.

Workflow ID: wflow_X4aFlhhUqVsHncaQ


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

instructor/client.py Outdated Show resolved Hide resolved
ellipsis-dev bot added a commit that referenced this pull request Mar 30, 2024
…e new client with support for different providers and sync/async operations);
…547)

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@jxnl
Copy link
Owner Author

jxnl commented Mar 30, 2024

@ellipsis_dev can you update everything in /docs to use instructor.from_openai instead of instructor.patch

Copy link
Contributor

ellipsis-dev bot commented Mar 30, 2024

I was in the process of checking all the .md files in the /docs directory and its subdirectories for instructor.patch and replacing it with instructor.from_openai. I managed to replace instructor.patch in the files /docs/api.md, /docs/index.md, and /docs/why.md. However, I didn't have time to check the rest of the files. Please let me know if you would like me to continue this task.


Responding to this comment by @jxnl. For more information about Ellipsis, check the documentation.

ellipsis-dev bot added a commit that referenced this pull request Mar 30, 2024
…ew client with support for different providers and sync/async operations);
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!

  • Performed an incremental review on 73ab9b5
  • Looked at 1120 lines of code in 67 files
  • Took 1 minute and 51 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 3 additional comments because they didn't meet confidence threshold of 85%.
1. examples/avail/run_mixtral.py:71:
  • Assessed confidence : 80%
  • Comment:
    This function used to return an Iterable[AvailabilityResponse]. The change to return an iterable might cause issues if the function's return value is expected to be an iterable of AvailabilityResponse objects elsewhere in the code. Consider reverting this change.
 def parse_availability(text: str) -> Iterable[AvailabilityResponse]:
  • Reasoning:
    The function parse_availability in run_mixtral.py has been changed to return an iterable instead of an Iterable[AvailabilityResponse]. This change might cause issues if the function's return value is expected to be an iterable of AvailabilityResponse objects elsewhere in the code. I'll suggest reverting this change.
2. examples/batch-classification/run_langsmith.py:61:
  • Assessed confidence : 80%
  • Comment:
    This function used to return a QuestionClassification. The change to return a None type might cause issues if the function's return value is expected to be a QuestionClassification object elsewhere in the code. Consider reverting this change.
 async def classify(data: str) -> QuestionClassification:
  • Reasoning:
    The function classify in run_langsmith.py has been changed to return a None type instead of a QuestionClassification. This change might cause issues if the function's return value is expected to be a QuestionClassification object elsewhere in the code. I'll suggest reverting this change.
3. examples/vision/run_table.py:59:
  • Assessed confidence : 80%
  • Comment:
    This function used to return an Iterable[Table]. The change to return an iterable might cause issues if the function's return value is expected to be an iterable of Table objects elsewhere in the code. Consider reverting this change.
 def extract_table(url: str) -> Iterable[Table]:
  • Reasoning:
    The function extract_table in run_table.py has been changed to return an iterable instead of an Iterable[Table]. This change might cause issues if the function's return value is expected to be an iterable of Table objects elsewhere in the code. I'll suggest reverting this change.

Workflow ID: wflow_8MVyD8po5wYS0mKV


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on 9b49f97
  • Looked at 24 lines of code in 2 files
  • Took 1 minute and 2 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 0 additional comments because they didn't meet confidence threshold of 85%.

Workflow ID: wflow_IZaRFEloJeT063Mj


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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.

  • Performed an incremental review on 73f53c6
  • Looked at 87 lines of code in 2 files
  • Took 1 minute and 48 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 3 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/dsl/validators.py:6:
  • Assessed confidence : 0%
  • Comment:
    Good job updating the import statement for the Instructor class to import from the correct location.
  • Reasoning:
    The import statement for the Instructor class has been updated to import from the correct location. This is a good change as it fixes a potential ImportError.
2. instructor/client.py:24:
  • Assessed confidence : 0%
  • Comment:
    Good job updating the import statement for the Partial class to import from the correct location.
  • Reasoning:
    The import statement for the Partial class has been updated to import from the correct location. This is a good change as it fixes a potential ImportError.
3. instructor/client.py:5:
  • Assessed confidence : 0%
  • Comment:
    Good job updating the import statement for the Provider enum and get_provider function to import from the correct location.
  • Reasoning:
    The import statement for the Provider enum and get_provider function has been updated to import from the correct location. This is a good change as it fixes a potential ImportError.

Workflow ID: wflow_2gnt5GoIvR9g0i55


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

mode: instructor.Mode = instructor.Mode.ANTHROPIC_JSON,
**kwargs,
) -> Instructor | AsyncInstructor:
assert mode in {
Copy link
Contributor

Choose a reason for hiding this comment

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

The assertion error message is not formatted correctly. The variables instructor.Mode.ANTHROPIC_JSON and instructor.Mode.ANTHROPIC_TOOLS are not being interpolated into the string. Consider using an f-string to format the error message correctly.

Suggested change
assert mode in {
assert mode in {instructor.Mode.ANTHROPIC_JSON, instructor.Mode.ANTHROPIC_TOOLS}, f"Mode must be one of {{instructor.Mode.ANTHROPIC_JSON}}, {{instructor.Mode.ANTHROPIC_TOOLS}}"

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.

  • Performed an incremental review on 9a3653f
  • Looked at 40 lines of code in 1 files
  • Took 1 minute and 31 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 0 additional comments because they didn't meet confidence threshold of 85%.

Workflow ID: wflow_c0T38i9OAN7CBlgP


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

def test_client_chat_completions_create_with_response():
client = instructor.from_openai(openai.OpenAI(), model="gpt-3.5-turbo")

user, completion = client.chat.completions.create_with_completion(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name in the test case does not match the function being called. Consider renaming the test case to 'test_client_chat_completions_create_with_completion' to match the function being called.

Suggested change
user, completion = client.chat.completions.create_with_completion(
def test_client_chat_completions_create_with_completion():

client = openai.AsyncOpenAI()
instructor_client = instructor.from_openai(client, model="gpt-3.5-turbo")

user, response = await instructor_client.chat.completions.create_with_completion(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name in the test case does not match the function being called. Consider renaming the test case to 'test_async_client_chat_completions_create_with_completion' to match the function being called.

Suggested change
user, response = await instructor_client.chat.completions.create_with_completion(
async def test_async_client_chat_completions_create_with_completion():

model="claude-3-haiku-20240307",
)

user, response = client.messages.create_with_completion(
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name in the test case does not match the function being called. Consider renaming the test case to 'test_client_from_anthropic_with_completion' to match the function being called.

Suggested change
user, response = client.messages.create_with_completion(
def test_client_from_anthropic_with_completion():

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!

  • Performed an incremental review on 6e355be
  • Looked at 43 lines of code in 1 files
  • Took 1 minute and 27 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. /tests/llm/test_openai/test_validators.py:25:
  • Assessed confidence : 50%
  • Comment:
    The changes in this test file seem to reflect the updates to the client correctly. The client is now created using the from_openai function, which is expected to handle different providers and modes. The llm_validator function now takes the new client as a parameter instead of the openai_client. This seems to be a logical change given the updates to the client. However, I would need to check the implementation of these functions to confirm that they are correctly handling the new client.
  • Reasoning:
    The PR introduces a new client with support for different providers and sync/async operations. The changes in the test file seem to reflect these changes correctly. The client is now created using the from_openai function, which is expected to handle different providers and modes. The llm_validator function now takes the new client as a parameter instead of the openai_client. This seems to be a logical change given the updates to the client. However, I would need to check the implementation of these functions to confirm that they are correctly handling the new client.

Workflow ID: wflow_tA1SSpLfwoZZUlUg


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on f3d5cc4
  • Looked at 23 lines of code in 1 files
  • Took 4 minutes and 22 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/dsl/validators.py:32:
  • Assessed confidence : 50%
  • Comment:
    The client parameter type has been changed from OpenAI to Instructor. Please ensure that the Instructor class has all the necessary methods and attributes that were previously available in the OpenAI class.
  • Reasoning:
    The llm_validator function has been updated to use the Instructor class instead of OpenAI. This is a significant change as it means the function will now be using the new Instructor client which supports different providers and sync/async operations. This change seems to be in line with the PR's intent of enhancing the instructor module with support for different providers and sync/async operations. However, I need to check if the Instructor class has all the necessary methods and attributes that were previously available in the OpenAI class.

Workflow ID: wflow_TbKodKMwQDs9KazD


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on 339a7ab
  • Looked at 234 lines of code in 10 files
  • Took 1 minute and 8 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. instructor/client.py:291:
  • Assessed confidence : 50%
  • Comment:
    Please ensure that the new md file is added to mkdocs.yml and that all assertions have well-formatted error messages.
  • Reasoning:
    The PR introduces a new client with support for different providers and sync/async operations. It also updates the instructor module, introduces new protocols, and overloads functions to support Awaitable. The Provider enum and Instructor classes are updated to handle different providers. A new test file is added for validation. The documentation is updated with examples of using the new classes with different providers. The client parameter type in the llm_validator function is changed from OpenAI to Instructor. The PR seems to follow best practices and doesn't appear to introduce any new bugs. However, I'll need to check if the new md file is added to mkdocs.yml and if the assertions have well-formatted error messages.

Workflow ID: wflow_D8iE1X4cNf6Latwu


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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.

  • Performed an incremental review on 9f1b26d
  • Looked at 265 lines of code in 1 files
  • Took 1 minute and 7 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 0 additional comments because they didn't meet confidence threshold of 85%.

Workflow ID: wflow_Pdl8LgcfzCUTWa4l


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

docs/blog/posts/version-1.md 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!

  • Performed an incremental review on 3b2949b
  • Looked at 14 lines of code in 1 files
  • Took 3 minutes and 0 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. pyproject.toml:57:
  • Assessed confidence : 50%
  • Comment:
    New dependencies pydantic_extra_types, litellm, and anthropic have been added. Please ensure that these are being used in the code and are necessary. Also, check if these dependencies have any known security vulnerabilities.
  • Reasoning:
    The PR introduces new dependencies in the pyproject.toml file. It's important to ensure that these new dependencies are actually used in the code and are necessary. Also, it's crucial to check if these new dependencies have any known security vulnerabilities.

Workflow ID: wflow_7fTwhnCEwdfFxhnt


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on 6fc4d92
  • Looked at 10 lines of code in 1 files
  • Took 2 minutes and 11 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. /tests/llm/test_openai/util.py:6:
  • Assessed confidence : 80%
  • Comment:
    It seems like instructor.Mode.MD_JSON has been removed from the modes list. This mode might still be necessary for testing the instructor module's functionality. Consider adding it back unless there's a specific reason for its removal.
modes = [
    instructor.Mode.TOOLS,
    instructor.Mode.MD_JSON,
]
  • Reasoning:
    The PR description mentions that the instructor module has been enhanced with support for different providers and sync/async operations. However, in the diff, I see that the instructor.Mode.MD_JSON has been removed from the modes list in the tests/llm/test_openai/util.py file. This could potentially lead to incomplete testing of the instructor module's functionality. I need to suggest adding it back.

Workflow ID: wflow_y4rBmY6McAn2ahKG


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on de8a3ea
  • Looked at 455 lines of code in 1 files
  • Took 10 minutes and 32 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 85%.
1. poetry.lock:1397:
  • Assessed confidence : 50%
  • Comment:
    This PR introduces a new dependency 'litellm'. Please ensure that this library is necessary and well-maintained. Also, check if the version used is the latest stable version.
  • Reasoning:
    The PR introduces a new dependency 'litellm'. I need to check if this library is necessary and if it's a well-maintained and trusted library. Also, I need to check if the version used is the latest stable version.

Workflow ID: wflow_qkN77Amc01rMLnFZ


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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!

  • Performed an incremental review on 57c98d5
  • Looked at 35 lines of code in 2 files
  • Took 2 minutes and 18 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 2 additional comments because they didn't meet confidence threshold of 85%.
1. .github/workflows/test.yml:39:
  • Assessed confidence : 50%
  • Comment:
    The 'docs' are excluded from the tests and coverage report. If there are any tests in the 'docs' directory, they will be skipped. Please confirm if this is the intended behavior.
  • Reasoning:
    The changes in the test.yml file are excluding the 'docs' from the tests and coverage report. This might be intentional if the 'docs' do not contain any testable code. However, it's worth noting that if there are any tests in the 'docs' directory, they will be skipped. The changes in the test_hub.py file are adding 'contact' to the list of excluded sources. This seems fine if 'contact' is indeed a source that is not supported in testing.
2. tests/llm/test_openai/docs/test_hub.py:13:
  • Assessed confidence : 50%
  • Comment:
    The 'contact' source is added to the excluded sources. Any tests related to the 'contact' source will be skipped. Please confirm if this is the intended behavior.
  • Reasoning:
    The 'contact' source is added to the list of excluded sources in the test_hub.py file. This seems fine if 'contact' is indeed a source that is not supported in testing. However, it's worth noting that any tests related to the 'contact' source will be skipped.

Workflow ID: wflow_a8sMmmq8u7lUn1pC


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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.

  • Performed an incremental review on 7b55741
  • Looked at 13 lines of code in 1 files
  • Took 3 minutes and 16 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 0 additional comments because they didn't meet confidence threshold of 85%.

Workflow ID: wflow_0n56XRiSz8Zultdi


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

@@ -36,15 +36,15 @@ jobs:
run: poetry install --with dev,anthropic

- name: Run tests
run: poetry run pytest tests/ -k "not openai and not anthropic and not evals"
run: poetry run pytest tests/ -k "not openai and not anthropic and not evals and not docs"
Copy link
Contributor

Choose a reason for hiding this comment

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

The test exclusion pattern in the 'Generate coverage report' step should also be updated to exclude 'docs' for consistency with the 'Run tests' step. Please ensure this change is made to maintain consistency in test results and coverage reports.

@jxnl jxnl merged commit d7378ba into main Apr 1, 2024
7 of 12 checks passed
@jxnl jxnl deleted the v1-attempt2 branch April 1, 2024 14:42
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.

None yet

3 participants