feat: Add GET/POST /task/list endpoint#277
Open
saathviksheerla wants to merge 6 commits intoopenml:mainfrom
Open
feat: Add GET/POST /task/list endpoint#277saathviksheerla wants to merge 6 commits intoopenml:mainfrom
saathviksheerla wants to merge 6 commits intoopenml:mainfrom
Conversation
Contributor
WalkthroughA new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
==========================================
- Coverage 54.20% 52.24% -1.96%
==========================================
Files 38 38
Lines 1581 1667 +86
Branches 137 155 +18
==========================================
+ Hits 857 871 +14
- Misses 722 794 +72
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/routers/openml/tasks.py" line_range="362-363" />
<code_context>
+ )
+ # build a reverse map: dataset_id -> task_id
+ # needed because quality rows come back keyed by did, but our tasks dict is keyed by task_id
+ did_to_task_id = {t["did"]: tid for tid, t in tasks.items()}
+ for row in qualities_result.all():
+ tid = did_to_task_id.get(row.data)
+ if tid is not None:
</code_context>
<issue_to_address>
**issue (bug_risk):** Qualities are only attached to one task per dataset, dropping qualities for other tasks sharing the same dataset.
Because `did_to_task_id` maps each dataset id to a single `task_id`, only the last task using a given dataset receives qualities; earlier tasks end up with empty `quality` lists. To handle multiple tasks per dataset, map `did -> list[task_id]` and, when iterating over `qualities_result`, attach each quality row to all tasks in that list.
</issue_to_address>
### Comment 2
<location path="tests/routers/openml/task_test.py" line_range="36-42" />
<code_context>
+ assert all(t["task_type_id"] == 1 for t in tasks)
+
+
+async def test_list_tasks_filter_tag(py_api: httpx.AsyncClient) -> None:
+ """Filter by tag returns only tasks with that tag."""
+ response = await py_api.post("/tasks/list", json={"tag": "OpenML100"})
+ assert response.status_code == HTTPStatus.OK
+ tasks = response.json()
+ assert len(tasks) > 0
+ assert all("OpenML100" in t["tag"] for t in tasks)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the remaining filters (`data_tag`, `task_id`, `data_id`, `data_name`, and other quality ranges).
Current tests exercise `task_type_id`, `tag`, `pagination`, and `number_instances`, but not the other supported filters: `data_tag`, `task_id`, `data_id`, `data_name`, `number_features`, `number_classes`, and `number_missing_values`. This leaves several WHERE-clause branches untested.
Consider adding targeted tests such as:
- `data_tag`: only tasks whose dataset has the given tag are returned.
- `task_id`: multiple IDs return exactly those tasks (and correct ordering/uniqueness if relevant).
- `data_id`: only tasks for the specified dataset(s) are returned.
- `data_name`: verifies expected matching behavior (case-(in)sensitivity as intended).
- One or more quality ranges using `number_features` or `number_classes` to exercise `_quality_clause` beyond `NumberOfInstances`.
Suggested implementation:
```python
async def test_list_tasks_filter_type(py_api: httpx.AsyncClient) -> None:
"""Filter by task_type_id returns only tasks of that type."""
response = await py_api.post("/tasks/list", json={"task_type_id": 1})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert all(t["task_type_id"] == 1 for t in tasks)
async def test_list_tasks_default(py_api: httpx.AsyncClient) -> None:
"""Default call returns active tasks with correct shape."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert isinstance(tasks, list)
assert len(tasks) > 0
# verify shape of first task
task = tasks[0]
async def test_list_tasks_filter_data_tag(py_api: httpx.AsyncClient) -> None:
"""Filter by data_tag returns only tasks whose dataset has that tag."""
# Get a tag actually present on at least one dataset
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
# Find first task that has at least one data tag
tagged_task = next(
t for t in tasks if t.get("data_tags") is not None and len(t.get("data_tags")) > 0
)
data_tag = tagged_task["data_tags"][0]
response = await py_api.post("/tasks/list", json={"data_tag": data_tag})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
data_tag in t.get("data_tags", []) for t in filtered
), "All returned tasks should have the dataset tag used for filtering"
async def test_list_tasks_filter_task_id(py_api: httpx.AsyncClient) -> None:
"""Filter by task_id returns exactly the requested tasks."""
# Discover a small set of valid task IDs from the default listing
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) >= 2
requested_ids = sorted(t["task_id"] for t in tasks[:3])
response = await py_api.post("/tasks/list", json={"task_id": requested_ids})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
returned_ids = sorted(t["task_id"] for t in filtered)
assert set(returned_ids) == set(
requested_ids
), "Filtering by task_id should return exactly the requested tasks"
# If the API defines a particular ordering, assert on that here as well
# (e.g. sorted by task_id). The current check ensures no duplicates.
assert len(returned_ids) == len(set(returned_ids))
async def test_list_tasks_filter_data_id(py_api: httpx.AsyncClient) -> None:
"""Filter by data_id returns only tasks from those datasets."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) > 0
# Take a couple of distinct data_ids from the available tasks
data_ids = []
for t in tasks:
if t["data_id"] not in data_ids:
data_ids.append(t["data_id"])
if len(data_ids) == 2:
break
response = await py_api.post("/tasks/list", json={"data_id": data_ids})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t["data_id"] in data_ids for t in filtered
), "All returned tasks should belong to one of the requested data_ids"
async def test_list_tasks_filter_data_name(py_api: httpx.AsyncClient) -> None:
"""Filter by data_name returns only tasks whose dataset has that name."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) > 0
data_name = tasks[0]["data_name"]
# Use the exact name observed in the listing
response = await py_api.post("/tasks/list", json={"data_name": data_name})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t["data_name"] == data_name for t in filtered
), "All returned tasks should have the requested data_name"
async def test_list_tasks_filter_number_features_range(py_api: httpx.AsyncClient) -> None:
"""Filter by number_features quality range returns tasks within that range."""
# Probe existing tasks to discover a concrete number_features value
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
# Find a task with a defined number_features value
task_with_features = next(
t for t in tasks if t.get("number_features") is not None
)
num_features = task_with_features["number_features"]
# Build a narrow range around that concrete value to exercise the quality clause
payload = {
"number_features": {
"min": num_features,
"max": num_features,
}
}
response = await py_api.post("/tasks/list", json=payload)
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t.get("number_features") is not None
and payload["number_features"]["min"]
<= t["number_features"]
<= payload["number_features"]["max"]
for t in filtered
), "All returned tasks should have number_features within the requested range"
```
These tests assume:
- Response objects include `data_tags` (a list of strings on each task), `task_id`, `data_id`, `data_name`, and `number_features`.
- The `/tasks/list` request model accepts:
- `data_tag` as a single string.
- `task_id` and `data_id` as lists of integers.
- `data_name` as a string.
- Quality ranges in the form `{"number_features": {"min": ..., "max": ...}}`.
To fully align with your existing API:
1. Adjust the field names (`data_tags` vs. `dataset_tags`, etc.) in the assertions to match the actual task schema.
2. Align the request payload shapes with whatever is already used in the existing `number_instances` test (e.g., if it uses `number_features_min` / `number_features_max` instead of a nested dict, mirror that here).
3. If `task_id` / `data_id` only accept a single integer instead of a list, modify `requested_ids` / `data_ids` accordingly and assert on a single returned task.
4. If data-name filtering is case-insensitive, extend `test_list_tasks_filter_data_name` to issue a request with `data_name` in a different case and assert that the same tasks are returned.
</issue_to_address>
### Comment 3
<location path="tests/routers/openml/task_test.py" line_range="56-62" />
<code_context>
+ assert ids1 != ids2
+
+
+async def test_list_tasks_number_instances_range(py_api: httpx.AsyncClient) -> None:
+ """number_instances range filter returns tasks whose dataset matches."""
+ response = await py_api.post(
+ "/tasks/list",
+ json={"number_instances": "100..1000"},
+ )
+ assert response.status_code == HTTPStatus.OK
+ assert len(response.json()) > 0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative test for invalid range syntax to ensure it does not result in a server error.
Since `_quality_clause` raises `ValueError` when `range_` doesn’t match `integer_range_regex`, it would be valuable to add a test that covers an invalid `number_instances` value (e.g. `"abc"` or `"1...2"`) and verifies:
- The response status is a validation error (e.g. 422) rather than 500.
- The error body matches the API’s standard error format.
This guards against accidentally surfacing raw `ValueError`s as 500s if validation behavior changes later.
```suggestion
async def test_list_tasks_pagination_offset(py_api: httpx.AsyncClient) -> None:
"""Offset returns different results than no offset."""
r1 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 0}})
r2 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 5}})
ids1 = [t["task_id"] for t in r1.json()]
ids2 = [t["task_id"] for t in r2.json()]
assert ids1 != ids2
async def test_list_tasks_number_instances_range(py_api: httpx.AsyncClient) -> None:
"""number_instances range filter returns tasks whose dataset matches."""
response = await py_api.post(
"/tasks/list",
json={"number_instances": "100..1000"},
)
assert response.status_code == HTTPStatus.OK
assert len(response.json()) > 0
async def test_list_tasks_number_instances_invalid_range(py_api: httpx.AsyncClient) -> None:
"""Invalid number_instances range does not raise server error and returns validation error."""
response = await py_api.post(
"/tasks/list",
json={"number_instances": "1...2"},
)
assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY
body = response.json()
assert isinstance(body, dict)
# Assert the error body follows the API's standard validation error format.
# Adjust these expectations if your API uses a different envelope.
assert "detail" in body
```
</issue_to_address>
### Comment 4
<location path="src/routers/openml/tasks.py" line_range="220" />
<code_context>
+
[email protected](path="/list", description="Provided for convenience, same as `GET` endpoint.")
[email protected](path="/list")
+async def list_tasks( # noqa: PLR0913
+ pagination: Annotated[Pagination, Body(default_factory=Pagination)],
+ task_type_id: Annotated[int | None, Body(description="Filter by task type id.")] = None,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting small helper functions for IN-clause construction, range parsing, WHERE-clause assembly, dataset-status subquery, and task-enrichment to make `list_tasks` shorter and easier to follow.
You can keep the new behavior but reduce complexity by extracting a few focused helpers and reusing them.
### 1. Extract a generic `IN` clause builder
You repeat `",".join(...)` and f-string interpolation for IDs and names. A tiny helper keeps SQL construction consistent and reduces visual noise:
```python
def _in_clause(values: list[int | str]) -> str:
# Callers are responsible for correctness of values (already sanitized IDs or enums)
return ", ".join(str(v) for v in values)
```
Usage:
```python
task_ids_str = _in_clause(tasks.keys())
did_list = _in_clause([t["did"] for t in tasks.values()])
basic_inputs_str = _in_clause([f"'{i}'" for i in BASIC_TASK_INPUTS])
qualities_str = _in_clause([f"'{q}'" for q in QUALITIES_TO_SHOW])
```
This shrinks the `list_tasks` body and centralizes the `IN (...)` formatting.
### 2. Split range parsing from quality SQL rendering
`_quality_clause` currently parses and renders SQL. Splitting the concerns makes both parts easier to read and test.
```python
def _parse_integer_range(range_: str | None) -> tuple[str, str | None] | None:
if not range_:
return None
match = re.match(integer_range_regex, range_)
if not match:
msg = f"`range_` not a valid range: {range_}"
raise ValueError(msg)
start, end = match.groups()
end_num = end[2:] if end else None # strip ".."
return start, end_num
```
Then a generic quality clause builder:
```python
def _quality_clause(quality: str, range_: str | None) -> str:
parsed = _parse_integer_range(range_)
if not parsed:
return ""
start, end = parsed
value_expr = f"`value` BETWEEN {start} AND {end}" if end else f"`value`={start}"
return f"""
AND t.`task_id` IN (
SELECT ti.`task_id` FROM task_inputs ti
WHERE ti.`input`='source_data' AND ti.`value` IN (
SELECT `data` FROM data_quality
WHERE `quality`='{quality}' AND {value_expr}
)
)
"""
```
This leaves `list_tasks` with just the high-level mapping:
```python
where_number_instances = _quality_clause("NumberOfInstances", number_instances)
...
```
### 3. Extract WHERE-clause assembly into a helper
The top of `list_tasks` builds many related conditions inline. Move that to a dedicated function that returns both fragments and the parameter dict:
```python
def _build_task_where_clauses(
status: TaskStatusFilter,
task_type_id: int | None,
tag: str | None,
data_tag: str | None,
task_id: list[int] | None,
data_id: list[int] | None,
data_name: str | None,
number_instances: str | None,
number_features: str | None,
number_classes: str | None,
number_missing_values: str | None,
) -> tuple[str, dict[str, Any]]:
if status == TaskStatusFilter.ALL:
where_status = ""
else:
where_status = f"AND IFNULL(ds.`status`, 'in_preparation') = '{status}'"
where_type = "" if task_type_id is None else "AND t.`ttid` = :task_type_id"
where_tag = "" if tag is None else \
"AND t.`task_id` IN (SELECT `id` FROM task_tag WHERE `tag` = :tag)"
where_data_tag = "" if data_tag is None else \
"AND d.`did` IN (SELECT `id` FROM dataset_tag WHERE `tag` = :data_tag)"
where_task_id = "" if not task_id else f"AND t.`task_id` IN ({_in_clause(task_id)})"
where_data_id = "" if not data_id else f"AND d.`did` IN ({_in_clause(data_id)})"
where_data_name = "" if data_name is None else "AND d.`name` = :data_name"
where_number_instances = _quality_clause("NumberOfInstances", number_instances)
where_number_features = _quality_clause("NumberOfFeatures", number_features)
where_number_classes = _quality_clause("NumberOfClasses", number_classes)
where_number_missing_values = _quality_clause(
"NumberOfMissingValues", number_missing_values
)
where_sql = (
where_status
+ where_type
+ where_tag
+ where_data_tag
+ where_task_id
+ where_data_id
+ where_data_name
+ where_number_instances
+ where_number_features
+ where_number_classes
+ where_number_missing_values
)
params = {
"task_type_id": task_type_id,
"tag": tag,
"data_tag": data_tag,
"data_name": data_name,
}
return where_sql, params
```
Then `list_tasks` reduces to:
```python
where_sql, params = _build_task_where_clauses(
status, task_type_id, tag, data_tag, task_id, data_id,
data_name, number_instances, number_features,
number_classes, number_missing_values,
)
query = text(f"""
SELECT ...
FROM ...
WHERE 1=1
{where_sql}
GROUP BY ...
LIMIT {pagination.limit} OFFSET {pagination.offset}
""")
result = await expdb.execute(query, parameters=params)
```
### 4. Extract the dataset-status subquery
Moving the status subquery out of the main query makes the main SQL much easier to read:
```python
LATEST_DATASET_STATUS_SUBQUERY = """
SELECT ds1.did, ds1.status
FROM dataset_status ds1
WHERE ds1.status_date = (
SELECT MAX(ds2.status_date)
FROM dataset_status ds2
WHERE ds1.did = ds2.did
)
"""
```
Then in `list_tasks`:
```python
query = text(f"""
SELECT ...
FROM task t
...
LEFT JOIN ({LATEST_DATASET_STATUS_SUBQUERY}) ds ON ds.`did` = d.`did`
WHERE 1=1
{where_sql}
...
""")
```
### 5. Unify enrichment of `tasks` (inputs/qualities/tags)
You can use a generic helper for the “append or init list” pattern to avoid repetition:
```python
def _append_task_attr(
tasks: dict[int, dict[str, Any]],
task_id: int,
key: str,
value: Any,
) -> None:
tasks[task_id].setdefault(key, []).append(value)
```
Then:
```python
# inputs
for row in inputs_result.all():
_append_task_attr(
tasks,
row.task_id,
"input",
{"name": row.input, "value": row.value},
)
# qualities
for row in qualities_result.all():
tid = did_to_task_id.get(row.data)
if tid is not None:
_append_task_attr(
tasks,
tid,
"quality",
{"name": row.quality, "value": str(row.value)},
)
# tags
for row in tags_result.all():
_append_task_attr(tasks, row.id, "tag", row.tag)
```
And a single “ensure keys” helper:
```python
def _ensure_task_keys(task: dict[str, Any], keys: list[str]) -> None:
for key in keys:
task.setdefault(key, [])
```
Usage:
```python
for task in tasks.values():
_ensure_task_keys(task, ["input", "quality", "tag"])
```
---
These extractions keep all current behavior and SQL intact, but make `list_tasks` mostly a high‑level orchestration over small, single-purpose helpers, which should address the complexity concerns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+36
to
+42
| async def test_list_tasks_filter_tag(py_api: httpx.AsyncClient) -> None: | ||
| """Filter by tag returns only tasks with that tag.""" | ||
| response = await py_api.post("/tasks/list", json={"tag": "OpenML100"}) | ||
| assert response.status_code == HTTPStatus.OK | ||
| tasks = response.json() | ||
| assert len(tasks) > 0 | ||
| assert all("OpenML100" in t["tag"] for t in tasks) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for the remaining filters (data_tag, task_id, data_id, data_name, and other quality ranges).
Current tests exercise task_type_id, tag, pagination, and number_instances, but not the other supported filters: data_tag, task_id, data_id, data_name, number_features, number_classes, and number_missing_values. This leaves several WHERE-clause branches untested.
Consider adding targeted tests such as:
data_tag: only tasks whose dataset has the given tag are returned.task_id: multiple IDs return exactly those tasks (and correct ordering/uniqueness if relevant).data_id: only tasks for the specified dataset(s) are returned.data_name: verifies expected matching behavior (case-(in)sensitivity as intended).- One or more quality ranges using
number_featuresornumber_classesto exercise_quality_clausebeyondNumberOfInstances.
Suggested implementation:
async def test_list_tasks_filter_type(py_api: httpx.AsyncClient) -> None:
"""Filter by task_type_id returns only tasks of that type."""
response = await py_api.post("/tasks/list", json={"task_type_id": 1})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert all(t["task_type_id"] == 1 for t in tasks)
async def test_list_tasks_default(py_api: httpx.AsyncClient) -> None:
"""Default call returns active tasks with correct shape."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert isinstance(tasks, list)
assert len(tasks) > 0
# verify shape of first task
task = tasks[0]
async def test_list_tasks_filter_data_tag(py_api: httpx.AsyncClient) -> None:
"""Filter by data_tag returns only tasks whose dataset has that tag."""
# Get a tag actually present on at least one dataset
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
# Find first task that has at least one data tag
tagged_task = next(
t for t in tasks if t.get("data_tags") is not None and len(t.get("data_tags")) > 0
)
data_tag = tagged_task["data_tags"][0]
response = await py_api.post("/tasks/list", json={"data_tag": data_tag})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
data_tag in t.get("data_tags", []) for t in filtered
), "All returned tasks should have the dataset tag used for filtering"
async def test_list_tasks_filter_task_id(py_api: httpx.AsyncClient) -> None:
"""Filter by task_id returns exactly the requested tasks."""
# Discover a small set of valid task IDs from the default listing
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) >= 2
requested_ids = sorted(t["task_id"] for t in tasks[:3])
response = await py_api.post("/tasks/list", json={"task_id": requested_ids})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
returned_ids = sorted(t["task_id"] for t in filtered)
assert set(returned_ids) == set(
requested_ids
), "Filtering by task_id should return exactly the requested tasks"
# If the API defines a particular ordering, assert on that here as well
# (e.g. sorted by task_id). The current check ensures no duplicates.
assert len(returned_ids) == len(set(returned_ids))
async def test_list_tasks_filter_data_id(py_api: httpx.AsyncClient) -> None:
"""Filter by data_id returns only tasks from those datasets."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) > 0
# Take a couple of distinct data_ids from the available tasks
data_ids = []
for t in tasks:
if t["data_id"] not in data_ids:
data_ids.append(t["data_id"])
if len(data_ids) == 2:
break
response = await py_api.post("/tasks/list", json={"data_id": data_ids})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t["data_id"] in data_ids for t in filtered
), "All returned tasks should belong to one of the requested data_ids"
async def test_list_tasks_filter_data_name(py_api: httpx.AsyncClient) -> None:
"""Filter by data_name returns only tasks whose dataset has that name."""
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
assert len(tasks) > 0
data_name = tasks[0]["data_name"]
# Use the exact name observed in the listing
response = await py_api.post("/tasks/list", json={"data_name": data_name})
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t["data_name"] == data_name for t in filtered
), "All returned tasks should have the requested data_name"
async def test_list_tasks_filter_number_features_range(py_api: httpx.AsyncClient) -> None:
"""Filter by number_features quality range returns tasks within that range."""
# Probe existing tasks to discover a concrete number_features value
response = await py_api.post("/tasks/list", json={})
assert response.status_code == HTTPStatus.OK
tasks = response.json()
# Find a task with a defined number_features value
task_with_features = next(
t for t in tasks if t.get("number_features") is not None
)
num_features = task_with_features["number_features"]
# Build a narrow range around that concrete value to exercise the quality clause
payload = {
"number_features": {
"min": num_features,
"max": num_features,
}
}
response = await py_api.post("/tasks/list", json=payload)
assert response.status_code == HTTPStatus.OK
filtered = response.json()
assert len(filtered) > 0
assert all(
t.get("number_features") is not None
and payload["number_features"]["min"]
<= t["number_features"]
<= payload["number_features"]["max"]
for t in filtered
), "All returned tasks should have number_features within the requested range"These tests assume:
- Response objects include
data_tags(a list of strings on each task),task_id,data_id,data_name, andnumber_features. - The
/tasks/listrequest model accepts:data_tagas a single string.task_idanddata_idas lists of integers.data_nameas a string.- Quality ranges in the form
{"number_features": {"min": ..., "max": ...}}.
To fully align with your existing API:
- Adjust the field names (
data_tagsvs.dataset_tags, etc.) in the assertions to match the actual task schema. - Align the request payload shapes with whatever is already used in the existing
number_instancestest (e.g., if it usesnumber_features_min/number_features_maxinstead of a nested dict, mirror that here). - If
task_id/data_idonly accept a single integer instead of a list, modifyrequested_ids/data_idsaccordingly and assert on a single returned task. - If data-name filtering is case-insensitive, extend
test_list_tasks_filter_data_nameto issue a request withdata_namein a different case and assert that the same tasks are returned.
src/routers/openml/tasks.py
Outdated
|
|
||
| @router.post(path="/list", description="Provided for convenience, same as `GET` endpoint.") | ||
| @router.get(path="/list") | ||
| async def list_tasks( # noqa: PLR0913 |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting small helper functions for IN-clause construction, range parsing, WHERE-clause assembly, dataset-status subquery, and task-enrichment to make list_tasks shorter and easier to follow.
You can keep the new behavior but reduce complexity by extracting a few focused helpers and reusing them.
1. Extract a generic IN clause builder
You repeat ",".join(...) and f-string interpolation for IDs and names. A tiny helper keeps SQL construction consistent and reduces visual noise:
def _in_clause(values: list[int | str]) -> str:
# Callers are responsible for correctness of values (already sanitized IDs or enums)
return ", ".join(str(v) for v in values)Usage:
task_ids_str = _in_clause(tasks.keys())
did_list = _in_clause([t["did"] for t in tasks.values()])
basic_inputs_str = _in_clause([f"'{i}'" for i in BASIC_TASK_INPUTS])
qualities_str = _in_clause([f"'{q}'" for q in QUALITIES_TO_SHOW])This shrinks the list_tasks body and centralizes the IN (...) formatting.
2. Split range parsing from quality SQL rendering
_quality_clause currently parses and renders SQL. Splitting the concerns makes both parts easier to read and test.
def _parse_integer_range(range_: str | None) -> tuple[str, str | None] | None:
if not range_:
return None
match = re.match(integer_range_regex, range_)
if not match:
msg = f"`range_` not a valid range: {range_}"
raise ValueError(msg)
start, end = match.groups()
end_num = end[2:] if end else None # strip ".."
return start, end_numThen a generic quality clause builder:
def _quality_clause(quality: str, range_: str | None) -> str:
parsed = _parse_integer_range(range_)
if not parsed:
return ""
start, end = parsed
value_expr = f"`value` BETWEEN {start} AND {end}" if end else f"`value`={start}"
return f"""
AND t.`task_id` IN (
SELECT ti.`task_id` FROM task_inputs ti
WHERE ti.`input`='source_data' AND ti.`value` IN (
SELECT `data` FROM data_quality
WHERE `quality`='{quality}' AND {value_expr}
)
)
"""This leaves list_tasks with just the high-level mapping:
where_number_instances = _quality_clause("NumberOfInstances", number_instances)
...3. Extract WHERE-clause assembly into a helper
The top of list_tasks builds many related conditions inline. Move that to a dedicated function that returns both fragments and the parameter dict:
def _build_task_where_clauses(
status: TaskStatusFilter,
task_type_id: int | None,
tag: str | None,
data_tag: str | None,
task_id: list[int] | None,
data_id: list[int] | None,
data_name: str | None,
number_instances: str | None,
number_features: str | None,
number_classes: str | None,
number_missing_values: str | None,
) -> tuple[str, dict[str, Any]]:
if status == TaskStatusFilter.ALL:
where_status = ""
else:
where_status = f"AND IFNULL(ds.`status`, 'in_preparation') = '{status}'"
where_type = "" if task_type_id is None else "AND t.`ttid` = :task_type_id"
where_tag = "" if tag is None else \
"AND t.`task_id` IN (SELECT `id` FROM task_tag WHERE `tag` = :tag)"
where_data_tag = "" if data_tag is None else \
"AND d.`did` IN (SELECT `id` FROM dataset_tag WHERE `tag` = :data_tag)"
where_task_id = "" if not task_id else f"AND t.`task_id` IN ({_in_clause(task_id)})"
where_data_id = "" if not data_id else f"AND d.`did` IN ({_in_clause(data_id)})"
where_data_name = "" if data_name is None else "AND d.`name` = :data_name"
where_number_instances = _quality_clause("NumberOfInstances", number_instances)
where_number_features = _quality_clause("NumberOfFeatures", number_features)
where_number_classes = _quality_clause("NumberOfClasses", number_classes)
where_number_missing_values = _quality_clause(
"NumberOfMissingValues", number_missing_values
)
where_sql = (
where_status
+ where_type
+ where_tag
+ where_data_tag
+ where_task_id
+ where_data_id
+ where_data_name
+ where_number_instances
+ where_number_features
+ where_number_classes
+ where_number_missing_values
)
params = {
"task_type_id": task_type_id,
"tag": tag,
"data_tag": data_tag,
"data_name": data_name,
}
return where_sql, paramsThen list_tasks reduces to:
where_sql, params = _build_task_where_clauses(
status, task_type_id, tag, data_tag, task_id, data_id,
data_name, number_instances, number_features,
number_classes, number_missing_values,
)
query = text(f"""
SELECT ...
FROM ...
WHERE 1=1
{where_sql}
GROUP BY ...
LIMIT {pagination.limit} OFFSET {pagination.offset}
""")
result = await expdb.execute(query, parameters=params)4. Extract the dataset-status subquery
Moving the status subquery out of the main query makes the main SQL much easier to read:
LATEST_DATASET_STATUS_SUBQUERY = """
SELECT ds1.did, ds1.status
FROM dataset_status ds1
WHERE ds1.status_date = (
SELECT MAX(ds2.status_date)
FROM dataset_status ds2
WHERE ds1.did = ds2.did
)
"""Then in list_tasks:
query = text(f"""
SELECT ...
FROM task t
...
LEFT JOIN ({LATEST_DATASET_STATUS_SUBQUERY}) ds ON ds.`did` = d.`did`
WHERE 1=1
{where_sql}
...
""")5. Unify enrichment of tasks (inputs/qualities/tags)
You can use a generic helper for the “append or init list” pattern to avoid repetition:
def _append_task_attr(
tasks: dict[int, dict[str, Any]],
task_id: int,
key: str,
value: Any,
) -> None:
tasks[task_id].setdefault(key, []).append(value)Then:
# inputs
for row in inputs_result.all():
_append_task_attr(
tasks,
row.task_id,
"input",
{"name": row.input, "value": row.value},
)
# qualities
for row in qualities_result.all():
tid = did_to_task_id.get(row.data)
if tid is not None:
_append_task_attr(
tasks,
tid,
"quality",
{"name": row.quality, "value": str(row.value)},
)
# tags
for row in tags_result.all():
_append_task_attr(tasks, row.id, "tag", row.tag)And a single “ensure keys” helper:
def _ensure_task_keys(task: dict[str, Any], keys: list[str]) -> None:
for key in keys:
task.setdefault(key, [])Usage:
for task in tasks.values():
_ensure_task_keys(task, ["input", "quality", "tag"])These extractions keep all current behavior and SQL intact, but make list_tasks mostly a high‑level orchestration over small, single-purpose helpers, which should address the complexity concerns.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/routers/openml/task_test.py (3)
7-25: Assert the defaultactivecontract.This currently checks shape only, so it still passes if the endpoint stops applying the default status filter. Please assert that every returned task has
status == "active".🧪 Small test hardening
tasks = response.json() assert isinstance(tasks, list) assert len(tasks) > 0 + assert all(task["status"] == "active" for task in tasks) # verify shape of first task🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_test.py` around lines 7 - 25, The test test_list_tasks_default should enforce the default "active" contract: after fetching response from the "/tasks/list" endpoint and validating tasks is a non-empty list (as shown), iterate over the returned tasks (e.g., for task in tasks) and assert task["status"] == "active" for every item; add this assertion to the test_list_tasks_default function so the endpoint is required to apply the default active status filter.
28-33: Avoid a vacuous pass in the type-filter test.
all(t["task_type_id"] == 1 for t in tasks)is true for[], so a regression that returns no rows still passes.🧪 Small test hardening
assert response.status_code == HTTPStatus.OK tasks = response.json() + assert len(tasks) > 0 assert all(t["task_type_id"] == 1 for t in tasks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_test.py` around lines 28 - 33, The test test_list_tasks_filter_type currently uses all(...) which vacuously passes for an empty list; modify the test to first assert that the response contains at least one task (e.g., assert tasks), then keep the existing check that all(t["task_type_id"] == 1 for t in tasks). Locate the test function test_list_tasks_filter_type and the POST to "/tasks/list" (json={"task_type_id": 1}) and add a non-empty assertion before the all(...) assertion so the test fails if no rows are returned.
65-72: Check the filtered values, not just that something came back.
assert len(response.json()) > 0does not provenumber_instancesis working. This test would still pass if the filter were ignored completely.🧪 Stronger assertion
) assert response.status_code == HTTPStatus.OK - assert len(response.json()) > 0 + tasks = response.json() + assert len(tasks) > 0 + for task in tasks: + qualities = {q["name"]: q["value"] for q in task["quality"]} + assert 100 <= float(qualities["NumberOfInstances"]) <= 1000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_test.py` around lines 65 - 72, The test test_list_tasks_number_instances_range currently only asserts that something was returned; update it to validate the filter by parsing response.json() (ensure it's a list), then for each returned task object (from the POST to "/tasks/list" with json {"number_instances":"100..1000"}) extract the numeric value (e.g., task["number_instances"] or task["dataset"]["number_instances"] depending on payload shape), coerce it to int, and assert each value is between 100 and 1000 inclusive so the test fails if the filter is ignored or misapplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/tasks.py`:
- Around line 277-307: The SELECT query built into the variable query currently
lacks an ORDER BY, making pagination via pagination.limit and pagination.offset
unstable; modify the SQL assembled in query (the multi-line text(...) that
references status_subquery and the various {where_*} clauses) to append a
deterministic ORDER BY before the LIMIT/OFFSET — e.g. order by a stable unique
column such as t.`task_id` (and secondary columns like d.`did` if needed) so
results are consistently sorted across pages.
- Around line 360-368: The current did_to_task_id mapping only keeps one task_id
per dataset, so replace it with a mapping from dataset id to a list of task ids
(e.g., build did_to_task_ids from tasks by collecting tid into lists for each
t["did"]), then when iterating qualities_result.all() look up the list
(did_to_task_ids.get(row.data, [])) and append the quality entry to each
corresponding tasks[tid] (using the same setdefault("quality", []) logic) so
every task sharing the same source_data receives the quality rows.
- Around line 218-233: The current combined handler list_tasks is using
Body(...) for all parameters which breaks GET semantics; split into two
endpoints: keep the existing POST /list handler (function list_tasks_post) that
accepts Pagination, task_type_id, tag, etc. from Body(...) and keep using
Depends(expdb_connection) for expdb, and create a new GET /list handler
(function list_tasks_get) that accepts the same parameters from Query(...) (or
via a Depends-based query model) instead of Body so query-string filters
populate correctly; ensure both handlers delegate shared logic to a common
helper (e.g., _list_tasks_impl) that takes the normalized args (Pagination,
TaskStatusFilter, lists like task_id/data_id, number_* ranges, expdb) to avoid
duplicating business logic.
---
Nitpick comments:
In `@tests/routers/openml/task_test.py`:
- Around line 7-25: The test test_list_tasks_default should enforce the default
"active" contract: after fetching response from the "/tasks/list" endpoint and
validating tasks is a non-empty list (as shown), iterate over the returned tasks
(e.g., for task in tasks) and assert task["status"] == "active" for every item;
add this assertion to the test_list_tasks_default function so the endpoint is
required to apply the default active status filter.
- Around line 28-33: The test test_list_tasks_filter_type currently uses
all(...) which vacuously passes for an empty list; modify the test to first
assert that the response contains at least one task (e.g., assert tasks), then
keep the existing check that all(t["task_type_id"] == 1 for t in tasks). Locate
the test function test_list_tasks_filter_type and the POST to "/tasks/list"
(json={"task_type_id": 1}) and add a non-empty assertion before the all(...)
assertion so the test fails if no rows are returned.
- Around line 65-72: The test test_list_tasks_number_instances_range currently
only asserts that something was returned; update it to validate the filter by
parsing response.json() (ensure it's a list), then for each returned task object
(from the POST to "/tasks/list" with json {"number_instances":"100..1000"})
extract the numeric value (e.g., task["number_instances"] or
task["dataset"]["number_instances"] depending on payload shape), coerce it to
int, and assert each value is between 100 and 1000 inclusive so the test fails
if the filter is ignored or misapplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 318531a5-1859-48cb-9afb-9606ff2ddb84
📒 Files selected for processing (2)
src/routers/openml/tasks.pytests/routers/openml/task_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/routers/openml/task_test.py (1)
92-96: Test doesn't verify GET with query parameters.This test only exercises GET without parameters, which works because all endpoint parameters have defaults. It doesn't verify that GET with query parameters (e.g.,
?task_type_id=1) actually filters results. Given theBody(...)parameter binding issue in the endpoint, such a test would currently fail—which would help surface the problem.Consider adding a test case like:
async def test_list_tasks_get_with_query_params(py_api: httpx.AsyncClient) -> None: """GET /tasks/list with query params should filter results.""" response = await py_api.get("/tasks/list", params={"task_type_id": 1}) assert response.status_code == HTTPStatus.OK tasks = response.json() assert all(t["task_type_id"] == 1 for t in tasks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_test.py` around lines 92 - 96, Add a new test that calls the same endpoint as test_list_tasks_get but supplies query parameters to ensure filtering works: create async def test_list_tasks_get_with_query_params(py_api: httpx.AsyncClient) that performs py_api.get("/tasks/list", params={"task_type_id": 1}), asserts response.status_code == HTTPStatus.OK, parses response.json() into tasks, and asserts all returned items have t["task_type_id"] == 1; reference the existing test_list_tasks_get to place the new test nearby and validate the query-parameter behavior for the /tasks/list endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/routers/openml/task_test.py`:
- Around line 82-89: The test test_list_tasks_no_results should assert
error["code"] equals the exact string instead of using substring containment;
update the assertion that currently checks "372" in error["code"] to a direct
equality check comparing error["code"] to the full expected string (use the same
serialized form used elsewhere, i.e., str(exc.code) format) so that
error["code"] is asserted with equality rather than containment.
---
Nitpick comments:
In `@tests/routers/openml/task_test.py`:
- Around line 92-96: Add a new test that calls the same endpoint as
test_list_tasks_get but supplies query parameters to ensure filtering works:
create async def test_list_tasks_get_with_query_params(py_api:
httpx.AsyncClient) that performs py_api.get("/tasks/list",
params={"task_type_id": 1}), asserts response.status_code == HTTPStatus.OK,
parses response.json() into tasks, and asserts all returned items have
t["task_type_id"] == 1; reference the existing test_list_tasks_get to place the
new test nearby and validate the query-parameter behavior for the /tasks/list
endpoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 946d27de-85ae-4812-a074-8a3f092dadea
📒 Files selected for processing (2)
src/routers/openml/tasks.pytests/routers/openml/task_test.py
PGijsbers
requested changes
Mar 26, 2026
Contributor
PGijsbers
left a comment
PGijsbers
left a comment
There was a problem hiding this comment.
Hi! Thanks for the contribution! While reviewing this PR, I noticed some issues with the way input is processed in the query and generally the readability of the function. I had some ideas on how to improve this, but realised that it would be very similar for the datasets list function. So I went ahead and refactored that to get a better idea:
https://github.com/openml/server-api/pull/286/changes
Please update the implementation similarly, using parameters for the queries (as opposed to string formatting) and using a consistent pattern on when building up the clauses.
Second, make sure to add a test which compares the output of the Python API to that of the PHP API. Cover a few different use cases (potentially using hypothesis, but make sure to mark it as slow if you do).
After that, I'll have a closer look.
4 tasks
Contributor
Author
|
Hi, thanks for detailed feedback, I will look at the reference and understand the pattern then I'll make changes and write more tests. |
Contributor
|
I updated the test file structure in #295. We now use a file per endpoint. So to avoid the merge conflict, just move the new task list tests to a separate file. |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/routers/openml/migration/tasks_migration_test.py" line_range="111-117" />
<code_context>
+
+# Filter combos: (php_path_params, python_body_extras)
+# PHP uses path-based filter keys (e.g. "type"), Python uses JSON body keys (e.g. "task_type_id")
+_FILTER_COMBOS: list[tuple[dict[str, Any], dict[str, Any]]] = [
+ ({"type": 1}, {"task_type_id": 1}), # by task type
+ ({"tag": "OpenML100"}, {"tag": "OpenML100"}), # by tag
+ ({"type": 1, "tag": "OpenML100"}, {"task_type_id": 1, "tag": "OpenML100"}), # combined
+]
+
+_FILTER_IDS = ["type", "tag", "type_and_tag"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider extending migration filter combos to cover additional filters like data_name/data_id and quality-based ranges to better prove parity with the PHP API.
Right now these tests only cover `type` and `tag` (and their combination). Since the endpoint also supports filters like `data_name`, `data_id`, `data_tag`, and the `number_*` quality ranges, please add a few representative combinations using those as well to validate parity for dataset- and quality-based filtering.
Suggested implementation:
```python
# Filter combos: (php_path_params, python_body_extras)
# PHP uses path-based filter keys (e.g. "type"), Python uses JSON body keys (e.g. "task_type_id")
_FILTER_COMBOS: list[tuple[dict[str, Any], dict[str, Any]]] = [
# by task type
({"type": 1}, {"task_type_id": 1}),
# by task tag
({"tag": "OpenML100"}, {"tag": "OpenML100"}),
# combined task type + tag
({"type": 1, "tag": "OpenML100"}, {"task_type_id": 1, "tag": "OpenML100"}),
# by dataset name
({"data_name": "iris"}, {"data_name": "iris"}),
# by dataset id
({"data_id": 61}, {"data_id": 61}),
# by dataset tag
({"data_tag": "study_14"}, {"data_tag": "study_14"}),
# by quality-based numeric filter (number of instances)
({"number_instances": 500}, {"number_instances": 500}),
# combined dataset- and quality-based filtering
({"data_id": 61, "number_instances": 500}, {"data_id": 61, "number_instances": 500}),
]
_FILTER_IDS = [
"type",
"tag",
"type_and_tag",
"data_name",
"data_id",
"data_tag",
"number_instances",
"data_and_quality",
]
import deepdiff
import httpx
ignore_order=True,
)
assert not differences
```
This update assumes that:
- The Python task-list endpoint accepts `data_name`, `data_id`, `data_tag`, and `number_instances` in the JSON body using the same key names as the PHP query parameters.
- `_FILTER_COMBOS` and `_FILTER_IDS` are used together in a parametrized test (e.g. `ids=_FILTER_IDS`); if the test structure differs, you may need to align the new IDs with how they are referenced.
If your API expects a different shape for the quality-based filters (e.g. explicit min/max ranges instead of a single integer), adjust the `{"number_instances": 500}` entries to match the actual request format on both the PHP and Python sides (for example, `{"number_instances": "1-500"}` vs. `{"number_instances": {"min": 1, "max": 500}}`).
</issue_to_address>
### Comment 2
<location path="tests/routers/openml/task_list_test.py" line_range="111-117" />
<code_context>
+ assert len(response.json()) <= limit
+
+
+async def test_list_tasks_pagination_order_stable(py_api: httpx.AsyncClient) -> None:
+ """Results are ordered by task_id — consecutive pages are in ascending order."""
+ r1 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 0}})
+ r2 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 5}})
+ ids1 = [t["task_id"] for t in r1.json()]
+ ids2 = [t["task_id"] for t in r2.json()]
+ assert max(ids1) < min(ids2)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the pagination ordering test to be more robust and explicit about global ordering guarantees.
This assertion assumes both pages are non-empty and that there are at least `limit * 2` tasks. To make the test more robust, consider skipping the `max(ids1) < min(ids2)` check if either page is empty, and also asserting that IDs are strictly increasing within each page. That more directly verifies the `ORDER BY task_id` guarantee while reducing false negatives if test data volume changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+111
to
+117
| _FILTER_COMBOS: list[tuple[dict[str, Any], dict[str, Any]]] = [ | ||
| ({"type": 1}, {"task_type_id": 1}), # by task type | ||
| ({"tag": "OpenML100"}, {"tag": "OpenML100"}), # by tag | ||
| ({"type": 1, "tag": "OpenML100"}, {"task_type_id": 1, "tag": "OpenML100"}), # combined | ||
| ] | ||
|
|
||
| _FILTER_IDS = ["type", "tag", "type_and_tag"] |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider extending migration filter combos to cover additional filters like data_name/data_id and quality-based ranges to better prove parity with the PHP API.
Right now these tests only cover type and tag (and their combination). Since the endpoint also supports filters like data_name, data_id, data_tag, and the number_* quality ranges, please add a few representative combinations using those as well to validate parity for dataset- and quality-based filtering.
Suggested implementation:
# Filter combos: (php_path_params, python_body_extras)
# PHP uses path-based filter keys (e.g. "type"), Python uses JSON body keys (e.g. "task_type_id")
_FILTER_COMBOS: list[tuple[dict[str, Any], dict[str, Any]]] = [
# by task type
({"type": 1}, {"task_type_id": 1}),
# by task tag
({"tag": "OpenML100"}, {"tag": "OpenML100"}),
# combined task type + tag
({"type": 1, "tag": "OpenML100"}, {"task_type_id": 1, "tag": "OpenML100"}),
# by dataset name
({"data_name": "iris"}, {"data_name": "iris"}),
# by dataset id
({"data_id": 61}, {"data_id": 61}),
# by dataset tag
({"data_tag": "study_14"}, {"data_tag": "study_14"}),
# by quality-based numeric filter (number of instances)
({"number_instances": 500}, {"number_instances": 500}),
# combined dataset- and quality-based filtering
({"data_id": 61, "number_instances": 500}, {"data_id": 61, "number_instances": 500}),
]
_FILTER_IDS = [
"type",
"tag",
"type_and_tag",
"data_name",
"data_id",
"data_tag",
"number_instances",
"data_and_quality",
]
import deepdiff
import httpx
ignore_order=True,
)
assert not differencesThis update assumes that:
- The Python task-list endpoint accepts
data_name,data_id,data_tag, andnumber_instancesin the JSON body using the same key names as the PHP query parameters. _FILTER_COMBOSand_FILTER_IDSare used together in a parametrized test (e.g.ids=_FILTER_IDS); if the test structure differs, you may need to align the new IDs with how they are referenced.
If your API expects a different shape for the quality-based filters (e.g. explicit min/max ranges instead of a single integer), adjust the {"number_instances": 500} entries to match the actual request format on both the PHP and Python sides (for example, {"number_instances": "1-500"} vs. {"number_instances": {"min": 1, "max": 500}}).
Comment on lines
+111
to
+117
| async def test_list_tasks_pagination_order_stable(py_api: httpx.AsyncClient) -> None: | ||
| """Results are ordered by task_id — consecutive pages are in ascending order.""" | ||
| r1 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 0}}) | ||
| r2 = await py_api.post("/tasks/list", json={"pagination": {"limit": 5, "offset": 5}}) | ||
| ids1 = [t["task_id"] for t in r1.json()] | ||
| ids2 = [t["task_id"] for t in r2.json()] | ||
| assert max(ids1) < min(ids2) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen the pagination ordering test to be more robust and explicit about global ordering guarantees.
This assertion assumes both pages are non-empty and that there are at least limit * 2 tasks. To make the test more robust, consider skipping the max(ids1) < min(ids2) check if either page is empty, and also asserting that IDs are strictly increasing within each page. That more directly verifies the ORDER BY task_id guarantee while reducing false negatives if test data volume changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/routers/openml/task_list_test.py (2)
130-133: Make the range test fail whenNumberOfInstancesis missing.The filter itself only returns tasks whose source dataset has a
NumberOfInstancesrow, so theifmakes this pass even if response enrichment drops that quality entirely.💡 Tighten the assertion
for task in tasks: qualities = {q["name"]: q["value"] for q in task["quality"]} - if "NumberOfInstances" in qualities: - assert min_instances <= float(qualities["NumberOfInstances"]) <= max_instances + assert "NumberOfInstances" in qualities + assert min_instances <= float(qualities["NumberOfInstances"]) <= max_instances🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_list_test.py` around lines 130 - 133, The loop over tasks currently skips tests when "NumberOfInstances" is missing; change it so the test fails when that key is absent by replacing the conditional with an assertion: for each task in tasks compute qualities = {q["name"]: q["value"] for q in task["quality"]}, assert "NumberOfInstances" in qualities, then assert min_instances <= float(qualities["NumberOfInstances"]) <= max_instances so missing or out-of-range values both fail (use the same variable names tasks, qualities, NumberOfInstances, min_instances, max_instances).
32-36: Add one filtered GET case.This only proves the empty-body GET path. Since
/tasks/listintentionally supports the same body-backed filters on GET and POST here, add one filtered GET assertion so a regression in the GET wiring doesn't slip past while POST keeps passing. Based on learnings: in openml/server-api, GET/tasks/listand POST/tasks/listintentionally share a single handler withBody(...)parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/task_list_test.py` around lines 32 - 36, The test only verifies an unfiltered GET; add a filtered GET assertion to ensure GET supports the same body-backed filters as POST. Update or add a test (near test_list_tasks_get) to call py_api.get("/tasks/list", params=...) with a simple filter key/value used by the suite (for example a known task property used in POST tests), assert HTTPStatus.OK, assert the response is a list, and assert every returned item matches the filter (e.g., item["<filter_field>"] == "<expected_value>"); this ensures the GET query-param wiring for the /tasks/list handler is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/tasks.py`:
- Around line 266-272: The filter logic treats empty lists as falsy and
therefore skips filtering; change the checks around task_id and data_id (and the
similar block at the other occurrence) to distinguish None from an empty list:
if task_id is None do nothing, if task_id is an empty list append a clause that
yields no matches (e.g. "AND 1=0") to clauses, otherwise append the "AND
t.`task_id` IN :task_ids" clause and set parameters["task_ids"]=task_id; apply
the same pattern for data_id and the duplicated block near the other occurrence
so body-based empty filters return zero results instead of the unfiltered set.
In `@tests/routers/openml/migration/tasks_migration_test.py`:
- Around line 159-160: The test assumes php_response.json()["tasks"]["task"] is
always a list but PHP can collapse single-element arrays into a dict; update the
php_tasks assignment to normalize that value into a list before iterating (e.g.
check the type of php_response.json()["tasks"]["task"] and if it's a dict wrap
it in a list, otherwise keep the list) so parity with py_tasks (which uses
[_normalize_py_task(...) for ...]) remains valid; adjust the variable php_tasks
accordingly where php_response and php_tasks are used.
---
Nitpick comments:
In `@tests/routers/openml/task_list_test.py`:
- Around line 130-133: The loop over tasks currently skips tests when
"NumberOfInstances" is missing; change it so the test fails when that key is
absent by replacing the conditional with an assertion: for each task in tasks
compute qualities = {q["name"]: q["value"] for q in task["quality"]}, assert
"NumberOfInstances" in qualities, then assert min_instances <=
float(qualities["NumberOfInstances"]) <= max_instances so missing or
out-of-range values both fail (use the same variable names tasks, qualities,
NumberOfInstances, min_instances, max_instances).
- Around line 32-36: The test only verifies an unfiltered GET; add a filtered
GET assertion to ensure GET supports the same body-backed filters as POST.
Update or add a test (near test_list_tasks_get) to call
py_api.get("/tasks/list", params=...) with a simple filter key/value used by the
suite (for example a known task property used in POST tests), assert
HTTPStatus.OK, assert the response is a list, and assert every returned item
matches the filter (e.g., item["<filter_field>"] == "<expected_value>"); this
ensures the GET query-param wiring for the /tasks/list handler is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9da7431a-9336-4314-a65d-27dba79e8a93
📒 Files selected for processing (3)
src/routers/openml/tasks.pytests/routers/openml/migration/tasks_migration_test.pytests/routers/openml/task_list_test.py
Comment on lines
+266
to
+272
| if task_id: | ||
| clauses.append("AND t.`task_id` IN :task_ids") | ||
| parameters["task_ids"] = task_id | ||
|
|
||
| if data_id: | ||
| clauses.append("AND d.`did` IN :data_ids") | ||
| parameters["data_ids"] = data_id |
Contributor
There was a problem hiding this comment.
Handle empty ID lists explicitly.
task_id=[] or data_id=[] currently skips these branches because empty lists are falsy, so the request falls back to the unfiltered query and returns tasks instead of zero matches. For body-based filters, that widens the result set unexpectedly.
💡 Minimal fix
- if task_id:
+ if task_id is not None:
+ if not task_id:
+ msg = "No tasks match the search criteria."
+ raise NoResultsError(msg)
clauses.append("AND t.`task_id` IN :task_ids")
parameters["task_ids"] = task_id
- if data_id:
+ if data_id is not None:
+ if not data_id:
+ msg = "No tasks match the search criteria."
+ raise NoResultsError(msg)
clauses.append("AND d.`did` IN :data_ids")
parameters["data_ids"] = data_id- if task_id:
+ if task_id is not None:
main_query = main_query.bindparams(bindparam("task_ids", expanding=True))
- if data_id:
+ if data_id is not None:
main_query = main_query.bindparams(bindparam("data_ids", expanding=True))Also applies to: 321-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routers/openml/tasks.py` around lines 266 - 272, The filter logic treats
empty lists as falsy and therefore skips filtering; change the checks around
task_id and data_id (and the similar block at the other occurrence) to
distinguish None from an empty list: if task_id is None do nothing, if task_id
is an empty list append a clause that yields no matches (e.g. "AND 1=0") to
clauses, otherwise append the "AND t.`task_id` IN :task_ids" clause and set
parameters["task_ids"]=task_id; apply the same pattern for data_id and the
duplicated block near the other occurrence so body-based empty filters return
zero results instead of the unfiltered set.
Comment on lines
+159
to
+160
| php_tasks: list[dict[str, Any]] = php_response.json()["tasks"]["task"] | ||
| py_tasks: list[dict[str, Any]] = [_normalize_py_task(t) for t in py_response.json()] |
Contributor
There was a problem hiding this comment.
Normalize the single-task PHP shape before iterating.
This assumes php_response.json()["tasks"]["task"] is always a list, but the helper docstring above already notes that PHP collapses single-element arrays. A filter that matches exactly one task will turn this into a dict and break the parity check for the wrong reason.
💡 Minimal fix
- php_tasks: list[dict[str, Any]] = php_response.json()["tasks"]["task"]
+ php_tasks_raw = php_response.json()["tasks"]["task"]
+ php_tasks: list[dict[str, Any]] = (
+ php_tasks_raw if isinstance(php_tasks_raw, list) else [php_tasks_raw]
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routers/openml/migration/tasks_migration_test.py` around lines 159 -
160, The test assumes php_response.json()["tasks"]["task"] is always a list but
PHP can collapse single-element arrays into a dict; update the php_tasks
assignment to normalize that value into a list before iterating (e.g. check the
type of php_response.json()["tasks"]["task"] and if it's a dict wrap it in a
list, otherwise keep the list) so parity with py_tasks (which uses
[_normalize_py_task(...) for ...]) remains valid; adjust the variable php_tasks
accordingly where php_response and php_tasks are used.
Contributor
Author
|
Hi! I have implemented the changes you have suggested. quick questions:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements
GET /tasks/listandPOST /tasks/listendpoints, migrating from the PHP API.Fixes: #23
Filters supported
task_type_id,tag,data_tag,status,limit,offset,task_id,data_id,data_name,number_instances,number_features,number_classes,number_missing_valuesImplementation notes
datasets.py— filters via request body, both GET and POST decorators on the same functiondatabase/file needed)active(matches PHP behavior)list_datasetstask,task_type,task_inputs,dataset,dataset_status,data_quality,task_tagChecklist