[ENH] V1 → V2 API Migration - Tasks #1611
Conversation
ported functions to APIv1
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1611 +/- ##
==========================================
+ Coverage 52.04% 52.17% +0.13%
==========================================
Files 36 46 +10
Lines 4333 4546 +213
==========================================
+ Hits 2255 2372 +117
- Misses 2078 2174 +96 ☔ 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.
openml/_api/resources/base.py
Outdated
| # @abstractmethod | ||
| # def list_tasks( | ||
| # self, | ||
| # *, | ||
| # task_type: TaskType | None = None, | ||
| # offset: int | None = None, | ||
| # size: int | None = None, | ||
| # **filters: Any, | ||
| # ): |
There was a problem hiding this comment.
this method should simply be called list
There was a problem hiding this comment.
Replaced the name in TasksV1 (where the function actually exists)
There was a problem hiding this comment.
signatures should be same for all 3 classes as of now: #1611 (review)
There was a problem hiding this comment.
As I said above, in terms of functionality only get matches, else i'd have done that.
There was a problem hiding this comment.
I still don't understand
This reverts commit fd43c48.
for more information, see https://pre-commit.ci
5 participants
Metadata