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 Issues #966

Merged
merged 7 commits into from
Aug 31, 2024
Merged

Fix Issues #966

merged 7 commits into from
Aug 31, 2024

Conversation

ivanleomk
Copy link
Collaborator

@ivanleomk ivanleomk commented Aug 30, 2024

This fixes issues that were introduced with our formatting and linting pipeline with #947's recent merge into main.


🚀 This description was created by Ellipsis for commit 8ba8f79

Summary:

Fixes formatting and linting issues, simplifies test functions, and improves code readability without changing functionality.

Key points:

  • Fixes formatting and linting issues from Adding support for multi-modal file input for Vertex AI #947.
  • Simplifies test functions by removing unnecessary parameterization and unused parameters.
  • Simplifies test functions in tests/dsl/test_partial.py by removing unnecessary assertions.
  • Improves code readability in instructor/client_vertexai.py.
  • Adds # type: ignore comments to suppress type checking warnings.
  • Adjusts type hints and comments in vertexai_message_parser function.
  • No changes to logic or functionality.

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.

👍 Looks good to me! Reviewed everything up to d6306c6 in 9 seconds

More details
  • Looked at 44 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/llm/test_vertexai/test_message_parser.py:8
  • Draft comment:
    The removal of @pytest.mark.parametrize decorator is not explained. If the tests no longer require parameters, ensure this change is intentional and justified.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The removal of the @pytest.mark.parametrize decorator is not explained in the PR description. It seems like an intentional change, but it should be justified.
2. tests/llm/test_vertexai/test_message_parser.py:8
  • Draft comment:
    The removal of @pytest.mark.parametrize suggests a change in the test strategy. Ensure that documentation and test files are updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The removal of the @pytest.mark.parametrize decorator suggests a change in the test strategy, but there is no update to the documentation or test files to reflect this change.

Workflow ID: wflow_cuFvB7udFQLNZIZx


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

Copy link

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

Deploying instructor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 299b9ef
Status: ✅  Deploy successful!
Preview URL: https://d63154b5.instructor.pages.dev
Branch Preview URL: https://fix-vertexai-part-errors.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! Incremental review on 5bb5004 in 23 seconds

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

Workflow ID: wflow_sVGV9z68o2DkQB3v


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 82b35c2 in 27 seconds

More details
  • Looked at 30 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/client_vertexai.py:46
  • Draft comment:
    For compatibility with Python versions below 3.9, consider using List[gm.Part] instead of list[gm.Part]. This applies to other type hints in this file as well.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The type hint for the 'parts' list in the 'vertexai_message_parser' function is not compatible with Python versions below 3.9. The code should be compatible with earlier versions as well.

Workflow ID: wflow_gnRDXw4oDdVpbeHT


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 495a3fa in 21 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. instructor/client_vertexai.py:89
  • Draft comment:
    Avoid using # type: ignore without justification. If the type checker is raising an error, consider addressing the root cause or provide a comment explaining why it's safe to ignore.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of # type: ignore should be justified or removed if not necessary.
2. instructor/client_vertexai.py:103
  • Draft comment:
    Avoid using # type: ignore without justification. If the type checker is raising an error, consider addressing the root cause or provide a comment explaining why it's safe to ignore.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of # type: ignore should be justified or removed if not necessary.
3. instructor/client_vertexai.py:87
  • Draft comment:
    Assertions should have error messages. Please add descriptive error messages to the assertions in the from_vertexai function.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The assertions in the from_vertexai function lack error messages, which is against the rule that assertions should always have an error message that is formatted well.

Workflow ID: wflow_YvMMTePZdXLn0pmF


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 8ba8f79 in 33 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. tests/dsl/test_partial.py:122
  • Draft comment:
    Consider re-adding the assertion assert extraction.summary.startswith(previous_summary) to ensure that each new summary is a continuation of the previous one. This is important for validating the incremental nature of the summary updates.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The removal of the assertion assert extraction.summary.startswith(previous_summary) in the test_summary_extraction_async function might lead to a loss of validation for the incremental nature of the summary updates. This assertion was ensuring that each new summary is a continuation of the previous one, which is a valuable check for the functionality being tested.

Workflow ID: wflow_ybNu6dm9SjpdVmKu


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

@@ -95,7 +95,6 @@ class Summary(BaseModel):
updates = 0
for extraction in extraction_stream:
if previous_summary is not None and extraction:
assert extraction.summary.startswith(previous_summary)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this since we only update the field once with the new on parsing strategy in jiter to support literals

Copy link
Collaborator Author

@ivanleomk ivanleomk Aug 30, 2024

Choose a reason for hiding this comment

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

Relevant PR : #948

Adding new docs to showcase `gm.Part` integration
@ivanleomk ivanleomk merged commit c1bbfa5 into main Aug 31, 2024
14 of 15 checks passed
@ivanleomk ivanleomk deleted the fix-vertexai-part-errors branch August 31, 2024 01:54
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