F/sdk verb update #40

philipbalbas · 2024-04-12T02:54:26Z

	🚀 This PR description was created by Ellipsis for commit `866b72d`.

Summary:

This PR introduces an 'overwrite' option to the update methods in the Python and TypeScript SDKs, allowing for full or partial updates of agents, sessions, tools, and users, with updated or new tests for this functionality, and removes the unrelated pyjwt dependency from pyproject.toml.

Key points:

Introduced 'overwrite' option to update methods in Python and TypeScript SDKs
'overwrite' option allows for full or partial updates
Changes affect agents, sessions, tools, and users
Changes reflected in /sdks/ts/src/managers/agent.ts, /sdks/ts/src/managers/session.ts, /sdks/ts/src/managers/tool.ts, and /sdks/ts/src/managers/user.ts
Tests updated or added for new functionality
Unrelated change: pyjwt dependency removed from pyproject.toml

Generated with ❤️ by ellipsis.dev

ellipsis-dev

👍 Looks good to me!

Reviewed the entire pull request up to 624d979
Looked at 433 lines of code in 8 files
Took 1 minute and 50 seconds to review

More info

Skipped 0 files when reviewing.
Skipped posting 4 additional comments because they didn't meet confidence threshold of 50%.

1. sdks/python/julep/managers/agent.py:135:

Assessed confidence : 100%
Grade: 0%
Comment:
Please add a type hint for the 'overwrite' parameter to ensure it is a boolean.
Reasoning:
The PR introduces an 'overwrite' flag to the update methods of various managers. This flag determines whether to use the 'update' or 'patch' method of the api client. However, there is no validation to ensure that the 'overwrite' flag is a boolean. This could lead to unexpected behavior if a non-boolean value is passed.

2. sdks/python/julep/managers/session.py:241:

Assessed confidence : 100%
Grade: 0%
Comment:
Please add a type hint for the 'overwrite' parameter to ensure it is a boolean. This comment is also applicable to the update methods in the 'tool.py' and 'user.py' files.
Reasoning:
The PR introduces an 'overwrite' flag to the update methods of various managers. This flag determines whether to use the 'update' or 'patch' method of the api client. However, there is no validation to ensure that the 'overwrite' flag is a boolean. This could lead to unexpected behavior if a non-boolean value is passed.

3. sdks/ts/src/managers/agent.ts:121:

Assessed confidence : 100%
Grade: 0%
Comment:
Please add a type hint for the 'overwrite' parameter to ensure it is a boolean.
Reasoning:
The PR introduces an 'overwrite' flag to the update methods of various managers. This flag determines whether to use the 'update' or 'patch' method of the api client. However, there is no validation to ensure that the 'overwrite' flag is a boolean. This could lead to unexpected behavior if a non-boolean value is passed.

4. sdks/ts/src/managers/session.ts:98:

Assessed confidence : 100%
Grade: 0%
Comment:
Please add a type hint for the 'overwrite' parameter to ensure it is a boolean. This comment is also applicable to the update method in the 'tool.ts' and 'user.ts' files.
Reasoning:
The PR introduces an 'overwrite' flag to the update methods of various managers. This flag determines whether to use the 'update' or 'patch' method of the api client. However, there is no validation to ensure that the 'overwrite' flag is a boolean. This could lead to unexpected behavior if a non-boolean value is passed.

Workflow ID: wflow_Ne0rL9Q8IpQKXHUm

Not what you expected? You can customize the content of the reviews using rules. Learn more here.

ellipsis-dev

❌ Changes requested.

Performed an incremental review on 09ce9a6
Looked at 497 lines of code in 9 files
Took 1 minute and 39 seconds to review

More info

Skipped 0 files when reviewing.
Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.

1. /sdks/ts/src/managers/session.ts:96:

