feat(agents-api): Add async boto client #911
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to b82d159 in 1 minute and 17 seconds
More details
- Looked at
1495lines of code in35files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. integrations-service/gunicorn_conf.py:40
- Draft comment:
Duplicate import ofosand redundantTESTINGandDEBUGvariable definitions. Remove the duplicate import and redundant variable definitions. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. integrations-service/integrations/utils/integrations/browserbase.py:25
- Draft comment:
Redundant comment:# Import env to access environment variablesis repeated. Remove the duplicate comment. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces async operations for S3 interactions, which is a good improvement. However, there are some redundant imports and duplicate code blocks that need to be addressed.
3. integrations-service/integrations/utils/integrations/llama_parse.py:10
- Draft comment:
Redundant comment:# Import env to access environment variablesis repeated. Remove the duplicate comment. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces async operations for S3 interactions, which is a good improvement. However, there are some redundant imports and duplicate code blocks that need to be addressed.
4. integrations-service/integrations/utils/integrations/spider.py:11
- Draft comment:
Redundant comment:# Import env to access environment variablesis repeated. Remove the duplicate comment. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces async operations for S3 interactions, which is a good improvement. However, there are some redundant imports and duplicate code blocks that need to be addressed.
Workflow ID: wflow_gvwgEBvR1huKJI6C
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <[email protected]>
whiterabbit1983
approved these changes
Nov 29, 2024
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f6bf839 in 1 minute and 7 seconds
More details
- Looked at
575lines of code in8files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/remote.py:3
- Draft comment:
TheRemoteListclass has been removed, but there are still references to it in the codebase. Ensure all instances ofRemoteListare replaced with standard Python lists and that the new implementation handles all previous use cases correctly. - Reason this comment was not posted:
Comment did not seem useful.
2. agents-api/agents_api/common/storage_handler.py:192
- Draft comment:
Ensureunload_return_valueis awaited inasync_wrapperto handle asynchronous operations correctly. - Reason this comment was not posted:
Comment did not seem useful.
3. agents-api/pyproject.toml:55
- Draft comment:
Ensure all functionalities previously dependent onboto3are now correctly handled byaiobotocore. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_YHDHl9UKdnMRmelU
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.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 14e4c3c in 1 minute and 12 seconds
More details
- Looked at
221lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/gunicorn_conf.py:4
- Draft comment:
Consider using theenvironslibrary for consistency in environment variable management.
from environs import Env
env = Env()
debug = env.bool("AGENTS_API_DEBUG", default=False)
- Reason this comment was not posted:
Confidence changes required:50%
The change ingunicorn_conf.pyto set workers to 1 in debug mode is a good practice for development environments. However, the use ofos.getenvdirectly here is inconsistent with the rest of the codebase, which uses theenvironslibrary for environment variable management. This inconsistency can lead to confusion and potential errors if the environment variable management strategy changes in the future.
2. agents-api/agents_api/common/storage_handler.py:115
- Draft comment:
Remove the redundantawaitinsideasyncio.gather.
load_from_blob_store_if_remote(item)
- Reason this comment was not posted:
Comment looked like it was already resolved.
3. agents-api/agents_api/env.py:36
- Draft comment:
Consider uncommenting and using environment variables foruse_blob_store_for_temporalandblob_store_cutoff_kbto allow configurability.
use_blob_store_for_temporal: bool = env.bool("USE_BLOB_STORE_FOR_TEMPORAL", default=False) if not testing else False
blob_store_cutoff_kb: int = env.int("BLOB_STORE_CUTOFF_KB", default=64)
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The author has intentionally commented out the environment variable usage, which suggests they have a reason for using fixed values instead. The comment is suggesting a change that the author might have already considered and decided against. Without strong evidence that the change is necessary, the comment seems speculative.
I might be missing the context of why the author commented out the environment variable usage. There could be a specific reason for this change that the comment does not address.
The comment does not provide strong evidence or reasoning for why the environment variables should be used instead of the current implementation. It seems speculative and not actionable without further context.
The comment should be deleted as it suggests a speculative change without strong evidence or reasoning. The author likely has a reason for the current implementation.
Workflow ID: wflow_wKDD1Pk0gH2gUS6h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a0ce527 in 34 seconds
More details
- Looked at
48lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integrations-service/gunicorn_conf.py:2
- Draft comment:
The import statement forosis duplicated. Please remove the redundant import on line 2. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for 'os' is duplicated, which is unnecessary and should be removed.
Workflow ID: wflow_k96VZKNk2emN5KDs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 73b9321 in 26 seconds
More details
- Looked at
55lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/common/storage_handler.py:70
- Draft comment:
Consider usingkwargs.keys()andkwargs.values()directly instead oflist(zip(*kwargs.items())) or ([], [])for better readability and efficiency.
kwargs_keys, kwargs_values = kwargs.keys(), kwargs.values()
- Reason this comment was not posted:
Confidence changes required:50%
The use oflist(zip(*kwargs.items())) or ([], [])is a workaround to handle empty dictionaries. However, it can be simplified by usingkwargs.keys()andkwargs.values()directly, which are more readable and efficient.
2. agents-api/agents_api/common/storage_handler.py:90
- Draft comment:
Consider usingarg.keys()andarg.values()directly instead oflist(zip(*arg.items())) or ([], [])for better readability and efficiency.
keys, values = arg.keys(), arg.values()
- Reason this comment was not posted:
Confidence changes required:50%
The same issue withlist(zip(*arg.items())) or ([], [])appears multiple times in the code. It should be addressed in all instances for consistency and efficiency.
3. agents-api/agents_api/common/storage_handler.py:141
- Draft comment:
Consider usingv.keys()andv.values()directly instead oflist(zip(*v.items())) or ([], [])for better readability and efficiency.
keys, values = v.keys(), v.values()
- Reason this comment was not posted:
Confidence changes required:50%
The same issue withlist(zip(*v.items())) or ([], [])appears multiple times in the code. It should be addressed in all instances for consistency and efficiency.
Workflow ID: wflow_CAY1HUFt7dAatVS1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c095f09 in 32 seconds
More details
- Looked at
140lines of code in9files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. integrations-service/integrations/autogen/Tools.py:12
- Draft comment:
The PR description mentions removing unused Cloudinary and Ffmpeg integration definitions fromTools.py, but these changes are not visible in the diff. Please ensure these definitions are removed if they are indeed unused. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_p2L2lmQItQEIddr9
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces async S3 client using
aiobotocore, updates task execution workflows, and refines integration services inagents-api.async_s3.pyfor async S3 operations usingaiobotocore.create_file.py,delete_file.py, andget_file.py.store_in_blob_store_if_large()andload_from_blob_store_if_remote()to async instorage_handler.py.TaskExecutionWorkflowintask_execution/__init__.pyfor async blob storage operations.prepare_for_step()intasks.pyfor async input loading.execute_integration.pyand integration modules for async operations.Tools.py.aiobotocoreandasync-lrutopyproject.toml.boto3dependency frompyproject.toml.This description was created by
for c095f09. It will automatically update as commits are pushed.