Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for the data-platform API, enabling users to search and query various entities (ligands, proteins, etc.) through the DeepOriginClient.
Changes:
- Added a new
DataAPI wrapper class with methods for health checks, model listing, and entity searching - Integrated the Data wrapper into DeepOriginClient
- Added comprehensive test coverage and mock server endpoints for the new functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/data.py | New Data API wrapper class with methods for searching entities, ligands, and proteins, plus health and model listing endpoints |
| src/platform/client.py | Integration of Data wrapper into DeepOriginClient initialization |
| tests/test_data.py | Comprehensive test suite for data platform functionality including health checks, search operations, and error handling |
| tests/mock_server/server.py | Mock endpoints for data platform API including health, search, and model listing |
| docs/platform/ref/data.md | User-facing documentation for the Data API with usage examples |
| mkdocs.yaml | Added data reference to documentation navigation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_search_proteins_lv1(): | ||
| """Test searching proteins using convenience method.""" | ||
| client = DeepOriginClient() | ||
| response = client.data.search_proteins() | ||
|
|
||
| assert isinstance(response, dict), "Expected a dictionary response" | ||
| assert "data" in response, "Expected 'data' key in response" | ||
| assert isinstance(response["data"], list), "Expected 'data' to be a list" | ||
|
|
||
|
|
||
| def test_search_proteins_molecular_weight(): | ||
| """Test searching proteins with molecular weight filters.""" | ||
| client = DeepOriginClient() | ||
| response = client.data.search_proteins( | ||
| min_molecular_weight=250, max_molecular_weight=550 | ||
| ) | ||
|
|
||
| assert isinstance(response, dict), "Expected a dictionary response" | ||
| assert "data" in response, "Expected 'data' key in response" | ||
| assert isinstance(response["data"], list), "Expected 'data' to be a list" | ||
|
|
There was a problem hiding this comment.
There is no test coverage for the pdb_id parameter in search_proteins. Consider adding a test that verifies the pdb_id filter works correctly.
| entity: str, | ||
| *, | ||
| cursor: str | None = None, | ||
| filter_dict: dict[str, Any] | None = None, |
There was a problem hiding this comment.
The parameter name is inconsistent between search_ligands_with_results which uses 'filter' (line 48, 157) and the internal search method which uses 'filter_dict' (line 95). This inconsistency makes the API confusing. Consider renaming 'filter_dict' to 'filter' in the search method signature to maintain consistency across the API, or rename 'filter' to 'filter_dict' in search_ligands_with_results and search_ligands for consistency.
| def search_proteins( | ||
| self, | ||
| *, | ||
| cursor: str | None = None, | ||
| pdb_id: str | None = None, | ||
| min_molecular_weight: float | int | None = None, | ||
| max_molecular_weight: float | int | None = None, | ||
| sequence: str | None = None, | ||
| limit: int | None = None, | ||
| offset: int | None = None, | ||
| select: list[str] | None = None, | ||
| sort: dict[str, str] | None = None, | ||
| ) -> dict: |
There was a problem hiding this comment.
The search_proteins method does not accept a filter parameter like search_ligands does (line 157). Users cannot pass custom filters to search_proteins, which limits flexibility. Consider adding a filter parameter to maintain consistency with search_ligands and allow users to specify custom filter criteria beyond the convenience parameters (pdb_id, molecular_weight, sequence).
| """ | ||
| # Build filter dict, starting with provided filter or empty dict | ||
| filter_dict = filter.copy() if filter is not None else {} | ||
| filter_dict.setdefault("deleted", False) |
There was a problem hiding this comment.
The search_ligands method uses filter_dict.setdefault("deleted", False) which will not override an existing "deleted" key if the user passes one in the filter. In contrast, search_proteins and the search method unconditionally set filter_dict["deleted"] = False, which overrides any user-provided value. This inconsistency in handling the "deleted" field could lead to unexpected behavior. Consider using a consistent approach across all methods - either always override or always preserve user values.
| filter_dict.setdefault("deleted", False) | |
| # Ensure deleted records are excluded consistently with other search methods | |
| filter_dict["deleted"] = False |
| def test_search_ligands_lv1(): | ||
| """Test searching ligands using convenience method.""" | ||
| client = DeepOriginClient() | ||
| response = client.data.search_ligands() | ||
|
|
||
| assert isinstance(response, dict), "Expected a dictionary response" | ||
| assert "data" in response, "Expected 'data' key in response" | ||
| assert isinstance(response["data"], list), "Expected 'data' to be a list" | ||
|
|
There was a problem hiding this comment.
There is no test coverage for the filter parameter in search_ligands. The test_search_ligands_lv1 function only tests the basic case without filters, and test_search_ligands_molecular_weight only tests the molecular weight convenience parameters. Consider adding a test that verifies the filter parameter works correctly, similar to how the search method is tested in test_search_entity_lv1.
| org_key: str, request: Request | ||
| ) -> dict[str, Any]: | ||
| """Search ligands joined with tool results.""" | ||
| body = await request.json() |
There was a problem hiding this comment.
Variable body is not used.
| org_key: str, entity: str, request: Request | ||
| ) -> dict[str, Any]: | ||
| """Search an entity.""" | ||
| body = await request.json() |
There was a problem hiding this comment.
Variable body is not used.
1 participant
changes