[ENH] V1 → V2 API Migration - setups#1619
[ENH] V1 → V2 API Migration - setups#1619EmanAbdelhaleem wants to merge 103 commits intoopenml:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
==========================================
+ Coverage 52.04% 54.02% +1.98%
==========================================
Files 36 64 +28
Lines 4333 5090 +757
==========================================
+ Hits 2255 2750 +495
- Misses 2078 2340 +262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- removing this since it was not part of the sdk previously - some tests fail because of the timeout in stacked PRs - this option can easily be added if needed in future
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
https clients are already initialized, you copy design from tests/test_api/test_versions.py
| flow.name = f"{get_sentinel()}{flow.name}" | ||
| flow.publish() | ||
| TestBase._mark_entity_for_removal("flow", flow.flow_id, flow.name) | ||
| TestBase.logger.info(f"collected from {__file__.split('/')[-1]}: {flow.flow_id}") |
There was a problem hiding this comment.
it's nice that you have it here, but it won't take affect, I should add TestBase as parent for TestAPIBase so this works
tests/test_api/test_setup.py
Outdated
| assert setup_id == run.setup_id | ||
|
|
||
|
|
||
| class TestSetupV2(TestAPIBase): |
There was a problem hiding this comment.
this class has no tests, you should check if it not implemented methods raise the correct exception
tests/test_api/test_setup.py
Outdated
| self.resource = SetupV1API(self.http_client) | ||
| self.extension = SklearnExtension() | ||
|
|
||
| @pytest.mark.uses_test_server() |
There was a problem hiding this comment.
can be moved to class level
There was a problem hiding this comment.
You are talking about the extension, right?
I followed the code of the existing tests. and it's initialized in the setUp() function, I am not even sure if moving it would make difference, the setUp() is already called before running the class tests, rigth?
There was a problem hiding this comment.
I was talking about @pytest.mark.uses_test_server()
There was a problem hiding this comment.
good, hopefully other tests are not breaking.
identified while debugging openml#1616 (comment)
tests/test_api/test_setup.py
Outdated
| # although the flow exists (created as of previous statement), | ||
| # we can be sure there are no setups (yet) as it was just created | ||
| # and hasn't been ran | ||
| setup_id = openml.setups.setup_exists(flow) |
There was a problem hiding this comment.
@geetu040
I am not sure how to test the exists method without mocking as it is dependent on a Flow object and a run Object. These tests I am using here, which I copied from test_setups_functions.py, are not actually testing the API cuz they are using the openml.setups.setup_exists()
The API methods signature is def exists(self, file_elements: dict[str, Any]) -> int | bool:, so, I need to pass file_elements if I want to test it, and to get these file elements, the following code is used in the def setup_exists(flow: OpenMLFlow) -> int: function in setups/functions.py
# sadly, this api call relies on a run object
openml.flows.functions._check_flow_for_server_id(flow)
if flow.model is None:
raise ValueError("Flow should have model field set with the actual model.")
if flow.extension is None:
raise ValueError("Flow should have model field set with the correct extension.")
# checks whether the flow exists on the server and flow ids align
exists = flow_exists(flow.name, flow.external_version)
if exists != flow.flow_id:
raise ValueError(
f"Local flow id ({flow.id}) differs from server id ({exists}). "
"If this issue persists, please contact the developers.",
)
openml_param_settings = flow.extension.obtain_parameter_values(flow)
description = xmltodict.unparse(_to_dict(flow.flow_id, openml_param_settings), pretty=True)
file_elements = {
"description": ("description.arff", description),
}
So, I first need to create and publish the flow as in these tests and then get the file_elements as in the above code, However, I think this might be wrong for a unit test, What do you think?
There was a problem hiding this comment.
yes you are right, this is not ideal for a unit test, but we already have many not so ideal tests, adding one more won't be a problem. but I would suggest if mocking can make it simple for you then sure go ahead and use that, we are already using it in some places where without mocking irrelevant parts of code would need to be executed.
There was a problem hiding this comment.
This question is outdated actually, I would ask for a review for the recent changes, both for the tests and the resource module.
` to class level
6 participants
Fixes #1625
Depends on #1576
Related to #1575
Details
This PR implements
Setupsresource, and refactor its existing functions