MRG: Merge pull request #445 from octue/enhancement/add-simpler-dataset-instantiation

Unify cloud and local dataset instantiation

Author: cortadocodes

octue/resources/dataset.py

@@ -5,6 +5,7 @@
 import logging
 import os
 import tempfile
+import warnings
 
 import coolname
 import requests
@@ -40,6 +41,7 @@ class Dataset(Labelable, Taggable, Serialisable, Identifiable, Hashable, Metadat
     :param str|None path:
     :param dict|octue.resources.tag.TagDict|None tags:
     :param iter(str)|octue.resources.label.LabelSet|None labels:
+    :param bool recursive: if `True`, include in the dataset all files in the subdirectories recursively contained within the dataset directory
     :param bool hypothetical: if `True`, ignore any metadata stored for this dataset locally or in the cloud and use whatever is given at instantiation
     :return None:
     """
@@ -50,10 +52,45 @@ class Dataset(Labelable, Taggable, Serialisable, Identifiable, Hashable, Metadat
     # Paths to files are added to the serialisation in `Dataset.to_primitive`.
     _SERIALISE_FIELDS = (*_METADATA_ATTRIBUTES, "path")
 
-    def __init__(self, files=None, name=None, id=None, path=None, tags=None, labels=None):
+    def __init__(
+        self,
+        files=None,
+        name=None,
+        id=None,
+        path=None,
+        tags=None,
+        labels=None,
+        recursive=False,
+        hypothetical=False,
+    ):
         super().__init__(name=name, id=id, tags=tags, labels=labels)
         self.path = path or os.getcwd()
-        self.files = self._instantiate_datafiles(files or [])
+        self.files = FilterSet()
+
+        if files:
+            self.files = self._instantiate_datafiles(files)
+            return
+
+        if storage.path.is_cloud_path(self.path):
+            self._instantiate_from_cloud(path=self.path, recursive=recursive, hypothetical=hypothetical)
+        else:
+            self._instantiate_from_local_directory(path=self.path, recursive=recursive, hypothetical=hypothetical)
+
+        if hypothetical:
+            logger.debug("Ignored stored metadata for %r.", self)
+            return
+
+        if self.metadata(include_sdk_version=False) != {
+            "name": name or self.name,
+            "id": id or self.id,
+            "tags": TagDict(tags),
+            "labels": LabelSet(labels),
+        }:
+            logger.warning(
+                "Overriding metadata given at instantiation with stored metadata for %r - set `hypothetical` to `True` "
+                "at instantiation to avoid this.",
+                self,
+            )
 
     @classmethod
     def from_local_directory(cls, path_to_directory, recursive=False, hypothetical=False, **kwargs):
@@ -66,26 +103,13 @@ def from_local_directory(cls, path_to_directory, recursive=False, hypothetical=F
         :param kwargs: other keyword arguments for the `Dataset` instantiation
         :return Dataset:
         """
-        datafiles = FilterSet()
-
-        for level, (directory_path, _, filenames) in enumerate(os.walk(path_to_directory)):
-            for filename in filenames:
-
-                if filename == METADATA_FILENAME:
-                    continue
-
-                if not recursive and level > 0:
-                    break
-
-                datafiles.add(Datafile(path=os.path.join(directory_path, filename)))
-
-        dataset = Dataset(path=path_to_directory, files=datafiles, **kwargs)
-
-        if not hypothetical:
-            dataset._use_local_metadata()
+        warnings.warn(
+            "The `Dataset.from_local_directory` class method is now deprecated. Please use the `Dataset` constructor "
+            "instead, passing it the `path`, `recursive`, and `hypothetical` kwargs as necessary.",
+            category=DeprecationWarning,
+        )
 
-        dataset._warn_about_metadata_override(hypothetical=hypothetical, **kwargs)
-        return dataset
+        return Dataset(path=path_to_directory, recursive=recursive, hypothetical=hypothetical, **kwargs)
 
     @classmethod
     def from_cloud(
@@ -107,32 +131,16 @@ def from_cloud(
         :param kwargs: other keyword arguments for the `Dataset` instantiation
         :return Dataset:
         """
+        warnings.warn(
+            "The `Dataset.from_cloud` class method is now deprecated. Please use the `Dataset` constructor instead, "
+            "passing it the `path`, `recursive`, and `hypothetical` kwargs as necessary.",
+            category=DeprecationWarning,
+        )
+
         if not cloud_path:
             cloud_path = translate_bucket_name_and_path_in_bucket_to_cloud_path(bucket_name, path_to_dataset_directory)
 
-        bucket_name = storage.path.split_bucket_name_from_cloud_path(cloud_path)[0]
-
-        dataset = Dataset(path=cloud_path, **kwargs)
-
-        if not hypothetical:
-            dataset._use_cloud_metadata()
-
-        if not dataset.files:
-            dataset.files = FilterSet(
-                Datafile(path=storage.path.generate_gs_path(bucket_name, blob.name))
-                for blob in GoogleCloudStorageClient().scandir(
-                    cloud_path,
-                    recursive=recursive,
-                    filter=(
-                        lambda blob: (
-                            not blob.name.endswith(METADATA_FILENAME) and SIGNED_METADATA_DIRECTORY not in blob.name
-                        )
-                    ),
-                )
-            )
-
-        dataset._warn_about_metadata_override(hypothetical=hypothetical, **kwargs)
-        return dataset
+        return Dataset(path=cloud_path, recursive=recursive, hypothetical=hypothetical, **kwargs)
 
     @property
     def name(self):
@@ -434,7 +442,7 @@ def download(iterable_element):
             for path in executor.map(download, files_and_paths):
                 logger.debug("Downloaded datafile to %r.", path)
 
-        logger.info("Downloaded %r dataset to %r.", self.name, local_directory)
+        logger.info("Downloaded %r to %r.", self, local_directory)
 
     def to_primitive(self, include_files=True):
         """Convert the dataset to a dictionary of primitives, converting its files into their paths for a lightweight
@@ -455,6 +463,57 @@ def to_primitive(self, include_files=True):
 
         return serialised_dataset
 
+    def _instantiate_from_cloud(self, path, recursive=True, hypothetical=False):
+        """Instantiate the dataset from a cloud directory.
+
+        :param str path: the cloud path to a directory in cloud storage
+        :param bool recursive: if `True`, include in the dataset all files in the subdirectories recursively contained within the dataset directory
+        :param bool hypothetical: if `True`, ignore any metadata stored for this dataset in the cloud and use whatever is given at instantiation
+        :return None:
+        """
+        if not hypothetical:
+            self._use_cloud_metadata()
+
+        if not self.files:
+            bucket_name = storage.path.split_bucket_name_from_cloud_path(path)[0]
+
+            self.files = FilterSet(
+                Datafile(path=storage.path.generate_gs_path(bucket_name, blob.name))
+                for blob in GoogleCloudStorageClient().scandir(
+                    path,
+                    recursive=recursive,
+                    filter=(
+                        lambda blob: (
+                            not blob.name.endswith(METADATA_FILENAME) and SIGNED_METADATA_DIRECTORY not in blob.name
+                        )
+                    ),
+                )
+            )
+
+    def _instantiate_from_local_directory(self, path, recursive=False, hypothetical=False):
+        """Instantiate the dataset from a local directory.
+
+        :param str path: the path to a local directory
+        :param bool recursive: if `True`, include in the dataset all files in the subdirectories recursively contained within the dataset directory
+        :param bool hypothetical: if `True`, ignore any metadata stored for this dataset locally and use whatever is given at instantiation
+        :return None:
+        """
+        self.files = FilterSet()
+
+        for level, (directory_path, _, filenames) in enumerate(os.walk(path)):
+            for filename in filenames:
+
+                if filename == METADATA_FILENAME:
+                    continue
+
+                if not recursive and level > 0:
+                    break
+
+                self.files.add(Datafile(path=os.path.join(directory_path, filename)))
+
+        if not hypothetical:
+            self._use_local_metadata()
+
     def _instantiate_datafiles(self, files):
         """Instantiate and add the given files to a `FilterSet`.
 
@@ -561,27 +620,3 @@ def _datafile_path_relative_to_self(self, datafile, path_type):
             datafile_path = datafile_path.split("?")[0]
 
         return storage.path.relpath(datafile_path, dataset_path)
-
-    def _warn_about_metadata_override(self, hypothetical, **kwargs):
-        """Issue a warning about instantiation metadata override if `hypothetical` is `False` and the dataset's metadata
-        is different from the provided instantiation keyword arguments.
-
-        :param bool hypothetical: if `True`, don't raise any warnings.
-        :param kwargs: the Dataset instantiation keyword arguments
-        :return None:
-        """
-        if hypothetical:
-            logger.debug("Ignored stored metadata for %r.", self)
-            return
-
-        if self.metadata(include_sdk_version=False) != {
-            "name": kwargs.get("name") or self.name,
-            "id": kwargs.get("id") or self.id,
-            "tags": TagDict(kwargs.get("tags")),
-            "labels": LabelSet(kwargs.get("labels")),
-        }:
-            logger.warning(
-                "Overriding metadata given at instantiation with stored metadata for %r - set `hypothetical` to `True` "
-                "at instantiation to avoid this.",
-                self,
-            )

octue/resources/manifest.py

@@ -49,7 +49,7 @@ def from_cloud(cls, cloud_path=None, bucket_name=None, path_to_manifest_file=Non
 
         return Manifest(
             id=serialised_manifest["id"],
-            datasets={key: Dataset.from_cloud(dataset) for key, dataset in serialised_manifest["datasets"].items()},
+            datasets={key: Dataset(path=dataset) for key, dataset in serialised_manifest["datasets"].items()},
         )
 
     @property
@@ -153,15 +153,11 @@ def _instantiate_dataset(self, key_and_dataset):
 
         # If `dataset` is just a path to a dataset:
         if isinstance(dataset, str):
-
-            if storage.path.is_cloud_path(dataset):
-                return (key, Dataset.from_cloud(cloud_path=dataset, recursive=True))
-
-            return (key, Dataset.from_local_directory(path_to_directory=dataset, recursive=True))
+            return (key, Dataset(path=dataset, recursive=True))
 
         # If `dataset` is a dictionary including a "path" key:
         if storage.path.is_cloud_path(dataset["path"]):
-            return (key, Dataset.from_cloud(cloud_path=dataset["path"], recursive=True))
+            return (key, Dataset(path=dataset["path"], recursive=True))
 
         return (key, Dataset(**dataset))
 

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "octue"
-version = "0.23.3"
+version = "0.23.4"
 description = "A package providing template applications for data services, and a python SDK to the Octue API."
 readme = "README.md"
 authors = ["Thomas Clark <[email protected]>", "cortadocodes <[email protected]>"]

tests/resources/__init__.py

@@ -15,4 +15,4 @@ def create_dataset_with_two_files(dataset_directory_path):
         with open(path, "w") as f:
             f.write(str(data))
 
-    return Dataset.from_local_directory(dataset_directory_path)
+    return Dataset(path=dataset_directory_path)

tests/resources/test_analysis.py

@@ -147,7 +147,7 @@ def test_finalise_with_upload(self):
             )
         )
 
-        downloaded_dataset = Dataset.from_cloud(signed_url_for_dataset)
+        downloaded_dataset = Dataset(path=signed_url_for_dataset)
         self.assertEqual(downloaded_dataset.name, "the_dataset")
         self.assertEqual(len(downloaded_dataset.files), 1)
         self.assertEqual(downloaded_dataset.labels, {"one", "two", "three"})