Conversation
ported functions to APIv1
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1611 +/- ##
==========================================
+ Coverage 52.04% 55.15% +3.11%
==========================================
Files 36 63 +27
Lines 4333 5089 +756
==========================================
+ Hits 2255 2807 +552
- Misses 2078 2282 +204 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
From a high-level review, I noticed a few points that need adjustment:
- Caching can likely be removed from the SDK, since these concerns should be handled by the base client.
- I don't see the
api_contextbeing used intasks/functions, so it's not clear to me how the SDK is actually using the new API interface here. - Instead of moving entire methods out of
tasks/functions.py, it would be better to stick to the goal of minimal SDK changes while enabling v2 support. - API calls should be updated at the specific root functions (for example
_get_task_description,OpenMLTask._download_split). - For listing tasks, please follow the approach discussed in #1575 comment.
for more information, see https://pre-commit.ci
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
I have left some comments, please take a look and make sure the signature of all methods in TasksAPI, TasksV1 and TasksV2 stay same.
for more information, see https://pre-commit.ci
- removing this since it was not part of the sdk previously - some tests fail because of the timeout in stacked PRs - this option can easily be added if needed in future
|
|
||
| return pd.DataFrame.from_dict(tasks, orient="index") | ||
|
|
||
| def _get_estimation_procedure_list(self) -> builtins.list[dict[str, Any]]: |
There was a problem hiding this comment.
@geetu040 We already have implemented this function in the estimation_procedure resource, so I think we don't need it to be here, right?
There was a problem hiding this comment.
Just checked, once we merge your PR we can replace it with the _get_details function yes.
openml/_api/resources/task.py
Outdated
| task_type: TaskType | int | None = None, | ||
| **kwargs: Any, | ||
| ) -> pd.DataFrame: | ||
| raise NotImplementedError(self._not_supported(method="list")) |
There was a problem hiding this comment.
You can just use self._not_supported(method="list")
no need for raise NotImplementedError()
| assert isinstance(procs, list) | ||
| assert len(procs) > 0 | ||
| assert "id" in procs[0] | ||
|
|
There was a problem hiding this comment.
I think you need to add TestTasksV2 class, and also add v1 and v2 matching tests and fallback tests as mentioned here: #1575 (comment)
There was a problem hiding this comment.
I dont need to add TestTasksV2 given those tests are a subset of TestTasksV1. Matching tests are in the shared class.
| task_type: TaskType | int | None = None, | ||
| **kwargs: Any, | ||
| ) -> pd.DataFrame: | ||
| """ |
There was a problem hiding this comment.
@geetu040
Should we all use the same style or not?
for me, I have splitted this this into 3 functions for more readability and less coupling
- list()
- _build_url()
- _parse_list_xml()
There was a problem hiding this comment.
I don't think we need private functions to match each other in terms of nomenclature or functionality as long as we share the same common function names (which we do).
6 participants
Metadata