[ENH] V1 → V2 API Migration - setups#1619
[ENH] V1 → V2 API Migration - setups#1619PGijsbers merged 271 commits intoopenml:mainopenml/openml-python:mainfrom EmanAbdelhaleem:setups-migEmanAbdelhaleem/openml-python:setups-migCopy head branch name to clipboard
Conversation
…into issue1564
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
==========================================
+ Coverage 53.75% 54.46% +0.70%
==========================================
Files 61 61
Lines 5062 5086 +24
==========================================
+ Hits 2721 2770 +49
+ Misses 2341 2316 -25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
some old tests will fail because the cache path has changed
old: tests/files/org/openml/test/tasks/1882
new: tests/files/org/openml/test/api/v1/xml/task/1882
regarding this we can do the following, as i've done in this PR
- move the relevant cache test files to new directory - (reference - setup/1/body.xml)
- create new directory
tests/files/org/openml/test/api/v1/xml/[resource]/[id]/, move thedescription.xmlfile there and rename it tobody.xml - inshort a file
tests/files/org/openml/test/setups/1/description.xmlbecomestests/files/org/openml/test/api/v1/xml/setup/1/body.xml - move other downloadable files from
minioandHTTPClient.downloadaccordingly
- create new directory
- if there are any tests that rely on cache, update them by mocking
Session.request- (reference - test_setup_functions) - update
conftest.pywith new file paths, for now you can manually hardcode the new paths and remove this resource from the code earlier, we can later clean this up - (reference - conftest.py)
geetu040
left a comment
There was a problem hiding this comment.
@EmanAbdelhaleem Thanks for the PR. Nicely done!
I have updated the PR to sync with latest changes in base PR.
@PGijsbers please review/merge.
PGijsbers
left a comment
There was a problem hiding this comment.
I left a few remarks, additionally the ability to add and remove tags was added, but I did not see any tests added that test this new functionality. Those tests should be added.
I believe we had briefly discussed this before on a different PR, but I can't find it easily. Why store the response body as a binary file? The response is always text (either xml or json, potentially also some error text but text regardless). I do not think it is good to store these files as binary because it doesn't play well with git -and- because it makes it hard to review the file content (see for example the recent xz/ssh exploit). I would prefer these files have an extension that matches their content, and if that needs to be more generic, then we should use .txt.
openml#1619 (comment) Co-authored-by: Pieter Gijsbers <p.gijsbers@tue.nl>
for more information, see https://pre-commit.ci
This reverts commit 05dfa88.
I have removed tagging from OpenMLSetup, since it was unrelated to this PR and already tracked in #1648
I stored it in binary since the @PGijsbers This PR is up for review/merge. |
Fixes #1625
Depends on #1576
Related to #1575
Details
This PR implements
Setupsresource, and refactor its existing functions