Add attach_runs/detach_runs to OpenMLStudy and attach_tasks/detach_tasks to OpenMLBenchmarkSuite - #1729
Open
N-Garai wants to merge 3 commits into
Open
Add attach_runs/detach_runs to OpenMLStudy and attach_tasks/detach_tasks to OpenMLBenchmarkSuite#1729N-Garai wants to merge 3 commits into
N-Garai wants to merge 3 commits into
Conversation
…sks to OpenMLBenchmarkSuite Implements openml#1109. Adds instance methods to study and suite objects so users can call: - study.attach_runs(run_ids) - study.detach_runs(run_ids) - suite.attach_tasks(task_ids) - suite.detach_tasks(task_ids) instead of the module-level: - openml.study.attach_to_study(study_id, run_ids) The new methods delegate to existing module-level functions, update local state, and raise ValueError if the object has not been published. Existing module-level functions are preserved for backward compatibility.
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds instance-level convenience methods to the OpenML study/suite objects so callers can attach/detach runs/tasks directly on an OpenMLStudy / OpenMLBenchmarkSuite instance (instead of calling the module-level functions with IDs).
Changes:
- Add
OpenMLStudy.attach_runs()/OpenMLStudy.detach_runs()instance methods. - Add
OpenMLBenchmarkSuite.attach_tasks()/OpenMLBenchmarkSuite.detach_tasks()instance methods. - Add test-server coverage for the new instance methods and for unpublished-object guards.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
openml/study/study.py |
Adds the new instance methods for attaching/detaching runs/tasks on study/suite objects. |
tests/test_study/test_study_functions.py |
Adds new test-server tests validating the instance methods and unpublished-object error guards. |
Suppressed comments (2)
openml/study/study.py:328
openml.study.functionsis an internal reference; prefer the publicopenml.study.detach_from_studyfunction. Also, convertingrun_idsto a set avoids O(n*m) behavior when removing many run IDs from a large local list.
result = openml.study.functions.detach_from_study(self.id, run_ids)
if self.runs is not None:
self.runs = [run_id for run_id in self.runs if run_id not in run_ids]
return result
openml/study/study.py:451
- Use the public
openml.study.detach_from_suitefunction instead of the internalopenml.study.functionsreference. Convertingtask_idsto a set also avoids repeated linear membership checks when updating the local list.
result = openml.study.functions.detach_from_suite(self.id, task_ids)
if self.tasks is not None:
self.tasks = [task_id for task_id in self.tasks if task_id not in task_ids]
return result
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+299
to
+301
| result = openml.study.functions.attach_to_study(self.id, run_ids) | ||
| self.runs = (self.runs or []) + list(run_ids) | ||
| return result |
Comment on lines
+422
to
+424
| result = openml.study.functions.attach_to_suite(self.id, task_ids) | ||
| self.tasks = (self.tasks or []) + list(task_ids) | ||
| return result |
Comment on lines
+287
to
+289
| run_list_additional = openml.runs.list_runs(size=3, offset=5) | ||
| run_list_additional_ids = list(run_list_additional["run_id"]) | ||
| attached_count = study.attach_runs(run_list_additional_ids) |
…hecks - Replace openml.study.functions.* with openml.study.* in all four methods - Use set(run_ids) and set(task_ids) for O(1) membership checks in detach methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Metadata
Details
What does this PR implement/fix? Adds instance methods
attach_runs(run_ids)anddetach_runs(run_ids)toOpenMLStudy, andattach_tasks(task_ids)anddetach_tasks(task_ids)toOpenMLBenchmarkSuite. These methods delegate to the existing public module-level functions (openml.study.attach_to_study,openml.study.detach_from_study,openml.study.attach_to_suite,openml.study.detach_from_suite), update local object state, and raiseValueErrorif the object has not been published yet.Why is this change necessary? The current API requires passing the study/suite ID back into a module-level function even when the object is already in hand. This is inconsistent with the object-oriented design of the rest of the SDK and makes iterative workflows (e.g., adding runs to a study as they complete on a cluster) unnecessarily verbose.
How can I reproduce the issue this PR is solving and its solution?
The object methods behave identically to the module-level functions but operate on the instance directly.
Any other comments? Existing module-level functions are preserved for backward compatibility. New tests cover both successful attach/detach operations against the test server and ValueError guards for unpublished objects. All uploaded test entities are collected for removal using TestBase._mark_entity_for_removal().