feat: phoenix-client-py delete example from dataset #9193
Conversation
There was a problem hiding this comment.
Let's not use mocks. Update the integration tests in Phoenix instead.
There was a problem hiding this comment.
got it, updated integration test instead
There was a problem hiding this comment.
Do we need these mocked tests if we have the IT?
| from io import BytesIO | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Any, BinaryIO, Iterator, Literal, Optional, Union | ||
| from typing import TYPE_CHECKING, Any, BinaryIO, Iterator, List, Literal, Optional, Union |
There was a problem hiding this comment.
As of Python 3.9, you can use list as a type hint, no need to import List from typing.
packages/phoenix-client/src/phoenix/client/resources/datasets/__init__.py
Outdated
Show resolved
Hide resolved
packages/phoenix-client/src/phoenix/client/resources/datasets/__init__.py
Outdated
Show resolved
Hide resolved
packages/phoenix-client/src/phoenix/client/resources/datasets/__init__.py
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
It looks good, would just adjust the route to use a DELETE method and think about nixing those mocked tests if we have sufficient coverage from the integration test since mocks tend to be difficult to maintain.
Small nits: It looks like some of the type hints have unnecessary quotes ("Union[str, List[str]]" can just be Union[str, list[str]]). Some of the comments feel verbose.
There was a problem hiding this comment.
Do we need these mocked tests if we have the IT?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…__init__.py Co-authored-by: Xander Song <axiomofjoy@gmail.com>
43cea27 to
55b8118
Compare
| if version_metadata is not None: | ||
| payload["version_metadata"] = version_metadata | ||
|
|
||
| response = self._client.delete( |
There was a problem hiding this comment.
There's an HTTP method inconsistency between the sync and async implementations. The sync client uses self._client.delete() (line 1104) while the async client uses self._client.post() (line 1851). Since the OpenAPI specification defines this endpoint with the DELETE method, the async implementation should be updated to use delete() instead of post() for consistency with both the sync implementation and the API specification.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
…httpx compatibility
| rest_response = Mock() | ||
| rest_response.raise_for_status.return_value = None | ||
|
|
||
| mock_datasets_client._client.post.return_value = rest_response |
There was a problem hiding this comment.
The test is mocking _client.post() but the implementation uses _client.request() with method='DELETE'. This mismatch will cause the test to fail. The mock setup should be changed to:
mock_datasets_client._client.request.return_value = rest_responseAdditionally, when verifying the call, the assertions should check request() was called with the correct parameters including method='DELETE'.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| assert call_args[1]["url"] == "v1/datasets/examples/delete" | ||
|
|
||
| # Verify payload structure (REST format) | ||
| payload = call_args[1]["json"] |
There was a problem hiding this comment.
There's a mismatch between the test and implementation. The test expects the request payload to be accessible via call_args[1]["json"], but the actual implementation in delete_examples() sends the data using content=json.dumps(payload) with a Content-Type header. This will cause a KeyError when the test runs.
To fix this, either:
- Update the test to parse the JSON content from
call_args[1]["content"], or - Update the mock setup to properly capture how the request is being made
The test should align with how the client actually makes the request to ensure proper validation.
| payload = call_args[1]["json"] | |
| payload = json.loads(call_args[1]["content"]) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| ) | ||
|
|
||
| await session.commit() | ||
|
|
There was a problem hiding this comment.
Bug: Validation and Revision Check Issues
The deleteDatasetExamples endpoint has two issues. It doesn't fully validate that all provided example IDs exist, which can lead to foreign key errors during batch insert for non-existent IDs. Additionally, the check for already deleted examples incorrectly looks for any DELETE revision instead of the most recent one, preventing deletion of examples that were previously deleted and then re-added.
#9057
one more pr for edit dataset example to resolve the issue