Skip to content

Making some unit tests work - #1000

Merged
PGijsbers merged 33 commits into
developfrom
fix_unit_tests
Dec 24, 2020
Merged

Making some unit tests work#1000
PGijsbers merged 33 commits into
developfrom
fix_unit_tests

Conversation

@Neeratyoy

@NeeratyoyNeeratyoy commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

What does this PR implement/fix? Explain your changes.

  • Fixes some of the failing unit tests post the test server clean-up.

  • Update docs to ensure that any publish to the test server during unit testing is also scheduled for deletion.

@Neeratyoy

Neeratyoy commented Nov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

@mfeurerthis step appears to be required in order to test the subsequent clause under which the failure message is raised. That is, without uploading a task, the test passes without exceptions.

However, on local tests with the current change, it gave the following error:
openml.exceptions.OpenMLServerException: https://test.openml.org/api/v1/xml/task/ returned code 623: Tasks can only be created upon active datasets. - problematic input: source_data, dataset not active.

This issue was addressed by waiting for the dataset to be processed before creating a task on it.

@Neeratyoy

Copy link
Copy Markdown
ContributorAuthor

@mfeurer many more tests fail now
This is a common error:

openml.exceptions.OpenMLServerException: https://test.openml.org/api/v1/xml/flow/exists returned code 104: This is a read-only account, it does not have permission for write operations. - API calls of the read-only user can only be of type GET.

While other fetches from the server return empty, which seems to make many of the assertions fail too!

@mfeurer

Copy link
Copy Markdown
Collaborator

@Neeratyoy I just discussed with @joaquinvanschoren and he gave the API key write permissions

@Neeratyoy

Copy link
Copy Markdown
ContributorAuthor

@mfeurer

  1. If this function is in line with what we discussed, I can then extend it to other tests: https://github.com/openml/openml-python/pull/1000/files#diff-cce4323af466db2f54edb9161a510818e26857ea520e94923c3b987aff28dfc8R35

  2. By the design of create_task, I wasn't able to create a meta_data that can be uniformly used to create a task, publish, retrieve and check the meta-data, so I needed this line:

    task_meta_data["task_type"] =TaskType.SUPERVISED_CLASSIFICATION

@codecov-io

codecov-io commented Dec 8, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1000 (b5e1242) into develop (560e952) will decrease coverage by 0.15%.
The diff coverage is 78.57%.

Impacted file tree graph

