[ENH] V1 → V2 API Migration - studies

[rohansen856]

Metadata

• Reference Issue: [ENH] V1 → V2 API Migration - studies #1594 (towards [ENH] V1 → V2 API Migration #1575)
• New Tests Added: No
• Documentation Updated: No

Details

Stackend PR, Depends on #1576

This PR adds Studies v2 migration.

A question:
Due to the pre commit hook i could not put 6 arguments in a function, so i had to workaround that with this instead:
openml_api\resources\studies.py (line 10-15)

        limit = kwargs.get("limit")
        offset = kwargs.get("offset")
        status = kwargs.get("status")
        main_entity_type = kwargs.get("main_entity_type")
        uploader = kwargs.get("uploader")
        benchmark_suite = kwargs.get("benchmark_suite")

I would like to confirm if this approach is correct or not. Raising a draft PR for now.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 23.91304% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.64%. Comparing base (e653ef6) to head (03ea718).

Files with missing lines	Patch %	Lines
openml/_api/resources/study.py	23.25%	33 Missing ⚠️
openml/study/functions.py	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1610      +/-   ##
==========================================
- Coverage   54.67%   54.64%   -0.04%     
==========================================
  Files          63       63              
  Lines        5108     5124      +16     
==========================================
+ Hits         2793     2800       +7     
- Misses       2315     2324       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rohansen856]

Implementing noqa instead of the kwargs following example from here: openml\testing.py:

    def _check_fold_timing_evaluations(  # noqa: PLR0913
        self,
        fold_evaluations: dict[str, dict[int, dict[int, float]]],
        num_repeats: int,
        num_folds: int,
        *,
        max_time_allowed: float = 60000.0,
        task_type: TaskType = TaskType.SUPERVISED_CLASSIFICATION,
        check_scores: bool = True,
    ) -> None:

Final function signature:

    def list(  # noqa: PLR0913
        self,
        limit: int | None = None,
        offset: int | None = None,
        status: str | None = None,
        main_entity_type: str | None = None,
        uploader: list[int] | None = None,
        benchmark_suite: int | None = None,
    ) -> Any:

[geetu040]

Good work. Just use the listing as suggested in #1575 (comment) which is already similar to what you have done.

[rohansen856]

@geetu040 I reviewed the specific changes needed and have a slight doubt in the pandas implementation.
So as i undertand, i need to use pandas Dataframe insteaf of ANY in openml\_api\resources\base.py like this:

class StudiesAPI(ResourceAPI, ABC):
    @abstractmethod
    def list(  # noqa: PLR0913
        self,
        limit: int | None = None,
        offset: int | None = None,
        status: str | None = None,
        main_entity_type: str | None = None,
        uploader: list[int] | None = None,
        benchmark_suite: int | None = None,
    ) -> pd.DataFrame: ...

and similarly i have to change the return object in openml\_api\resources\studies.py from this:return response.text
to this:

xml_string = response.text

        # Parse XML and convert to DataFrame
        study_dict = xmltodict.parse(xml_string, force_list=("oml:study",))

        # Minimalistic check if the XML is useful
        assert isinstance(study_dict["oml:study_list"]["oml:study"], list), type(
            study_dict["oml:study_list"],
        )
        assert (
            study_dict["oml:study_list"]["@xmlns:oml"] == "http://openml.org/openml"
        ), study_dict["oml:study_list"]["@xmlns:oml"]

        studies = {}
        for study_ in study_dict["oml:study_list"]["oml:study"]:
            # maps from xml name to a tuple of (dict name, casting fn)
            expected_fields = {
                "oml:id": ("id", int),
                "oml:alias": ("alias", str),
                "oml:main_entity_type": ("main_entity_type", str),
                "oml:benchmark_suite": ("benchmark_suite", int),
                "oml:name": ("name", str),
                "oml:status": ("status", str),
                "oml:creation_date": ("creation_date", str),
                "oml:creator": ("creator", int),
            }
            study_id = int(study_["oml:id"])
            current_study = {}
            for oml_field_name, (real_field_name, cast_fn) in expected_fields.items():
                if oml_field_name in study_:
                    current_study[real_field_name] = cast_fn(study_[oml_field_name])
            current_study["id"] = int(current_study["id"])
            studies[study_id] = current_study

        return pd.DataFrame.from_dict(studies, orient="index")

A total of 3 files would be affected: openml\_api\resources\base.py, openml\_api\resources\studies.py and openml\study\functions.py

Can you please confirm my approach... After that i will update the PR.

[geetu040]

@rohansen856 yes sounds right

[rohansen856]

Updated! Ready for review.

[geetu040]

Almost fine, just complety remove _list_studies as well and replace _list_studies with api_context.backend.studies.list as the parameter for partial in list_studies. Hope I didnot confuse you, just search for the exact method names in code. Let me know if I am not clear enough.

[rohansen856]

Almost fine, just complety remove _list_studies as well and replace _list_studies with api_context.backend.studies.list as the parameter for partial in list_studies. Hope I didnot confuse you, just search for the exact method names in code. Let me know if I am not clear enough.

Oh definitely! I prolly missed that in openml\study\functions.py but pushing the change with next commit.

[geetu040]

update with #1576 (comment)

[geetu040]

I think it's not synced with the base pr #1576 that's why the tests are failing.

[geetu040]

Please add in description Fixes 1594

[geetu040]

The base PR is merged now, please sync with main.

[geetu040]

Nicely done overall, I am a bit skeptical about the overuse of mocking, since it adds alot of hardcoded response content, but let's see if Pieter thinks otherwise, when he reviews.

[geetu040]

use abstractmethod and I'd say remove the docstring and simply use the placeholder ... inplace of NotImplementedError

[geetu040]

Suggested change

	assert result is True
	assert result

[geetu040]

also fix the failing tests, you probably just need to fix the fixture, see other test files for reference

[geetu040]

Thanks for the changes, looks good now.

@PGijsbers please review/merge.

[PGijsbers]

Minor test changes required, confirmation from @geetu040 requested on the intent of the added functions.

[PGijsbers]

Suggested change

	mock_request.assert_called_once()

[PGijsbers]

Make sure that we check the mocks are used so that we're testing what we think we are testing.

[PGijsbers]

I think the real test here is that we make the right request, e.g., that the pagination parameters are set correctly in the URL. We should assert that. The parsed responses are not really under test, and are dictated by the mocked responses anyway.

[PGijsbers]

note (no action required): as discussed in the call today, the publish method is going to be used internally through from OpenMLBase.publish. Users will not be expected to pass files manually to the function.
Similar work will be done for tag/untag so that the interface for those operations doesn't change.
This is listed as follow up work to be handled in a separate PR.

Just leaving the comment here to clarify the intent and double check my understanding is correct.

@geetu040