-
Notifications
You must be signed in to change notification settings - Fork 903
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
Dev tasks #447
Dev tasks #447
Conversation
* refactor(agents-api): Minor refactors Signed-off-by: Diwank Tomer <[email protected]> * feat(typespec): Add create-doc endpoint Signed-off-by: Diwank Tomer <[email protected]> * feat(agents-api): Add migrations for unifying the owner-docs tables Signed-off-by: Diwank Tomer <[email protected]> * feat(agents-api): Implement doc* models Signed-off-by: Diwank Tomer <[email protected]> --------- Signed-off-by: Diwank Tomer <[email protected]> Co-authored-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
* Reimplement tests for queries and operations Reimplement tests for queries and operations in `agents-api/agents_api/models` subdirectories using the new code. * **agents-api/agents_api/models/agent/test_agent_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/docs/test_docs_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/entry/test_entry_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/execution/test_execution_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/session/test_session_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/task/test_task_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/tools/test_tool_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. * **agents-api/agents_api/models/user/test_user_queries.py** - Uncomment import statements and test functions. - Update test functions to use new query functions. - Ensure tests pass with new code. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/julep-ai/julep?shareId=XXXX-XXXX-XXXX-XXXX). * wip: reimplement model tests Signed-off-by: Diwank Tomer <[email protected]> --------- Signed-off-by: Diwank Tomer <[email protected]> Co-authored-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]> Co-authored-by: Diwank Tomer <[email protected]>
…446) * fix(agents-api): Fix execution endpoints Signed-off-by: Diwank Tomer <[email protected]> * feat(agents-api): Add temporal workflow lookup relation and queries Signed-off-by: Diwank Tomer <[email protected]> * feat(agents-api): Add query for getting token of paused execution Signed-off-by: Diwank Tomer <[email protected]> * fix: Minorfix Signed-off-by: Diwank Tomer <[email protected]> --------- Signed-off-by: Diwank Tomer <[email protected]> Co-authored-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
feat: new routes for docs, users, search
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 2cf9af5 in 4 minutes and 32 seconds
More details
- Looked at
15185
lines of code in147
files - Skipped
3
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. sdks/ts/src/api/models/Docs_BaseDocSearchRequest.ts:8
- Draft comment:
Theconfidence
field inDocs_BaseDocSearchRequest
is not used inDocs_VectorDocSearchRequest
. Consider removing it if it's not intended for use in all extending search requests, or ensure it's utilized appropriately in all derived models. - Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/routers/tasks/routers.py:241
- Draft comment:
Theupdate_execution_query
function should be awaited to ensure the execution update completes before proceeding.
execution = await update_execution_query(
developer_id=x_developer_id,
task_id=task_id,
execution_id=execution_id,
data=data,
workflow_hande=handle,
)
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_PzIGDzTZMcRmv5IE
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.
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 6483fbe in 1 minute and 2 seconds
More details
- Looked at
28
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/tasks/routers.py:312
- Draft comment:
The change in the endpoint path from/tasks/{task_id}/executions/{execution_id}
to/executions/{execution_id}
removes the task_id from the path. Ensure this change is well-documented and communicated to avoid breaking existing integrations. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_YgAV1gUd0A3P57Cl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer <[email protected]>
feat(agents-api): Add litellm proxy to docker compose
Signed-off-by: Diwank Tomer <[email protected]>
* **agents-api/tests/test_agent_queries.py** - Update import statements to reflect the new location of the model functions - Update function calls to the new location of the model functions - Add `from agents_api.autogen.openapi_model import Agent` back * **agents-api/tests/test_docs_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_entry_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_execution_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_session_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_task_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_tool_queries.py** - Update import statements to reflect the new location of the model functions * **agents-api/tests/test_user_queries.py** - Update import statements to reflect the new location of the model functions
…eries.py` Update tests in `test_agent_queries.py` to use existing models and queries * Replace `agent.create_agent` with `create_agent` * Replace `agent.delete_agent` with `delete_agent` * Replace `agent.get_agent` with `get_agent` * Replace `agent.list_agents` with `list_agents` * Replace `agent.update_agent` with `update_agent` Update tests in `test_session_queries.py` to use existing models and queries * Import `Session` from `agents_api.autogen.openapi_model` Update tests in `test_task_queries.py` to use existing models and queries * Replace `CreateTaskRequest`, `DeleteTaskRequest`, `GetTaskRequest`, `ListTasksRequest`, `UpdateTaskRequest` with dictionaries Update tests in `test_tool_queries.py` to use existing models and queries * Replace `FunctionDef` with dictionaries Update tests in `test_user_queries.py` to use existing models and queries * Replace `agents_api.autogen.openapi_model` with `agents_api.autogen.Users`
Signed-off-by: Diwank Tomer <[email protected]>
fix(agents-api): Fixed tests for models and queries
* **`test_agent_queries.py`** - Add test for `create_or_update_agent` query - Add test for `patch_agent` query * **`test_docs_queries.py`** - Add test for `embed_snippets` query * **`test_entry_queries.py`** - Add test for `delete_entries` query * **`test_execution_queries.py`** - Add test for `create_execution_transition` query * **`test_session_queries.py`** - Add test for `create_or_update_session` query * **`test_task_queries.py`** - Add test for `create_or_update_task` query * **`test_tool_queries.py`** - Add test for `patch_tool` query * **`test_user_queries.py`** - Add test for `create_or_update_user` query
Signed-off-by: Diwank Tomer <[email protected]>
creatorrr/f add missing tests
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
feat(agents-api): Add route tests
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 30814b5 in 21 seconds
More details
- Looked at
24
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_ZLRjmFuFev9qcrLI
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.
|
||
|
||
# FIXME: Re enable this test | ||
# @test("workflow: map reduce step") |
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.
The test for the 'map reduce step' is commented out with a FIXME note. Consider re-enabling this test to ensure the functionality is covered.
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 a84cfcd in 19 seconds
More details
- Looked at
53
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution.py:8
- Draft comment:
RootModel
is not a valid import from Pydantic. UseBaseModel
instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is likely incorrect becauseRootModel
is a valid import in Pydantic v2. The comment does not suggest a necessary code change, as the import is correct. Therefore, the comment should be removed.
I might be missing some context about the specific version of Pydantic being used, but the code does not indicate any issues with the import itself.
The code does not show any errors or issues with theRootModel
import, and it aligns with Pydantic v2 features. The comment is likely based on outdated information.
The comment should be removed becauseRootModel
is a valid import in Pydantic v2, and the comment does not suggest a necessary code change.
Workflow ID: wflow_ysA264C18V9TIy0V
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 14c8258 in 22 seconds
More details
- Looked at
45
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/tests/test_execution_workflow.py:621
- Draft comment:
The 'map reduce step' test case seems to be missing the 'reduce' and 'initial' keys, which are typically part of a map-reduce operation. Ensure these are included to fully test the map-reduce functionality. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is pointing out a potential issue with the test case not fully implementing a map-reduce pattern. However, the test case seems to be using a different approach with 'over' and 'evaluate'. The comment might be assuming a traditional map-reduce pattern, which may not be applicable here. The test case might be valid as is, depending on the intended functionality.
I might be missing the context of how 'over' and 'evaluate' are intended to work in this test case. The comment could be valid if the test is indeed supposed to follow a traditional map-reduce pattern.
The test case might be using a different pattern intentionally, and the comment could be based on an incorrect assumption. It's important to consider the intended functionality of the test case.
The comment should be removed as it assumes a traditional map-reduce pattern without considering the possibility of a different intended functionality with 'over' and 'evaluate'.
Workflow ID: wflow_PZpFERwBWgQ9gjMH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 81af536 in 24 seconds
More details
- Looked at
114
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_8JmZvVYnA5hwXAwa
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.
mock_run_task_execution_workflow.assert_called_once() | ||
|
||
result = await handle.result() | ||
assert result["hello"] == "world" |
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.
The test expects result["hello"] == "world"
, but the PromptStep
logic sets state.output
to the message from response.get("choices", [{}])[0].get("message")
. Ensure the test expectation aligns with the actual output logic.
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 c3d9ce7 in 30 seconds
More details
- Looked at
114
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_Uf633ubPdW1ChP68
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer <[email protected]>
fix(agents-api): Fix map reduce step and activity
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 0a4708c in 19 seconds
More details
- Looked at
69
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_UEMtBhSwl2is2dPi
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.
[ | ||
{"value": result["value"], "network_request": request} | ||
for request in execution.input.network_requests | ||
if result["value"] in nr.response.body |
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.
The variable nr
is not defined. It should be request
instead of nr
.
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
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 3183267 in 27 seconds
More details
- Looked at
198
lines of code in6
files - Skipped
1
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/agents_api/activities/utils.py:12
- Draft comment:
Usingyaml.load
can be unsafe. Consider usingyaml.safe_load
instead for untrusted input. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is not necessary because the code already usesCSafeLoader
, which addresses the safety concern. The comment is about a change made in the diff, but the issue is already resolved. Therefore, the comment should be removed.
I might be overlooking any subtle differences betweenyaml.safe_load
andyaml.load
withCSafeLoader
. However,CSafeLoader
is generally considered safe, so the comment seems redundant.
Given thatCSafeLoader
is a safe option, the comment's suggestion to useyaml.safe_load
is unnecessary. The code is already handling the safety concern appropriately.
The comment should be removed because the code already usesCSafeLoader
, which is safe, making the comment redundant.
2. agents-api/agents_api/workflows/task_execution.py:303
- Draft comment:
Good use ofmodel_dump
to ensure the output is in a serializable format. - Reason this comment was not posted:
Confidence changes required:0%
Themodel_dump
method is used to convert the output to a dictionary. This is a good practice to ensure the output is in a serializable format.
Workflow ID: wflow_MA02U8NgVEji8Okq
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.
[ | ||
{"value": result["value"], "network_request": request} | ||
for request in inputs[0]["network_requests"] | ||
if result["value"] in nr["response"]["body"] |
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.
The variable nr
is not defined. It should be request
instead.
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 af121a2 in 31 seconds
More details
- Looked at
202
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/prompt_step.py:20
- Draft comment:
Consider adding a type hint for therole
parameter in the_content_to_dict
function to improve code clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The function_content_to_dict
inprompt_step.py
is missing a return type hint for therole
parameter. This can lead to confusion about what type of data is expected for this parameter.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:84
- Draft comment:
Remove commented-out code to keep the codebase clean and maintainable. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out code inprompt_step.py
is unnecessary and should be removed to keep the code clean and maintainable.
3. agents-api/agents_api/activities/task_steps/prompt_step.py:19
- Draft comment:
Consider adding error handling for unsupportedcontent
types in the_content_to_dict
function to prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
The_content_to_dict
function inprompt_step.py
does not handle the case wherecontent
is neither a string,Content
, norContentModel
. This could lead to unexpected behavior if an unsupported type is passed.
4. agents-api/agents_api/common/utils/template.py:71
- Draft comment:
Address the FIXME comment regardingChatMLTextContentPart
to ensure the function handles all expected input types correctly. - Reason this comment was not posted:
Confidence changes required:50%
Therender_template_parts
function intemplate.py
has a FIXME comment that suggests uncertainty about handlingChatMLTextContentPart
. This should be addressed to ensure the function handles all expected input types correctly.
5. agents-api/agents_api/activities/utils.py:17
- Draft comment:
Remove the unnecessary blank line to maintain code consistency. - Reason this comment was not posted:
Confidence changes required:10%
There is an unnecessary blank line added inutils.py
which should be removed to maintain code consistency.
Workflow ID: wflow_U4Q3KIZdEreGoR2h
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Tomer <[email protected]>
fix(agents-api): Make the sample work
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 9feb981 in 24 seconds
More details
- Looked at
37
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/tests/test_execution_workflow.py:760
- Draft comment:
The test case is missing assertions for the result content and role. Consider adding assertions to verify the expected output, similar to the previous version of the test. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it points out a change in the test logic where assertions were removed. This could potentially lead to a loss of test coverage, making it a valid concern. The comment is actionable as it suggests adding back the assertions to verify the expected output.
The removal of assertions might have been intentional if the test logic changed. However, without further context, it's reasonable to assume that the comment is valid as it addresses a change in the test's verification logic.
Even if the removal was intentional, the comment serves as a reminder to ensure that the test still covers the necessary logic. It's better to err on the side of caution and keep the comment.
Keep the comment as it addresses a change in the test logic and suggests a valid improvement to ensure test coverage.
Workflow ID: wflow_PQZWhLZ1JmRQp3nj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary:
Enhanced
agents-api
with new task and execution endpoints, updated Python, integrated functions into workflows, and added tests.Key points:
agents-api
with new task and execution endpoints.create_task
,create_task_execution
,get_execution_details
,get_task_details
,list_task_executions
,list_tasks
,patch_execution
,update_execution
.for_each_step
andmap_reduce_step
into task execution workflows.typespec/common/interfaces.tsp
for pagination and CRUD interfaces.typespec/docs/endpoints.tsp
andtypespec/executions/endpoints.tsp
.typespec/executions/models.tsp
.agents-api/agents_api/activities/summarization.py
andagents-api/agents_api/activities/task_steps/__init__.py
.agents-api/agents_api/worker/__main__.py
andagents-api/agents_api/workflows/task_execution.py
.get_developer_id
andget_developer_data
inagents-api/agents_api/dependencies/developer_id.py
.agents-api/tests/test_task_routes.py
.for_each_step
andmap_reduce_step
inagents-api/tests/test_execution_workflow.py
.agents-api/agents_api/activities/embed_docs.py
andagents-api/agents_api/activities/mem_mgmt.py
.agents-api/agents_api/worker/worker.py
for Temporal workers.Generated with ❤️ by ellipsis.dev