-
-
Notifications
You must be signed in to change notification settings - Fork 687
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
Migrating Anthropic #1281
Migrating Anthropic #1281
Conversation
Deploying instructor-py with Cloudflare Pages
|
There was a problem hiding this 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 b3aacfa in 1 minute and 8 seconds
More details
- Looked at
329
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. tests/llm/test_anthropic/test_multimodal.py:118
- Draft comment:
Theenable_prompt_caching
parameter has been removed from thefrom_anthropic
function, but it is still being used here. This will cause an error.
client = instructor.from_anthropic(client, mode=mode)
- Reason this comment was not posted:
Comment looked like it was already resolved.
2. tests/llm/test_anthropic/test_system.py:47
- Draft comment:
Theenable_prompt_caching
parameter has been removed from thefrom_anthropic
function, but it is still being used here. This will cause an error.
client = instructor.from_anthropic(client, mode=mode)
- Reason this comment was not posted:
Marked as duplicate.
3. tests/llm/test_anthropic/test_multimodal.py:12
- Draft comment:
The variableimage_url
is defined but not used consistently across the file. Ensure all instances of the hardcoded URL are replaced withimage_url
for consistency and to adhere to the DRY principle. - Reason this comment was not posted:
Confidence changes required:80%
The variableimage_url
is defined but not used consistently across the file. It should replace all instances of the hardcoded URL for consistency and DRY principle.
4. instructor/client_anthropic.py:42
- Draft comment:
The removal ofenable_prompt_caching
from the function signature and its related logic indicates a change in library code. Ensure that documentation and tests are updated accordingly to reflect this change. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a real API change - removing a parameter. The docstring is out of sync with the implementation. According to the rules, for library code changes we should check documentation and tests. This seems like a valid concern since the docstring needs to be updated. The comment is specific and actionable.
The comment could be too generic - maybe there are no tests that need updating. Also, the docstring update might be handled in another file or PR.
Even if tests don't need updating, the docstring definitely needs to be fixed in this file. The comment raises a valid documentation issue that needs to be addressed in this PR.
Keep the comment as it points out a real documentation inconsistency that needs to be fixed as part of this API change.
Workflow ID: wflow_bvXpcplZD1nNTlk1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 f5ecd88 in 19 seconds
More details
- Looked at
158
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. pyproject.toml:107
- Draft comment:
Consider moving the 'material' dependency to the 'dev' section if it's not specifically for documentation purposes. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'material' in 'pyproject.toml' and 'uv.lock' should be consistent with other dependencies. It should be added to the 'dev' section instead of 'docs' if it's not specifically for documentation purposes.
2. uv.lock:1665
- Draft comment:
Consider moving the 'material' dependency to the 'dev' section if it's not specifically for documentation purposes. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'material' in 'uv.lock' should be consistent with other dependencies. It should be added to the 'dev' section instead of 'docs' if it's not specifically for documentation purposes.
3. docs/concepts/prompt_caching.md:17
- Draft comment:
Make sure to updatemkdocs.yml
to include this newmd
file for proper documentation generation. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_QVINwPUf1DHT7JRF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
tests/llm/test_anthropic/evals/test_simple.py ...s..... [ 34%] |
This is a PR which targets issue #1253 by upgrading Anthropic's SDK to 0.42.0. In the process, we also remove the beta.prompt_caching since it's now generally available.
This should fix issue #1253
Important
Remove prompt caching and update Anthropic package to version 0.42.0, with test and formatting updates.
enable_prompt_caching
parameter fromfrom_anthropic()
inclient_anthropic.py
.PromptCachingBetaMessage
type checks inreask.py
.anthropic
package version to0.42.0
inpyproject.toml
anduv.lock
.test_multimodal.py
.enable_prompt_caching
from test cases intest_system.py
andtest_multimodal.py
.cache_read_input_tokens
andcache_creation_input_tokens
toAnthropicUsage
inretry.py
andutils.py
.retry.py
.This description was created by for f5ecd88. It will automatically update as commits are pushed.