-
-
Notifications
You must be signed in to change notification settings - Fork 191
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: limit image size, fixes exception for large images (fixes #185) #188
Conversation
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.
❌ Changes requested. Reviewed everything up to 9a4d2f6 in 21 seconds
More details
- Looked at
60
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_4pA8j01BQW1yowxI
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.
# anthropic limit is 5MB | ||
# TODO: use compression to reduce file size | ||
# print(f"{len(data)=}") | ||
if len(data) > 5_000_000: |
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.
Checking the size of data
(base64 encoded) is incorrect for file size validation. Base64 encoding increases the size by approximately 33%. Instead, check len(data_bytes)
to ensure the original file size is within limits.
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.
@ellipsis-dev I did that originally, but it didn't match the numbers returned by the API (roughly the 33% difference), maybe add a comment about this.
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.
@ErikBjare, I have addressed your comments in pull request #189
You can configure Ellipsis to address comments with a direct commit or a side PR, see docs.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #188 +/- ##
==========================================
+ Coverage 80.39% 80.78% +0.39%
==========================================
Files 56 56
Lines 3305 3315 +10
==========================================
+ Hits 2657 2678 +21
+ Misses 648 637 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ixes exception when viewing large images (#185));
Important
Adds a 5MB image size limit and provider check in
_content_files_list()
inmessage.py
to prevent exceptions with large images._content_files_list()
inmessage.py
. If exceeded, appends a warning message to content.ValueError
if neitheropenai
noranthropic
is specified in_content_files_list()
.data
variable for reuse inmessage.py
.This description was created by for 9a4d2f6. It will automatically update as commits are pushed.