-
Notifications
You must be signed in to change notification settings - Fork 904
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
F/file routes: Fix failing file routes #990
Conversation
Signed-off-by: Diwank Singh Tomer <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
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 7913187 in 39 seconds
More details
- Looked at
630
lines of code in13
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/tests/test_files_routes.py:54
- Draft comment:
Uncomment the test for the GET file route to ensure it's executed.
response = make_request(
method="GET",
url=f"/files/{file_id}",
)
assert response.status_code == 404
- Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_HiNhTyxl2uNo5Ypt
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.
result = await client.get_object(Bucket=async_s3.blob_store_bucket, Key=key) | ||
content = await result["Body"].read() | ||
|
||
print("-" * 100) |
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.
Remove print statements used for debugging.
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Failure Feedback 🧐
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
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 32dfd5f in 20 seconds
More details
- Looked at
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/routers/files/get_file.py:20
- Draft comment:
Consider implementing streaming for large payloads to avoid loading entire files into memory, which can lead to performance issues. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Q32KbcXVIfplh21h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
PR Type
Enhancement
Description
Changes walkthrough 📝
app.py
Add S3 client initialization and cleanup in app lifecycle
agents-api/agents_api/app.py
async_s3.py
Refactor S3 client to use singleton pattern
agents-api/agents_api/clients/async_s3.py
*.py
Update file routes to use shared S3 client
agents-api/agents_api/routers/files/*.py
fixtures.py
Replace mock S3 client with localstack in tests
agents-api/tests/fixtures.py
Important
Refactor S3 client management using a singleton pattern and enhance testing with localstack.
app.py
andasync_s3.py
.app.py
lifecycle.async_s3.py
to use shared S3 client, removing redundant client creation.routers/files/*.py
to use shared S3 client.fixtures.py
for realistic testing.pyproject.toml
to include localstack support.This description was created by for 32dfd5f. It will automatically update as commits are pushed.