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: removed use_async flag in cerebras client #1179

Merged
merged 7 commits into from
Nov 14, 2024
Merged

Conversation

ivanleomk
Copy link
Collaborator

@ivanleomk ivanleomk commented Nov 14, 2024

Important

Removed use_async flag from from_cerebras() in client_cerebras.py and updated cerebras_cloud_sdk version.

  • Function Signature:
    • Removed use_async flag from from_cerebras() in client_cerebras.py.
  • Logic:
    • Replaced use_async flag check with isinstance(client, AsyncCerebras) in from_cerebras().
  • Dependencies:
    • Updated cerebras_cloud_sdk version to ^1.11.0 in pyproject.toml.
  • Misc:
    • Added new files base.py, main.py, and output.wav.

This description was created by Ellipsis for 5302263. 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.

❌ Changes requested. Reviewed everything up to 1b747af in 20 seconds

More details
  • Looked at 37 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/client_cerebras.py:45
  • Draft comment:
    The removal of the use_async flag is appropriate since the type of client already determines whether the function should return an Instructor or AsyncInstructor.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the use_async flag is appropriate since the type of client already determines whether the function should return an Instructor or AsyncInstructor.

Workflow ID: wflow_fuoUKe8og5wpnThm


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/client_cerebras.py Show resolved Hide resolved
@github-actions github-actions bot added bug Something isn't working enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 14, 2024
Copy link

cloudflare-workers-and-pages bot commented Nov 14, 2024

Deploying instructor-py with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5302263
Status: ✅  Deploy successful!
Preview URL: https://7e42fb24.instructor-py.pages.dev
Branch Preview URL: https://migrate-cerebras-mode.instructor-py.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! Incremental review on 5cc7ed1 in 16 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/client_cerebras.py:3
  • Draft comment:
    The removal of Literal from the imports is correct since it is not used in the code.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The import of Literal was removed but not used in the code, so this change is correct.

Workflow ID: wflow_XSa4MnshchwTPeiR


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

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 1c988fb in 13 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pyproject.toml:41
  • Draft comment:
    Ensure that the updated cerebras_cloud_sdk version ^1.11.0 is compatible with the rest of the codebase and does not introduce any breaking changes.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR updates the version of cerebras_cloud_sdk from ^1.5.0 to ^1.11.0. This change is consistent across both the main dependencies and the extras section, which is good for consistency. However, it's important to ensure that the new version is compatible with the rest of the codebase and does not introduce any breaking changes.
2. pyproject.toml:41
  • Draft comment:
    Ensure that documentation and tests are updated to reflect the version change of cerebras_cloud_sdk from ^1.5.0 to ^1.11.0.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The version update of the cerebras_cloud_sdk library in pyproject.toml should be accompanied by updates in documentation and tests, as per the rules.

Workflow ID: wflow_C90zfYWgW9IaUk1O


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

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 0465c3e in 16 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pyproject.toml:20
  • Draft comment:
    If the cerebras_cloud_sdk library update involves any changes in functionality or usage, ensure that the documentation is updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The update to cerebras_cloud_sdk version in pyproject.toml should be accompanied by documentation updates if necessary.

Workflow ID: wflow_qcIdVR5XpLtFzu0x


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

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.

Skipped PR review on bcc491e because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

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 aca27b4 in 27 seconds

More details
  • Looked at 138 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. base.py:5
  • Draft comment:
    The environment variable GOOGLE_API_KEY should be OPENAI_API_KEY to match the expected variable for OpenAI API keys.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment points out a potential mismatch between the environment variable name and the library being used. This could be a valid issue if the API key is indeed for OpenAI and not Google. However, without more context on the environment setup or the intended use of the API key, it's speculative. The comment does not provide strong evidence that a change is required.
    I might be missing the context of the environment setup or the specific use case for the API key. The code could be intentionally using a Google API key for a specific reason not evident in the diff.
    Given the use of the OpenAI library, it is reasonable to expect the environment variable to match the library's naming conventions unless there is a specific reason otherwise.
    The comment highlights a potential issue with the environment variable name that could lead to runtime errors if incorrect. However, without strong evidence or additional context, it remains speculative.
2. main.py:9
  • Draft comment:
    The environment variable GOOGLE_API_KEY should be OPENAI_API_KEY to match the expected variable for OpenAI API keys.
  • Reason this comment was not posted:
    Marked as duplicate.
3. base.py:4
  • Draft comment:
    The client initialization is repeated in both base.py and main.py. Consider refactoring to avoid this repetition and adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The code in base.py and main.py is not DRY. The client initialization is repeated in both files, which violates the DRY principle.
4. main.py:7
  • Draft comment:
    The client initialization is repeated in both base.py and main.py. Consider refactoring to avoid this repetition and adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The code in base.py and main.py is not DRY. The client initialization is repeated in both files, which violates the DRY principle.

Workflow ID: wflow_I44JKPYxFSavqrSt


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

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 5302263 in 15 seconds

More details
  • Looked at 95 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_oa0dtVPdxvt5mKuU


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

@ivanleomk ivanleomk merged commit efabcf5 into main Nov 14, 2024
14 of 15 checks passed
@ivanleomk ivanleomk deleted the migrate-cerebras-mode branch November 14, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant