-
Notifications
You must be signed in to change notification settings - Fork 36
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
(do-not-merge) new feature trunk: tasks #351
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Diwank Tomer <[email protected]> Co-authored-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! Reviewed everything up to d1841b8 in 1 minute and 5 seconds
More details
- Looked at
87
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. agents-api/migrations/migrate_1716939839_task_relations.py:1
- Draft comment:
The shebang line is incorrectly formatted. It should be#!/usr/bin/env python3
without a space after#
to ensure proper execution. - Reason this comment was not posted:
Confidence of 30% on close inspection, compared to threshold of 50%.
2. agents-api/migrations/migrate_1716939839_task_relations.py:10
- Draft comment:
The method of constructing the final query string in therun
function might lead to invalid syntax. Each query should be correctly formatted as a complete JSON object if they are to be joined in this manner. - Reason this comment was not posted:
Confidence of 30% on close inspection, compared to threshold of 50%.
3. agents-api/migrations/migrate_1716939839_task_relations.py:20
- Draft comment:
The type 'Validity' used for 'updated_at_ms' is not a standard SQL type. Please ensure it is correctly defined or replace it with a standard type if it's an error. - Reason this comment was not posted:
Confidence of 40% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_nP7XDMDpmq0Uf9ys
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* openapi schema, definitions & sdks updated; endpoints added for tasks feature * follow convention for naming routes
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 170c301 in 1 minute and 56 seconds
More details
- Looked at
13
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:41
- Draft comment:
Ensure that the function calllist_tasks_query
is correctly named and intended to be used here, as the function name was changed in the endpoint but not inside the function body. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_tDOnBnhWOJY4HOiQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* wip(agents-api): cozo queries for tasks Signed-off-by: Diwank Tomer <[email protected]> * doc: Add documentation for the new endpoints * wip: Boilerplate for execution transitions queries * wip: add boilerplate execution transitions queries * wip: task feature routes and models * feat(agents-api): Add tests for task queries and fix bugs Signed-off-by: Diwank Tomer <[email protected]> * fix(agents-api): Fix cozo queries and make tests pass Signed-off-by: Diwank Tomer <[email protected]> --------- Signed-off-by: Diwank Tomer <[email protected]> Co-authored-by: Diwank Tomer <[email protected]> Co-authored-by: Siddharth Balyan <[email protected]>
* wip: Fix openapi.yaml Signed-off-by: Diwank Tomer <[email protected]> * wip: generate sdk files * feat(agents-api): Rename transition.type and execution.status Signed-off-by: Diwank Tomer <[email protected]> * fix(sdks/python): Minor fix Signed-off-by: Diwank Tomer <[email protected]> * wip: demo implementation of tasks Signed-off-by: Diwank Tomer <[email protected]> * wip: demo implementation of tasks Signed-off-by: Diwank Tomer <[email protected]> * wip(agents-api): temporal workflow codec Signed-off-by: Diwank Tomer <[email protected]> * wip: demo implementation of tasks Signed-off-by: Diwank Tomer <[email protected]> * feat(agents-api): Toy implementation of Tasks Signed-off-by: Diwank Tomer <[email protected]> --------- Signed-off-by: Diwank Tomer <[email protected]> Co-authored-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 ea7bbff in 1 minute and 53 seconds
More details
- Looked at
109
lines of code in2
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/pyproject.toml:36
- Draft comment:
The PR adds a new dependencycel-python
which is used in theevaluate_step
function. Please update the PR description to include this change for complete documentation. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_IcFvtqyNtcgR4xl2
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 5393c4b in 2 minutes and 9 seconds
More details
- Looked at
563
lines of code in6
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/routers/sessions/session.py:84
- Draft comment:
The@cache
decorator is incorrectly applied tollm_generate
, which is not an instance method. This could lead to errors sincecache
expects aself
parameter. Consider refactoringcache
to support both instance and standalone functions or adjust the usage accordingly.
# Consider refactoring cache or using a different approach for caching standalone functions
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_lEnYDsIJAe72RRgr
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 16b53fa in 1 minute and 16 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_vBL8MEKpmMdkHwLH
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.
|
||
# Transition type | ||
transition_type = ( | ||
"awaiting_input" if should_wait else ("finish" if is_last else "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 change in the order of conditions for determining transition_type
might introduce logical errors, especially if the workflow is expected to finish on the last step regardless of whether it is awaiting further input. Consider reverting the order or ensuring that the new logic aligns with the intended workflow behavior.
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.
I think we should make it more explicit and document this thoroughly coz otherwise it could get too confusing and coz logical errors for sure
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 4248da9 in 1 minute and 49 seconds
More details
- Looked at
39
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution.py:123
- Draft comment:
The variableshould_wait
is used here but it's only defined conditionally within thePromptWorkflowStep
case. This can lead to aNameError
if other step types are processed. Consider initializingshould_wait
before the match statement to ensure it always has a defined value.
should_wait = False
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_mkz2a3M85NYCIxk1
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 4770cbb in 2 minutes and 12 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/__init__.py:3
- Draft comment:
The import statement forcelpy
is commented out. Ifcelpy
is not used in this file or elsewhere in the project, consider removing this line to clean up the code. - Reason this comment was not posted:
Confidence changes required:33%
The import statement forChatCompletion
has been updated to a more specific path, which is generally good for clarity and avoiding potential conflicts. However, the import statement forcelpy
is commented out, which might indicate that it's not used or needed anymore. This could be a cleanup opportunity ifcelpy
is indeed not used elsewhere in the file.
Workflow ID: wflow_2f9gMFV2RkrZ3S4Q
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 9a5a36e in 2 minutes and 4 seconds
More details
- Looked at
143
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_pUS8SnWCeDIoLd2a
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 836d2c4 in 2 minutes and 26 seconds
More details
- Looked at
161
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_rDXiWCiv0baSmKb7
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.
|
||
class TransitionInfo(BaseModel): | ||
from_: List[str | int] | Tuple[str | int] | ||
to: List[str | int] | None = None |
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.
Consider allowing the to
field in TransitionInfo
to also accept Tuple[str | int]
for consistency with the from_
field.
to: List[str | int] | None = None | |
to: List[str | int] | Tuple[str | int] | None = None |
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.
it should be Tuple[str, int]
rather than Tuple[str | int]
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.
this is fixed
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 ab9011c in 1 minute and 52 seconds
More details
- Looked at
100
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/models/execution/create_execution_transition.py:17
- Draft comment:
The type annotation for theto
parameter is inconsistent with theTransitionInfo
class definition. Consider aligning the type annotations to ensure consistency and prevent type errors.
to: List[str | int] | None,
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_KlpIGBbP6X0vThFy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Signed-off-by: Diwank Tomer <[email protected]>
… types Signed-off-by: Diwank Tomer <[email protected]>
refactor(agents-api): Minor refactors to typespec types
Signed-off-by: Diwank Tomer <[email protected]>
Signed-off-by: Diwank Tomer <[email protected]>
fix(agents-api): Fix map reduce step and activity
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]>
Signed-off-by: Diwank Tomer <[email protected]>
fix(agents-api): Make the sample work
Summary:
Enhances task management with new features, consistent naming, validations, CRUD operations, and migration scripts.
Key points:
tools_available: "all"
option in task definition to include all tools accessible to the agent (agents-api/agents_api/autogen/openapi_model.py
).agents-api/agents_api/migrations/migrate_1716939839_task_relations.py
.tasks
,executions
,transitions
withup
(create) anddown
(remove) queries.run
function to execute queries formatted as JSON.list_sessions
tolist_tasks
inagents-api/agents_api/routers/tasks/routers.py
.agents-api/agents_api/activities/task_steps/__init__.py
for various workflow steps.cel-python
dependency inpyproject.toml
.prompt_step
andtransition_step
activities.TaskProtocol
,ExecutionInput
,StepContext
, andTransitionInfo
.create_execution_transition_query
to handle new transition data.llm_generate
and caching mechanism.PydanticPayloadConverter
for better serialization.TaskExecutionWorkflow
to manage task execution flow.agents-api/agents_api/workflows/task_execution.py
to handleawaiting_input
,finish
, andstep
states.TasksManager
andAsyncTasksManager
classes insdks/python/julep/managers/task.py
for task management.Client
andAsyncClient
classes insdks/python/julep/client.py
to include task management capabilities.beartype
decorator.agents-api/tests/test_tasks.py
andsdks/python/tests/test_tasks.py
.Generated with ❤️ by ellipsis.dev