Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1616 +/- ##
==========================================
- Coverage 54.67% 54.08% -0.60%
==========================================
Files 63 63
Lines 5108 5155 +47
==========================================
- Hits 2793 2788 -5
- Misses 2315 2367 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…u040/openml-python into runs-migration-stacked # Conflicts: # openml/_api/resources/__init__.py # openml/_api/runtime/core.py
…into runs-migration-stacked
…into runs-migration-stacked
geetu040
left a comment
There was a problem hiding this comment.
sync with base pr
sdk code look good so far, please take a look at #1575 (comment) and make changes accordingly where needed.
all tests (existing and new) should pass to make sure we are retaining the original functionality of the sdk
Signed-off-by: Omswastik-11 <omswastikpanda11@gmail.com>
…into runs-migration-stacked
Signed-off-by: Omswastik-11 <omswastikpanda11@gmail.com>
Signed-off-by: Omswastik-11 <omswastikpanda11@gmail.com>
…-11/openml-python into runs-migration-stacked
…-11/openml-python into runs-migration-stacked
This reverts commit 22d7179.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openml/runs/functions.py:1088
list_runsnow delegates toopenml._backend.run.list, but the legacy helper_list_runs/__list_runsimplementation remains below and appears unused (no internal references). Keeping both implementations increases maintenance burden and risks divergence; consider removing the dead code or clearly marking it as deprecated for eventual removal.
listing_call = partial(
openml._backend.run.list,
ids=id,
task=task,
setup=setup,
flow=flow,
uploader=uploader,
tag=tag,
study=study,
display_errors=display_errors,
task_type=task_type,
)
batches = openml.utils._list_all(listing_call, offset=offset, limit=size)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @mock.patch.object(requests.Session, "request") | ||
| def test_delete_run_not_owned(mock_delete, test_files_directory, test_server_v1, test_apikey_v1): | ||
| content_file = test_files_directory / "mock_responses" / "runs" / "run_delete_not_owned.xml" |
There was a problem hiding this comment.
The patched method is requests.Session.request, but the mock parameter is still named mock_delete. Renaming it to something like mock_request would make the test intent clearer and avoid confusion when reading failures.
| ) | ||
|
|
||
| assert isinstance(runs_dict["oml:runs"]["oml:run"], list), type(runs_dict["oml:runs"]) | ||
|
|
||
| runs = { |
There was a problem hiding this comment.
Avoid using assert for validating server-provided XML structure here. Asserts can be skipped with Python optimizations (-O), which would turn this into a hard-to-debug KeyError/TypeError later. Please replace with an explicit runtime check and raise a TypeError/ValueError (similar to openml/runs/functions.py::__list_runs).
openml/runs/functions.py
Outdated
There was a problem hiding this comment.
you can remove this function now, since it's not used anywhere
openml/runs/functions.py
Outdated
There was a problem hiding this comment.
this file still contains some old-style api calls through openml._api_calls which should be replaced with new-style api calls through openml._backend.run
Metadata
Details
fixes #1624