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 AttributeError with missing usage fields in AnthropicBedrock #1293

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emcf
Copy link

@emcf emcf commented Jan 3, 2025

Fixes #1292


Important

Fix AttributeError in AnthropicBedrock usage update and unskip related tests.

  • Behavior:
    • Fix AttributeError in update_total_usage() in utils.py by checking for field existence before updating input_tokens, output_tokens, cache_creation_input_tokens, and cache_read_input_tokens.
  • Tests:
    • Unskip test_client_anthropic_bedrock_response() and test_async_client_anthropic_bedrock_response() in test_new_client.py.

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

👍 Looks good to me! Reviewed everything up to 12135f6 in 13 seconds

More details
  • Looked at 75 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. instructor/utils.py:167
  • Draft comment:
    The PR resolves the issue by checking for the presence of fields before accessing them, which prevents the AttributeError. The changes in the test file ensure that the tests for the AnthropicBedrock client are now active and will run, verifying the fix.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR resolves the issue by checking for the presence of fields before accessing them, which prevents the AttributeError. The changes in the test file ensure that the tests for the AnthropicBedrock client are now active and will run, verifying the fix.
2. tests/llm/test_new_client.py:183
  • Draft comment:
    The unskipping of test_client_anthropic_bedrock_response and test_async_client_anthropic_bedrock_response tests is appropriate as it ensures the new behavior of handling missing fields in AnthropicUsage is verified.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The PR modifies the update_total_usage function to handle missing fields in AnthropicUsage. This change should be accompanied by updated tests to ensure the new behavior is verified. The PR does update tests by unskipping the test_client_anthropic_bedrock_response and test_async_client_anthropic_bedrock_response tests, which is appropriate.

Workflow ID: wflow_mKUJcAyovRdQcvTd


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

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.

Fix AttributeError with missing usage fields in AnthropicBedrock
1 participant