@@ Coverage Diff @@## develop #1000 +/- ##
===========================================
- Coverage 87.86% 87.70% -0.16% 
===========================================
Files 36 36 Lines 4531 4580 +49 ===========================================
+ Hits 3981 4017 +36 - Misses 550 563 +13 
Impacted FilesCoverage Δ
openml/testing.py83.92% <70.58%> (-3.58%)⬇️
openml/_api_calls.py88.97% <84.37%> (-2.02%)⬇️
openml/config.py79.67% <100.00%> (+0.33%)⬆️
openml/utils.py91.33% <100.00%> (+0.72%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 560e952...f5e4a3e. Read the comment docs.

@Neeratyoy
Neeratyoy marked this pull request as ready for review December 9, 2020 14:21
Comment threadopenml/utils.py Outdated
Comment threadopenml/utils.py Outdated
Comment threadopenml/utils.py Outdated
Comment threadopenml/utils.py Outdated
Comment threadtests/test_datasets/test_dataset_functions.py Outdated
Comment threadtests/test_runs/test_run_functions.py Outdated
Comment threadtests/test_runs/test_run_functions.py Outdated
Comment threadtests/test_runs/test_run_functions.py Outdated
Comment threadtests/test_runs/test_run_functions.py Outdated
Comment threadtests/test_tasks/test_regression_task.py Outdated
Neeratyoy added a commit that referenced this pull request Dec 15, 2020
@mfeurermfeurer mentioned this pull request Dec 15, 2020
Comment threadopenml/_api_calls.py Outdated
Comment threadopenml/_api_calls.py Outdated
Comment threadopenml/testing.py Outdated
Comment threadtests/test_runs/test_run_functions.py Outdated
Comment threadtests/test_study/test_study_functions.py

@PGijsbersPGijsbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at _api_calls. I think the main point of discussion is whether or not we want two loops of retries for server connects. Currently the outer loop is ran as long as the response content is incorrect (not same hash), and the inner loop also connects up to n_retries in _send_request until a status: 200. If we don't want to nest this loop (and have n_retries^2 retries, we should probably allow the _send_request to check the response against a checksum for get queries.

Comment threadopenml/_api_calls.py Outdated
Comment threadopenml/_api_calls.py Outdated
Comment threadopenml/_api_calls.py Outdated
@Neeratyoy

Copy link
Copy Markdown
ContributorAuthor

@PGijsbers the tests that fail are oddly due to either a worker crashing or the issue pertaining to attaching entities to a study.

Comment threadtests/test_runs/test_run_functions.py Outdated

@PGijsbersPGijsbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _api_calls looks fine now. I also left a comment on a change that was shown to me when I clicked the notification, would like some clarification for that first.
edit: but I do still need to look at the ci results as well

@PGijsbers

Copy link
Copy Markdown
Collaborator

Looks mostly like server failure? Though something like this is suspicious:

=================================== FAILURES ===================================
____________________ TestStudyFunctions.test_publish_study _____________________
[gw3] linux -- Python 3.6.12 /opt/hostedtoolcache/Python/3.6.12/x64/bin/python
self = <test_study_functions.TestStudyFunctions testMethod=test_publish_study>
@pytest.mark.flaky()
def test_publish_study(self):
# get some random runs to attach
run_list = openml.evaluations.list_evaluations("predictive_accuracy", size=10)
self.assertEqual(len(run_list), 10)
fixt_alias = None
fixt_name = "unit tested study"
fixt_descr = "bla"
fixt_flow_ids = set([evaluation.flow_id for evaluation in run_list.values()])
fixt_task_ids = set([evaluation.task_id for evaluation in run_list.values()])
fixt_setup_ids = set([evaluation.setup_id for evaluation in run_list.values()])
study = openml.study.create_study(
alias=fixt_alias,
benchmark_suite=None,
name=fixt_name,
description=fixt_descr,
run_ids=list(run_list.keys()),
)
study.publish()
# not tracking upload for delete since _delete_entity called end of function
# asserting return status from openml.study.delete_study()
self.assertGreater(study.id, 0)
study_downloaded = openml.study.get_study(study.id)
self.assertEqual(study_downloaded.alias, fixt_alias)
self.assertEqual(study_downloaded.name, fixt_name)
self.assertEqual(study_downloaded.description, fixt_descr)
self.assertEqual(study_downloaded.main_entity_type, "run")
self.assertSetEqual(set(study_downloaded.runs), set(run_list.keys()))
self.assertSetEqual(set(study_downloaded.setups), set(fixt_setup_ids))
self.assertSetEqual(set(study_downloaded.flows), set(fixt_flow_ids))
self.assertSetEqual(set(study_downloaded.tasks), set(fixt_task_ids))
# test whether the list run function also handles study data fine
run_ids = openml.runs.list_runs(study=study.id)
self.assertSetEqual(set(run_ids), set(study_downloaded.runs))
# test whether the list evaluation function also handles study data fine
run_ids = openml.evaluations.list_evaluations(
"predictive_accuracy", size=None, study=study.id
)
self.assertSetEqual(set(run_ids), set(study_downloaded.runs))
# attach more runs
run_list_additional = openml.runs.list_runs(size=10, offset=10)
> openml.study.attach_to_study(study.id, list(run_list_additional.keys()))
tests/test_study/test_study_functions.py:164: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ openml/study/functions.py:371: in attach_to_study
result_xml = openml._api_calls._perform_api_call(uri, "post", post_variables)
openml/_api_calls.py:61: in _perform_api_call
response = __read_url(url, request_method, data)
openml/_api_calls.py:161: in __read_url
request_method=request_method, url=url, data=data, md5_checksum=md5_checksum
openml/_api_calls.py:192: in _send_request
__check_response(response=response, url=url, file_elements=files)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ response = <Response [412]>
url = 'https://test.openml.org/api/v1/xml/study/1685/attach'
file_elements = None
def __check_response(response, url, file_elements):
if response.status_code != 200:
> raise __parse_server_exception(response, url, file_elements=file_elements)
E openml.exceptions.OpenMLServerException: https://test.openml.org/api/v1/xml/study/1685/attach returned code 1045: Problem attaching entities. Please ensure to only attach entities that exist - None
openml/_api_calls.py:230: OpenMLServerException

would mean there were no runs on the server (or at most10)?

@Neeratyoy

Copy link
Copy Markdown
ContributorAuthor

I also left a comment on a change that was shown to me when I clicked the notification, would like some clarification for that first.

I'm afraid I can't see exactly which comment :/

@PGijsbers

Copy link
Copy Markdown
Collaborator

I'm afraid I can't see exactly which comment :/

It's the one about the time measurements that you commented on 👍

@PGijsbers

PGijsbers commented Dec 23, 2020

Copy link
Copy Markdown
Collaborator

Thanks for all the work on this PR! I'm merging this as it's a big improvement to what was before, and so we don't need to carry this over a break.
Hopefully we can sort out the last failures soon. Ultimately we will need a better test structure one day, but that will be a major undertaking.

@PGijsbers
PGijsbers merged commit fba6aab into developDec 24, 2020
@PGijsbers
PGijsbers deleted the fix_unit_tests branch December 24, 2020 09:08
@mfeurer

Copy link
Copy Markdown
Collaborator

Thanks for merging @PGijsbers

The flaky error you found was reported by @Neeratyoy in openml/OpenML#1080

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Neeratyoy@mfeurer@codecov-io@PGijsbers