Delete files uploaded to test server during unit testing (#735)

* Collecting and cleaning unit test dump

* Adding session level fixture with yield to delay deletion of files

* Adding PEP8 ignore F401

* Changelog update + pytest argument fix

* Messy first draft of possible designs

* Leaner implementation without additional imports

* Reordering flows to delete subflows later

* Updating with design changes for tracking files for deletion

* Handling edge cases

* Fixing unit test git status

* Fixing PEP8 issues

* FIxing type annotation

* Logging and leaner flow

* Fixing PEP8 and unit test errors

* Fixing test cases; Renaming function

* Fixing clustering task unit test

* Updating docs for unit test deletion

Author: Neeratyoy

CONTRIBUTING.md

@@ -81,6 +81,10 @@ following rules before you submit a pull request:
    Drafts often benefit from the inclusion of a
    [task list](https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments)
    in the PR description.
+   
+- Add [unit tests](https://github.com/openml/openml-python/tree/develop/tests) and [examples](https://github.com/openml/openml-python/tree/develop/examples) for any new functionality being introduced. 
+    - If an unit test contains an upload to the test server, please ensure that it is followed by a file collection for deletion, to prevent the test server from bulking up. For example, `TestBase._mark_entity_for_removal('data', dataset.dataset_id)`, `TestBase._mark_entity_for_removal('flow', (flow.flow_id, flow.name))`.
+    - Please ensure that the example is run on the test server by beginning with the call to `openml.config.start_using_configuration_for_example()`.      
 
 -  All tests pass when running `pytest`. On
    Unix-like systems, check with (from the toplevel source folder):

PULL_REQUEST_TEMPLATE.md

@@ -9,6 +9,8 @@ Please make sure that:
 * for any new function or class added, please add it to doc/api.rst
     * the list of classes and functions should be alphabetical 
 * for any new functionality, consider adding a relevant example
+* add unit tests for new functionalities
+    * collect files uploaded to test server using _mark_entity_for_removal()
 -->
 
 #### Reference Issue

doc/progress.rst

@@ -8,14 +8,17 @@ Changelog
 
 0.10.0
 ~~~~~~
-* ADD #722: Automatic reinstantiation of flow in `run_model_on_task`. Clearer errors if that's not possible.
+* FIX #261: Test server is cleared of all files uploaded during unit testing.
+* FIX #447: All files created by unit tests no longer persist in local.
 * FIX #608: Fixing dataset_id referenced before assignment error in get_run function.
-* ADD #715: `list_evaluations` now has an option to sort evaluations by score (value).
+* FIX #447: All files created by unit tests are deleted after the completion of all unit tests.
 * FIX #589: Fixing a bug that did not successfully upload the columns to ignore when creating and publishing a dataset.
+* FIX #608: Fixing dataset_id referenced before assignment error in get_run function.
 * DOC #639: More descriptive documention for function to convert array format.
 * ADD #687: Adds a function to retrieve the list of evaluation measures available.
 * ADD #695: A function to retrieve all the data quality measures available.
-* FIX #447: All files created by unit tests are deleted after the completion of all unit tests.
+* ADD #715: `list_evaluations` now has an option to sort evaluations by score (value).
+* ADD #722: Automatic reinstantiation of flow in `run_model_on_task`. Clearer errors if that's not possible.
 * MAINT #726: Update examples to remove deprecation warnings from scikit-learn
 
 0.9.0

openml/testing.py

@@ -18,6 +18,7 @@
 from openml.tasks import TaskTypeEnum
 
 import pytest
+import logging
 
 
 class TestBase(unittest.TestCase):
@@ -28,6 +29,18 @@ class TestBase(unittest.TestCase):
     Currently hard-codes a read-write key.
     Hopefully soon allows using a test server, not the production server.
     """
+    publish_tracker = {'run': [], 'data': [], 'flow': [], 'task': [],
+                       'study': [], 'user': []}  # type: dict
+    test_server = "https://test.openml.org/api/v1/xml"
+    # amueller's read/write key that he will throw away later
+    apikey = "610344db6388d9ba34f6db45a3cf71de"
+
+    # creating logger for unit test file deletion status
+    logger = logging.getLogger("unit_tests")
+    logger.setLevel(logging.INFO)
+    fh = logging.FileHandler('TestBase.log')
+    fh.setLevel(logging.INFO)
+    logger.addHandler(fh)
 
     def setUp(self, n_levels: int = 1):
         """Setup variables and temporary directories.
@@ -46,6 +59,7 @@ def setUp(self, n_levels: int = 1):
             Number of nested directories the test is in. Necessary to resolve the path to the
             ``files`` directory, which is located directly under the ``tests`` directory.
         """
+
         # This cache directory is checked in to git to simulate a populated
         # cache
         self.maxDiff = None
@@ -71,12 +85,9 @@ def setUp(self, n_levels: int = 1):
         os.chdir(self.workdir)
 
         self.cached = True
-        # amueller's read/write key that he will throw away later
-        openml.config.apikey = "610344db6388d9ba34f6db45a3cf71de"
+        openml.config.apikey = TestBase.apikey
         self.production_server = "https://openml.org/api/v1/xml"
-        self.test_server = "https://test.openml.org/api/v1/xml"
-
-        openml.config.server = self.test_server
+        openml.config.server = TestBase.test_server
         openml.config.avoid_duplicate_runs = False
         openml.config.cache_directory = self.workdir
 
@@ -87,7 +98,7 @@ def setUp(self, n_levels: int = 1):
                 with open(openml.config.config_file, 'w') as fh:
                     fh.write('apikey = %s' % openml.config.apikey)
 
-        # Increase the number of retries to avoid spurios server failures
+        # Increase the number of retries to avoid spurious server failures
         self.connection_n_retries = openml.config.connection_n_retries
         openml.config.connection_n_retries = 10
 
@@ -104,9 +115,43 @@ def tearDown(self):
         openml.config.server = self.production_server
         openml.config.connection_n_retries = self.connection_n_retries
 
+    @classmethod
+    def _mark_entity_for_removal(self, entity_type, entity_id):
+        """ Static record of entities uploaded to test server
+
+        Dictionary of lists where the keys are 'entity_type'.
+        Each such dictionary is a list of integer IDs.
+        For entity_type='flow', each list element is a tuple
+        of the form (Flow ID, Flow Name).
+        """
+        if entity_type not in TestBase.publish_tracker:
+            TestBase.publish_tracker[entity_type] = [entity_id]
+        else:
+            TestBase.publish_tracker[entity_type].append(entity_id)
+
+    @classmethod
+    def _delete_entity_from_tracker(self, entity_type, entity):
+        """ Deletes entity records from the static file_tracker
+
+        Given an entity type and corresponding ID, deletes all entries, including
+        duplicate entries of the ID for the entity type.
+        """
+        if entity_type in TestBase.publish_tracker:
+            # removes duplicate entries
+            TestBase.publish_tracker[entity_type] = list(set(TestBase.publish_tracker[entity_type]))
+            if entity_type == 'flow':
+                delete_index = [i for i, (id_, _) in
+                                enumerate(TestBase.publish_tracker[entity_type])
+                                if id_ == entity][0]
+            else:
+                delete_index = [i for i, id_ in
+                                enumerate(TestBase.publish_tracker[entity_type])
+                                if id_ == entity][0]
+            TestBase.publish_tracker[entity_type].pop(delete_index)
+
     @pytest.fixture(scope="session", autouse=True)
     def _cleanup_fixture(self):
-        """Cleans up files generated by Unit tests
+        """Cleans up files generated by unit tests
 
         This function is called at the beginning of the invocation of
         TestBase (defined below), by each of class that inherits TestBase.
@@ -125,7 +170,6 @@ def _cleanup_fixture(self):
             else:
                 static_cache_dir = os.path.join(static_cache_dir, '../')
         directory = os.path.join(static_cache_dir, 'tests/files/')
-        # directory = "{}/tests/files/".format(static_cache_dir)
         files = os.walk(directory)
         old_file_list = []
         for root, _, filenames in files:
@@ -135,17 +179,51 @@ def _cleanup_fixture(self):
         # pauses the code execution here till all tests in the 'session' is over
         yield
         # resumes from here after all collected tests are completed
+
+        #
+        # Local file deletion
+        #
         files = os.walk(directory)
         new_file_list = []
         for root, _, filenames in files:
             for filename in filenames:
                 new_file_list.append(os.path.join(root, filename))
         # filtering the files generated during this run
         new_file_list = list(set(new_file_list) - set(old_file_list))
-        print("Files to delete in local: {}".format(new_file_list))
         for file in new_file_list:
             os.remove(file)
 
+        #
+        # Test server deletion
+        #
+        openml.config.server = TestBase.test_server
+        openml.config.apikey = TestBase.apikey
+
+        # legal_entities defined in openml.utils._delete_entity - {'user'}
+        entity_types = {'run', 'data', 'flow', 'task', 'study'}
+        # 'run' needs to be first entity to allow other dependent entities to be deleted
+        # cloning file tracker to allow deletion of entries of deleted files
+        tracker = TestBase.publish_tracker.copy()
+
+        # reordering to delete sub flows at the end of flows
+        # sub-flows have shorter names, hence, sorting by descending order of flow name length
+        if 'flow' in entity_types:
+            flow_deletion_order = [entity_id for entity_id, _ in
+                                   sorted(tracker['flow'], key=lambda x: len(x[1]), reverse=True)]
+            tracker['flow'] = flow_deletion_order
+
+        # deleting all collected entities published to test server
+        for entity_type in entity_types:
+            for i, entity in enumerate(tracker[entity_type]):
+                try:
+                    openml.utils._delete_entity(entity_type, entity)
+                    TestBase.logger.info("Deleted ({}, {})".format(entity_type, entity))
+                    # deleting actual entry from tracker
+                    TestBase._delete_entity_from_tracker(entity_type, entity)
+                except Exception as e:
+                    TestBase.logger.warn("Cannot delete ({},{}): {}".format(entity_type, entity, e))
+        TestBase.logger.info("End of cleanup_fixture from {}".format(self.__class__))
+
     def _get_sentinel(self, sentinel=None):
         if sentinel is None:
             # Create a unique prefix for the flow. Necessary because the flow

tests/test_datasets/test_dataset_functions.py

@@ -478,6 +478,9 @@ def test_publish_dataset(self):
             data_file=file_path,
         )
         dataset.publish()
+        TestBase._mark_entity_for_removal('data', dataset.dataset_id)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            dataset.dataset_id))
         self.assertIsInstance(dataset.dataset_id, int)
 
     def test__retrieve_class_labels(self):
@@ -498,6 +501,9 @@ def test_upload_dataset_with_url(self):
             url="https://www.openml.org/data/download/61/dataset_61_iris.arff",
         )
         dataset.publish()
+        TestBase._mark_entity_for_removal('data', dataset.dataset_id)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            dataset.dataset_id))
         self.assertIsInstance(dataset.dataset_id, int)
 
     def test_data_status(self):
@@ -507,6 +513,9 @@ def test_data_status(self):
             version=1,
             url="https://www.openml.org/data/download/61/dataset_61_iris.arff")
         dataset.publish()
+        TestBase._mark_entity_for_removal('data', dataset.dataset_id)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            dataset.dataset_id))
         did = dataset.dataset_id
 
         # admin key for test server (only adminds can activate datasets.
@@ -620,6 +629,9 @@ def test_create_dataset_numpy(self):
         )
 
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
 
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
@@ -682,6 +694,9 @@ def test_create_dataset_list(self):
         )
 
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
             dataset._dataset,
@@ -725,6 +740,9 @@ def test_create_dataset_sparse(self):
         )
 
         upload_did = xor_dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
             xor_dataset._dataset,
@@ -762,6 +780,9 @@ def test_create_dataset_sparse(self):
         )
 
         upload_did = xor_dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
             xor_dataset._dataset,
@@ -885,6 +906,9 @@ def test_create_dataset_pandas(self):
             paper_url=paper_url
         )
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
             dataset._dataset,
@@ -919,6 +943,9 @@ def test_create_dataset_pandas(self):
             paper_url=paper_url
         )
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         self.assertEqual(
             _get_online_dataset_arff(upload_did),
             dataset._dataset,
@@ -955,6 +982,9 @@ def test_create_dataset_pandas(self):
             paper_url=paper_url
         )
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         downloaded_data = _get_online_dataset_arff(upload_did)
         self.assertEqual(
             downloaded_data,
@@ -1123,6 +1153,9 @@ def test___publish_fetch_ignore_attribute(self):
 
         # publish dataset
         upload_did = dataset.publish()
+        TestBase._mark_entity_for_removal('data', upload_did)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                            upload_did))
         # test if publish was successful
         self.assertIsInstance(upload_did, int)
         # variables to carry forward for test_publish_fetch_ignore_attribute()
@@ -1253,6 +1286,9 @@ def test_create_dataset_row_id_attribute_inference(self):
             )
             self.assertEqual(dataset.row_id_attribute, output_row_id)
             upload_did = dataset.publish()
+            TestBase._mark_entity_for_removal('data', upload_did)
+            TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1],
+                                                                upload_did))
             arff_dataset = arff.loads(_get_online_dataset_arff(upload_did))
             arff_data = np.array(arff_dataset['data'], dtype=object)
             # if we set the name of the index then the index will be added to

tests/test_extensions/test_sklearn_extension/test_sklearn_extension.py

@@ -1126,6 +1126,8 @@ def test_openml_param_name_to_sklearn(self):
         task = openml.tasks.get_task(115)
         run = openml.runs.run_flow_on_task(flow, task)
         run = run.publish()
+        TestBase._mark_entity_for_removal('run', run.run_id)
+        TestBase.logger.info("collected from {}: {}".format(__file__.split('/')[-1], run.run_id))
         run = openml.runs.get_run(run.run_id)
         setup = openml.setups.get_setup(run.setup_id)