Make tools.download_datasets consistent with the runner

closes #454

Author: popescu-v

khiops/core/internals/runner.py

@@ -1032,8 +1032,9 @@ def _initialize_default_samples_dir(self):
         # Set the fallback value for the samples directory
         home_samples_dir = Path.home() / "khiops_data" / "samples"
 
-        # Take the value of an environment variable in priority
-        if "KHIOPS_SAMPLES_DIR" in os.environ:
+        # Take the value of an environment variable in priority, if set to
+        # non-empty string
+        if "KHIOPS_SAMPLES_DIR" in os.environ and os.environ["KHIOPS_SAMPLES_DIR"]:
             self._samples_dir = os.environ["KHIOPS_SAMPLES_DIR"]
 
         # The samples location of Windows systems is:

khiops/tools.py

@@ -13,6 +13,7 @@
 import argparse
 import os
 import pathlib
+import platform
 import shutil
 import sys
 import tempfile
@@ -115,7 +116,10 @@ def download_datasets(
     """Downloads the Khiops sample datasets for a given version
 
     The datasets are downloaded to:
-        - Windows: ``%USERPROFILE%\\khiops_data\\samples``
+        - Windows:
+            - ``%PUBLIC%\\khiops_data\\samples`` if ``%PUBLIC%`` is defined and
+              points to a directory
+            - ``%USERPROFILE%\\khiops_data\\samples`` otherwise
         - Linux/macOS: ``$HOME/khiops_data/samples``
 
     Parameters
@@ -128,8 +132,22 @@ def download_datasets(
     # Note: The hidden parameter _called_from_shell is just to change the user messages.
 
     # Check if the home sample dataset location is available and build it if necessary
-    samples_dir = pathlib.Path.home() / "khiops_data" / "samples"
-    if samples_dir.exists() and not force_overwrite:
+    home_samples_dir = pathlib.Path.home() / "khiops_data" / "samples"
+
+    # Take the value of an environment variable in priority, if set to non-empty string
+    # If the environment variable is not set, samples location is:
+    # - on Windows systems:
+    #   - %PUBLIC%\khiops_data\samples if %PUBLIC% exists
+    #   - %USERPROFILE%\khiops_data\samples otherwise
+    # - on Linux / macOS systems:
+    #   - $HOME/khiops_data/samples
+    if "KHIOPS_SAMPLES_DIR" in os.environ and os.environ["KHIOPS_SAMPLES_DIR"]:
+        samples_dir = os.environ["KHIOPS_SAMPLES_DIR"]
+    elif platform.system() == "Windows" and "PUBLIC" in os.environ:
+        samples_dir = os.path.join(os.environ["PUBLIC"], "khiops_data", "samples")
+    else:
+        samples_dir = str(home_samples_dir)
+    if os.path.exists(samples_dir) and not force_overwrite:
         if _called_from_shell:
             instructions = "Execute with '--force-overwrite' to overwrite it"
         else:
@@ -140,7 +158,7 @@ def download_datasets(
         )
     else:
         # Create the samples dataset directory
-        if samples_dir.exists():
+        if os.path.exists(samples_dir):
             shutil.rmtree(samples_dir)
         os.makedirs(samples_dir, exist_ok=True)
 

tests/test_khiops_integrations.py

@@ -17,6 +17,7 @@
 
 import khiops.core as kh
 import khiops.core.internals.filesystems as fs
+from khiops import tools
 from khiops.core.exceptions import KhiopsEnvironmentError
 from khiops.core.internals.runner import KhiopsLocalRunner
 from khiops.extras.docker import KhiopsDockerRunner
@@ -30,6 +31,130 @@
 class KhiopsRunnerEnvironmentTests(unittest.TestCase):
     """Test that runners in different environments work"""
 
+    @unittest.skipIf(
+        os.environ.get("SKIP_EXPENSIVE_TESTS", "false").lower() == "true",
+        "Skipping expensive test",
+    )
+    def test_samples_are_downloaded_according_to_the_runner_setting(self):
+        """Test that samples are downloaded to the runner samples directory"""
+
+        # Save initial state
+        initial_runner = kh.get_runner()
+        initial_khiops_samples_dir = os.environ.get("KHIOPS_SAMPLES_DIR")
+
+        # Test that default samples download location is consistent with the
+        # KhiopsLocalRunner samples directory
+        with tempfile.TemporaryDirectory() as tmp_samples_dir:
+
+            # Set environment variable to the temporary samples dir
+            os.environ["KHIOPS_SAMPLES_DIR"] = tmp_samples_dir
+
+            # Create test runner to update samples dir to tmp_samples_dir,
+            # according to the newly-set environment variable
+            test_runner = KhiopsLocalRunner()
+
+            # Set current runner to the test runner
+            kh.set_runner(test_runner)
+
+            # Check that samples are not in tmp_samples_dir
+            try:
+                self.assert_samples_dir_integrity(tmp_samples_dir)
+            except AssertionError:
+                pass
+
+            # Fail if the samples directory is populated (it should be empty)
+            else:
+                return False
+
+            # Download samples into existing, but empty, tmp_samples_dir
+            # Check that samples have been downloaded to tmp_samples_dir
+            tools.download_datasets(force_overwrite=True)
+            self.assert_samples_dir_integrity(tmp_samples_dir)
+
+        # Remove KHIOPS_SAMPLES_DIR
+        # Create test runner to update samples dir to the default runner samples
+        # dir, following the deletion of the KHIOPS_SAMPLES_DIR environment
+        # variable
+        # Set current runner to the test runner
+        del os.environ["KHIOPS_SAMPLES_DIR"]
+        test_runner = KhiopsLocalRunner()
+        kh.set_runner(test_runner)
+
+        # Get the default runner samples dir
+        default_runner_samples_dir = kh.get_samples_dir()
+
+        # Move existing default runner samples dir contents to temporary directory
+        if os.path.isdir(default_runner_samples_dir):
+            tmp_initial_samples_dir = tempfile.mkdtemp()
+            shutil.copytree(
+                default_runner_samples_dir,
+                tmp_initial_samples_dir,
+                dirs_exist_ok=True,
+            )
+            shutil.rmtree(default_runner_samples_dir)
+        else:
+            tmp_initial_samples_dir = None
+
+        # Check that the samples are not present in the default runner
+        # samples dir
+        try:
+            self.assert_samples_dir_integrity(default_runner_samples_dir)
+        except AssertionError:
+            pass
+
+        # Fail if the samples directory is populated (it should not exist)
+        else:
+            return False
+
+        # Download datasets to the default runner samples dir (which
+        # should be created on this occasion)
+        # Default samples dir does not exist anymore
+        # Check that the default samples dir is populated
+        tools.download_datasets()
+        self.assert_samples_dir_integrity(default_runner_samples_dir)
+
+        # Clean-up default samples dir
+        shutil.rmtree(default_runner_samples_dir)
+
+        # Restore initial state:
+        # - initial samples dir contents if previously present
+        # - initial KHIOPS_SAMPLES_DIR if set
+        # - initial runner
+        if tmp_initial_samples_dir is not None and os.path.isdir(
+            tmp_initial_samples_dir
+        ):
+            shutil.copytree(
+                tmp_initial_samples_dir,
+                default_runner_samples_dir,
+                dirs_exist_ok=True,
+            )
+
+            # Remove temporary directory
+            shutil.rmtree(tmp_initial_samples_dir)
+        if initial_khiops_samples_dir is not None:
+            os.environ["KHIOPS_SAMPLES_DIR"] = initial_khiops_samples_dir
+        kh.set_runner(initial_runner)
+
+    def assert_samples_dir_integrity(self, samples_dir):
+        """Checks that the samples dir has the expected structure"""
+        expected_dataset_names = [
+            "Accidents",
+            "AccidentsSummary",
+            "Adult",
+            "Customer",
+            "CustomerExtended",
+            "Iris",
+            "Letter",
+            "Mushroom",
+            "NegativeAirlineTweets",
+            "SpliceJunction",
+            "Vehicle",
+            "WineReviews",
+        ]
+        self.assertTrue(os.path.isdir(samples_dir))
+        for ds_name in expected_dataset_names:
+            self.assertIn(ds_name, os.listdir(samples_dir))
+
     @unittest.skipIf(
         platform.system() != "Linux", "Skipping test for non-Linux platform"
     )