Assessed confidence : 90%
Grade: 0%
Comment:
Ensure that the 'requestBody' object is being correctly passed to the 'updateSession' and 'patchSession' methods. Currently, it seems like it's not being spread into these methods.
Reasoning:
In the 'update' method of the 'SessionsManager' class, the 'requestBody' object is not being spread into the 'updateSession' or 'patchSession' methods. This could potentially lead to these methods not receiving the necessary data to perform their operations.

Workflow ID: wflow_KIuAmCzcSeOHZNPE

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

ellipsis-dev · 2024-04-13T01:04:57Z

sdks/ts/src/managers/agent.ts

+      });
+
+      const agent: Partial<Agent> & { id: string } = {
+        ...result,


Be careful with the order of spreading properties into the 'agent' object. The 'requestBody' object is being spread after the 'result' object, which could potentially overwrite properties in 'result'.

ellipsis-dev · 2024-04-13T01:04:57Z

sdks/ts/src/managers/tool.ts

+      const updatedTool: Tool = { type: "function", ...result, ...tool };
+      return updatedTool;
+    } else {
+      const result = await this.apiClient.default.patchAgentTool({


Ensure that the 'requestBody' object is being correctly passed to the 'patchAgentTool' method. Currently, it seems like it's not being passed to this method.

sdks/ts/src/managers/user.ts

Signed-off-by: Diwank Singh Tomer <[email protected]>

ellipsis-dev

👍 Looks good to me!

Performed an incremental review on 5978a18
Looked at 1534 lines of code in 18 files
Took 2 minutes and 58 seconds to review

More info

Skipped 1 files when reviewing.
Skipped posting 3 additional comments because they didn't meet confidence threshold of 50%.

1. /sdks/python/julep/managers/user.py:160:

Assessed confidence : 100%
Grade: 0%
Comment:
The docstring for the _update method does not mention the 'overwrite' parameter. Please update the docstring to include this parameter and explain its purpose and behavior.
Reasoning:
The 'overwrite' option in the update methods of the Python SDK is not properly documented. The docstrings for the update methods in the Agent and User managers do not mention the 'overwrite' parameter. This could lead to confusion for users of the SDK.

2. /sdks/python/julep/managers/agent.py:47:

Assessed confidence : 100%
Grade: 0%
Comment:
The 'metadata' field is not documented. Please add a comment or update the docstring to explain what this field is for and what type of data it expects.
Reasoning:
The 'metadata' field has been added to the Agent and User models, but it's not clear from the PR description or the code comments what this field is for. It's also not clear what type of data is expected for this field. This could lead to confusion for users of the SDK.

3. /sdks/python/julep/managers/user.py:82:

Assessed confidence : 100%
Grade: 0%
Comment:
The 'metadata' field is not documented. Please add a comment or update the docstring to explain what this field is for and what type of data it expects.
Reasoning:
The 'metadata' field has been added to the Agent and User models, but it's not clear from the PR description or the code comments what this field is for. It's also not clear what type of data is expected for this field. This could lead to confusion for users of the SDK.

Workflow ID: wflow_CahFxDX7yFt8ExTU

Not what you expected? You can customize the content of the reviews using rules. Learn more here.

* feat(agents-api): Make user_id optional when creating a new session Signed-off-by: Diwank Singh Tomer <[email protected]> * wip: added metadata arg to sdk methods Signed-off-by: Diwank Singh Tomer <[email protected]> --------- Signed-off-by: Diwank Singh Tomer <[email protected]>

Signed-off-by: Diwank Singh Tomer <[email protected]>

ellipsis-dev

👍 Looks good to me!

Performed an incremental review on 5980c79
Looked at 168 lines of code in 3 files
Took 2 minutes and 34 seconds to review

More info

Skipped 0 files when reviewing.
Skipped posting 2 additional comments because they didn't meet confidence threshold of 50%.

1. /sdks/python/julep/managers/user.py:199:

Assessed confidence : 50%
Comment:
The PR description mentions that an 'overwrite' option has been added to the update methods in the Python and TypeScript SDKs. However, the diff only shows changes in the Python SDK. Please confirm if the TypeScript SDK changes are missing or if the PR description is incorrect.
Reasoning:
The PR description mentions that an 'overwrite' option has been added to the update methods in the Python and TypeScript SDKs. However, the diff only shows changes in the Python SDK. I need to check if the TypeScript SDK changes are missing or if the PR description is incorrect.

2. /sdks/python/tests/test_users.py:82:

Assessed confidence : 80%
Grade: 0%
Comment:
It would be beneficial to add test cases that specifically test the 'overwrite' functionality in the update method to ensure it works as expected.
Reasoning:
The 'overwrite' option is not being used in the test cases. It would be good to add test cases to verify the functionality of the 'overwrite' option.

Workflow ID: wflow_YWeBCS45P35om3rj

Not what you expected? You can customize the content of the reviews using rules. Learn more here.

ellipsis-dev

❌ Changes requested.

Performed an incremental review on 55af93d
Looked at 277 lines of code in 6 files
Took 3 minutes and 41 seconds to review

More info

Skipped 0 files when reviewing.
Skipped posting 2 additional comments because they didn't meet confidence threshold of 50%.

1. sdks/python/julep/managers/agent.py:284:

Assessed confidence : 100%
Grade: 0%
Comment:
The 'overwrite' option is not being used in the 'update' method. Please ensure that this option is implemented correctly to allow for both full and partial updates.
Reasoning:
In the file 'sdks/python/julep/managers/agent.py', the 'update' method has been modified to include an 'overwrite' option. This option allows the user to choose between a full update (overwrite) or a partial update (patch). However, the 'overwrite' option is not being used in the 'update' method. This could lead to unexpected behavior if the user expects a full update but only a partial update is performed.

2. sdks/python/tests/test_agents.py:37:

Assessed confidence : 100%
Grade: 0%
Comment:
The 'client' fixture, which is an instance of Client, is being used in this test instead of the newly created instance of AsyncClient. Please ensure that the correct client instance is being used.
Reasoning:
In the file 'sdks/python/tests/test_agents.py', the test 'agents: async agents.create, agents.get, agents.update & agents.delete' creates a new instance of AsyncClient but does not use it in the test. Instead, it uses the 'client' fixture which is an instance of Client, not AsyncClient. This could lead to unexpected behavior in the test.

Workflow ID: wflow_67wucn91Rvh2271v

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.

ellipsis-dev · 2024-04-15T11:18:33Z

sdks/python/tests/test_sessions.py

+@test(
+    "sessions: async sessions.create, sessions.get, sessions.update & sessions.delete"
+)
+async def _(client=async_client, session_agent_user=test_session_agent_user):


The 'client' fixture, which is an instance of Client, is being used in this test instead of the newly created instance of AsyncClient. Please ensure that the correct client instance is being used.

ellipsis-dev · 2024-04-15T11:18:33Z

sdks/python/tests/test_tools.py

+    assert len(response) == 0
+
+
+@test("tools: async tools.update")


The 'client' fixture, which is an instance of Client, is being used in this test instead of the newly created instance of AsyncClient. Please ensure that the correct client instance is being used.

whiterabbit1983 · 2024-04-15T11:19:01Z

agents-api/pyproject.toml

@@ -33,6 +33,7 @@ cozo-migrate = "^0.2.0"
 poethepoet = "^0.24.4"
 pytype = "^2024.1.5"
 julep = "^0.2.4"
+pyjwt = "^2.8.0"


How do we use this package?

ellipsis-dev

👍 Looks good to me!

Performed an incremental review on 866b72d
Looked at 12 lines of code in 1 files
Took 2 minutes and 46 seconds to review

More info

Skipped 1 files when reviewing.
Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.

1. sdks/python/pyproject.toml:29:

Assessed confidence : 50%
Comment:
The 'pyjwt' package has been removed from the project dependencies. Please ensure that there are no parts of the codebase that still rely on it.
Reasoning:
The 'pyjwt' package has been removed from the project dependencies. This could potentially cause issues if there are parts of the codebase that still rely on it. I need to check the codebase to see if 'pyjwt' is being used anywhere.

Workflow ID: wflow_EzYVFwU4f3d2b5A0

Not what you expected? You can customize the content of the reviews using rules. Learn more here.

* feat(python-sdk): use patch methods * feat(ts-sdk): use patch methods * feat(python-sdk): add overwrite tests * feat(ts-sdk): add overwrite tests * wip(python-sdk): Fix python sdk fixtures Signed-off-by: Diwank Singh Tomer <[email protected]> * wip: metadata in sdk (#39) * feat(agents-api): Make user_id optional when creating a new session Signed-off-by: Diwank Singh Tomer <[email protected]> * wip: added metadata arg to sdk methods Signed-off-by: Diwank Singh Tomer <[email protected]> --------- Signed-off-by: Diwank Singh Tomer <[email protected]> * wip(python-sdk): Make user tests pass Signed-off-by: Diwank Singh Tomer <[email protected]> * feat(python-sdk): update tests * fix: remove unused deps --------- Signed-off-by: Diwank Singh Tomer <[email protected]> Co-authored-by: Diwank Singh Tomer <[email protected]>

philipbalbas added 2 commits April 12, 2024 10:42

feat(python-sdk): use patch methods

74e5798

feat(ts-sdk): use patch methods

624d979

ellipsis-dev bot reviewed Apr 12, 2024

View reviewed changes

philipbalbas added 2 commits April 13, 2024 08:44

feat(python-sdk): add overwrite tests

4d60151

feat(ts-sdk): add overwrite tests

09ce9a6

ellipsis-dev bot reviewed Apr 13, 2024

View reviewed changes

creatorrr added 2 commits April 14, 2024 11:58

Merge branch 'dev' into f/sdk-verb-update

04feaa2

wip(python-sdk): Fix python sdk fixtures

5978a18

Signed-off-by: Diwank Singh Tomer <[email protected]>

ellipsis-dev bot reviewed Apr 14, 2024

View reviewed changes

creatorrr added 2 commits April 15, 2024 09:41

wip(python-sdk): Make user tests pass

5980c79

Signed-off-by: Diwank Singh Tomer <[email protected]>

ellipsis-dev bot reviewed Apr 15, 2024

View reviewed changes

feat(python-sdk): update tests

55af93d

ellipsis-dev bot reviewed Apr 15, 2024

View reviewed changes

whiterabbit1983 reviewed Apr 15, 2024

View reviewed changes

fix: remove unused deps

866b72d

ellipsis-dev bot reviewed Apr 15, 2024

View reviewed changes

creatorrr and others added 2 commits April 15, 2024 18:24

Merge branch 'dev' into f/sdk-verb-update

90e23b1

Merge branch 'dev' into f/sdk-verb-update

452dc0a

philipbalbas merged commit f488389 into dev Apr 16, 2024
3 of 9 checks passed

philipbalbas deleted the f/sdk-verb-update branch April 16, 2024 04:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

F/sdk verb update #40

F/sdk verb update #40

philipbalbas commented Apr 12, 2024 •

edited by ellipsis-dev bot

Loading

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot Apr 13, 2024

ellipsis-dev bot Apr 13, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot Apr 15, 2024

ellipsis-dev bot Apr 15, 2024

whiterabbit1983 Apr 15, 2024

ellipsis-dev bot left a comment

F/sdk verb update #40

F/sdk verb update #40

Conversation

philipbalbas commented Apr 12, 2024 • edited by ellipsis-dev bot Loading

Summary:

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Apr 13, 2024

Choose a reason for hiding this comment

ellipsis-dev bot Apr 13, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Apr 15, 2024

Choose a reason for hiding this comment

ellipsis-dev bot Apr 15, 2024

Choose a reason for hiding this comment

whiterabbit1983 Apr 15, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

philipbalbas commented Apr 12, 2024 •

edited by ellipsis-dev bot

Loading