-
Notifications
You must be signed in to change notification settings - Fork 907
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(agents-api): Minor fix to tests #457
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 aba5421 in 48 seconds
More details
- Looked at
71
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/routers/tasks/create_task.py:26
- Draft comment:
The removal oftask_id
generation and direct passing ofdata
tocreate_task_query
simplifies the function but assumes changes in the handling of task creation logic. Ensure thatcreate_task_query
is updated to handle these changes correctly, including task ID generation and workflow construction. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_PbjM4zqbrhFxuRz1
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 Tomer <[email protected]>
aba5421
to
82c8af2
Compare
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. Incremental review on 82c8af2 in 32 seconds
More details
- Looked at
117
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_NpbgdlQGpILSfiDx
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 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.
❌ Changes requested. Incremental review on b1fc2a4 in 1 minute and 11 seconds
More details
- Looked at
285
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/agents_api/models/session/update_session.py:84
- Draft comment:
The hardcoded string 'ASSERT' forupdated_at
is likely incorrect and should be replaced with a valid timestamp or operation.
updated_at = floor(now()),
- Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/models/utils.py:71
- Draft comment:
The expandedmark_session_updated_query
function now includes many session attributes which might not be necessary and could complicate the query. Consider reverting to a simpler version that focuses only on updating theupdated_at
timestamp.
return f"""
input[developer_id, session_id] <- [[
to_uuid("{str(developer_id)}"),
to_uuid("{str(session_id)}"),
]]
?:update sessions {{
developer_id,
session_id,
updated_at = floor(now()),
}}
"""
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_EdvlBqzU9cvPkbTD
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 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! Incremental review on e570ead in 34 seconds
More details
- Looked at
811
lines of code in15
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/autogen/Entries.py:48
- Draft comment:
Thetokenizer
andtoken_count
fields have been updated to be non-optional in theBaseEntry
model. Ensure that all references and usages across the codebase and in SDKs are updated to reflect this change to maintain consistency and avoid runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is asking to ensure that all references and usages across the codebase are updated, which is speculative and not actionable within the context of this specific file. The change is clear in the diff, and the build process should catch any issues related to this change.
I might be overlooking the importance of ensuring consistency across the codebase, but the comment does not provide actionable steps for this specific file.
The comment is not actionable for this specific file and is speculative about the rest of the codebase, which is outside the scope of this review.
The comment should be removed as it is speculative and not actionable within the context of this file review.
Workflow ID: wflow_L3w1R0ogKv8BkN08
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer <[email protected]>
e570ead
to
313a312
Compare
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. Incremental review on 313a312 in 29 seconds
More details
- Looked at
553
lines of code in15
files - Skipped
1
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_dBptlXM3kMRHsjtR
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 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! Incremental review on 152c29a in 1 minute and 41 seconds
More details
- Looked at
353
lines of code in26
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. agents-api/agents_api/models/agent/create_agent.py:39
- Draft comment:
The_kind="inserted"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative as it asks to verify if the_kind="inserted"
parameter is correctly implemented. It does not point out a definite issue but rather suggests a potential problem. According to the rules, speculative comments should not be kept.
The comment might be useful if the_kind
parameter is indeed not supported by thewrap_in_class
decorator, but without definite evidence of an issue, it remains speculative.
The rules clearly state not to keep speculative comments, so even if there is a chance the comment is useful, it should not be kept unless it points out a definite issue.
The comment is speculative and does not point out a definite issue, so it should be removed according to the rules.
2. agents-api/agents_api/models/agent/delete_agent.py:41
- Draft comment:
The_kind="deleted"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative, asking to verify if the_kind="deleted"
parameter is correctly implemented. It does not point out a definite issue but rather raises a concern about a potential problem. According to the rules, speculative comments should not be kept unless there is a definite issue.
The comment could be useful if the_kind
parameter is indeed not supported by thewrap_in_class
decorator, but without more context, it remains speculative.
The rules clearly state not to keep speculative comments unless there is a definite issue. Without evidence of a problem, the comment should not be kept.
The comment is speculative and does not point out a definite issue, so it should be removed.
3. agents-api/agents_api/models/agent/patch_agent.py:29
- Draft comment:
The_kind="replaced"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/models/agent/update_agent.py:28
- Draft comment:
The_kind="replaced"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Marked as duplicate.
5. agents-api/agents_api/models/docs/delete_doc.py:32
- Draft comment:
The_kind="deleted"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative as it asks to verify if the parameter is supported, which goes against the rules of not making speculative comments. The comment does not point out a definite issue but rather raises a question about the implementation, which is not actionable without further context.
The comment could be useful if the_kind
parameter is indeed not supported by thewrap_in_class
decorator, but without context, it is speculative. The comment does not provide a clear action to take.
Without additional context or evidence that_kind
is unsupported, the comment remains speculative. The rules specify not to make speculative comments, so it should be removed.
The comment should be removed as it is speculative and does not point out a definite issue.
6. agents-api/agents_api/models/docs/embed_snippets.py:30
- Draft comment:
The_kind="replaced"
parameter is added to thewrap_in_class
decorator, which is not standard for this decorator and might not be supported. Please verify if this parameter is correctly implemented and handled in thewrap_in_class
function. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_kqLWSwbGTstihKhQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank 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! Incremental review on e97b6fb in 33 seconds
More details
- Looked at
250
lines of code in6
files - Skipped
2
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/routers/docs/create_doc.py:16
- Draft comment:
Therun_embed_docs_task
function is correctly implemented and used in the document creation endpoints to initiate the embedding process as a background job. This aligns with the PR's intent to enhance document handling by asynchronously processing embeddings. - Reason this comment was not posted:
Confidence changes required:0%
The PR description mentions refactoring and updating thecreate_doc
logic in theagents-api
to use a newrun_embed_docs_task
function. This function is defined increate_doc.py
and is used to execute a workflow for embedding documents after they are created. The function takes parameters likedoc_id
,title
,content
, andjob_id
, and it uses theTemporalClient
to execute the workflow. The PR also updates the tests to check that a job ID is returned when documents are created, which aligns with the changes made in thecreate_doc
logic.
Workflow ID: wflow_0oypy6EKJXmMDWpG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer [email protected]
Summary:
Refactored
agents-api
to enhance code quality, add document embedding tasks, and update tests for improved session handling.Key points:
agents-api
for improved code quality and session logic._kind
attribute towrap_in_class
decorators.create_entries
and include_kind="inserted"
.updated_at
logic in session models.create_task
.create_task
to useTask
object.graceful_shutdown_timeout
tocreate_worker
.temporal.py
.create_doc.py
.create_user_doc
andcreate_agent_doc
for document embedding.patch_temporal_get_client
fixture for mocking in tests.Generated with ❤️ by ellipsis.dev