-
Notifications
You must be signed in to change notification settings - Fork 930
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
feat(agents-api,typespec): Add file storage api #847
Conversation
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
@Vedantsahai18 inside |
Signed-off-by: Diwank Singh Tomer <[email protected]>
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 271076b in 1 minute and 23 seconds
More details
- Looked at
1541
lines of code in35
files - Skipped
1
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/base_evaluate.py:12
- Draft comment:
Consider revisiting the use of# noqa: E402
for import statements. Ensure that imports are placed at the top of the file unless there's a specific reason for local imports. - Reason this comment was not posted:
Confidence changes required:50%
The import statements in thebase_evaluate.py
file have been modified to include# noqa: E402
comments. This is generally used to suppress linting errors related to import order. However, it's important to ensure that this is necessary and not just a way to bypass proper import ordering. The imports should be at the top unless there's a specific reason not to.
2. agents-api/agents_api/models/chat/gather_messages.py:95
- Draft comment:
Thequery_text
variable is assigned but not used. If it's not needed, consider removing it to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. agents-api/agents_api/models/files/create_file.py:108
- Draft comment:
Ensure that excludingcontent
fromfile_data
inmodel_dump
is intentional and documented, as the content is handled separately. - Reason this comment was not posted:
Confidence changes required:30%
In thecreate_file
function, thecontent
field is being excluded fromfile_data
when callingmodel_dump
. This is correct since the content is handled separately, but it's important to ensure that this exclusion is intentional and documented.
4. agents-api/agents_api/models/files/create_file.py:78
- Draft comment:
Thecontent
field is set to a dummy value. Ensure this is clearly documented, as the actual content is stored in blob storage. - Reason this comment was not posted:
Confidence changes required:30%
In thecreate_file
function, thecontent
field is being set to a dummy value. This should be clearly documented to avoid confusion, as the actual content is stored in blob storage.
5. agents-api/agents_api/models/files/get_file.py:36
- Draft comment:
Thecontent
field is set to a dummy value. Ensure this is clearly documented, as the actual content is fetched from blob storage. - Reason this comment was not posted:
Confidence changes required:30%
In theget_file
function, thecontent
field is set to a dummy value. This should be clearly documented to avoid confusion, as the actual content is fetched from blob storage.
6. agents-api/agents_api/routers/files/create_file.py:20
- Draft comment:
Ensure that thefile_id
used as a key for blob storage is unique and properly managed to avoid conflicts. - Reason this comment was not posted:
Confidence changes required:30%
In thecreate_file
function, thefile_id
is used as a key for blob storage. It's important to ensure that this key is unique and properly managed to avoid conflicts.
7. agents-api/agents_api/routers/files/delete_file.py:15
- Draft comment:
Ensure that thefile_id
used as a key for blob storage is properly managed to avoid accidental deletions. - Reason this comment was not posted:
Confidence changes required:30%
In thedelete_file
function, thefile_id
is used as a key for blob storage. It's important to ensure that this key is properly managed to avoid accidental deletions.
8. agents-api/agents_api/routers/files/get_file.py:14
- Draft comment:
Ensure that thefile_id
used as a key for blob storage is properly managed to avoid fetching incorrect data. - Reason this comment was not posted:
Confidence changes required:30%
In thefetch_file_content
function, thefile_id
is used as a key for blob storage. It's important to ensure that this key is properly managed to avoid fetching incorrect data.
9. agents-api/agents_api/autogen/Files.py:12
-
Draft comment:
TheCreateFileRequest
class is a duplicate of an existing class inintegrations-service/integrations/autogen/Files.py
. Consider using the existing class to avoid redundancy. -
class CreateFileRequest (Files.py)
-
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential redundancy, which could be a valid concern if the classes are indeed duplicates. However, without seeing the full content of the class in the other file, it's speculative to assume they are duplicates based solely on the class name. The comment lacks strong evidence as it doesn't provide a comparison of the class contents.
I might be missing the fact that class names alone could be a strong indicator of duplication, especially in autogenerated files. However, without content comparison, it's hard to be certain.
Class names alone are not sufficient evidence of duplication, especially in large codebases where similar names might be used in different contexts. More evidence is needed to confirm redundancy.
The comment should be deleted as it lacks strong evidence of duplication and is speculative without a content comparison of the classes.
10. agents-api/agents_api/autogen/Files.py:44
-
Draft comment:
TheFile
class is a duplicate of an existing class inintegrations-service/integrations/autogen/Files.py
. Consider reusing the existing class to avoid duplication. -
class File (Files.py)
-
Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_zh4m7mik7KmbR3hs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add file storage API to agents-api and typespec, including models, endpoints, and tests for file operations.
agents-api
andtypespec
.File
andCreateFileRequest
models inagents-api/autogen/Files.py
andtypespec/files/models.tsp
.FilesRoute
intypespec/main.tsp
andtypespec/files/endpoints.tsp
for file operations.files_router
inagents-api/routers/__init__.py
andagents-api/routers/files
for handling file routes.files
relation inmigrations/migrate_1731953383_create_files_relation.py
.tests/test_files_queries.py
andtests/test_files_routes.py
.This description was created by
for 271076b. It will automatically update as commits are pushed.