Uh oh!
There was an error while loading. Please reload this page.
[ENH] V1 → V2 API Migration - setups - #1619
Conversation
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
openml#1619 (comment) Co-authored-by: Pieter Gijsbers <p.gijsbers@tue.nl>
for more information, see https://pre-commit.ci
This reverts commit 05dfa88.
geetu040
commented
Mar 25, 2026
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. |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#1625
Depends on #1576
Related to #1575
Details
This PR implements
Setupsresource, and refactor its existing functions