Repository: aeye-lab/pymovements (python)

Issue:
Don't extract dataset archives twice
## Description of the problem

Currently calling something like this at the top of your notebook
```python
dataset = pm.Dataset('JuDo1000', path='data/JuDo1000')
dataset.download()
```
will lead to extracting the archives everytime again.

This was implemented this way before as it was the simplest way without checking that all data was correctly extracted.

Of course calling
```python
dataset = pm.Dataset('JuDo1000', path='data/JuDo1000')
dataset.download(extract=False)
```
would handle this, but we would need to change the notebook for that.

## Description of a solution

It would be much nicer to have some kind of detection in place to check if the dataset archives were already extracted.

There we would need to check at least if all files were extracted.
I personally would feel safer if there would also be some checking if the contents were extracted correctly, because it could be that the extraction was aborted during the last file. In such a case we would have no chance in getting a waning without checking the checksums / contents of each file.

Pull request description:
feat!: resume archive extraction by skipping existing files
# :boom: Breaking change
`Dataset.extract()` now accepts only keyword arguments, no positional arguments.
Replace something like `Dataset.extract(True)` with `Dataset.extract(remove_finished=True)`

# Description

resolves #488 eventually

currently, tox does not work locally for whatever reason. 

- [x] time vs old extraction
- [x] coverage

Patch:
diff --git a/src/pymovements/dataset/dataset.py b/src/pymovements/dataset/dataset.py
index e7a892e..2670f1b 100644
--- a/src/pymovements/dataset/dataset.py
+++ b/src/pymovements/dataset/dataset.py
@@ -954,6 +954,7 @@ class Dataset:
             *,
             extract: bool = True,
             remove_finished: bool = False,
+            resume: bool = False,
             verbose: int = 1,
     ) -> Dataset:
         """Download dataset resources.
@@ -975,6 +976,9 @@ class Dataset:
             Extract dataset archive files. (default: True)
         remove_finished: bool
             Remove archive files after extraction. (default: False)
+        resume: bool
+            Resume previous extraction by skipping existing files.
+            Checks for correct size of existing files but not integrity. (default: False)
         verbose: int
             Verbosity levels: (1) Show download progress bar and print info messages on downloading
             and extracting archive files without printing messages for recursive archive extraction.
@@ -997,14 +1001,17 @@ class Dataset:
             paths=self.paths,
             extract=extract,
             remove_finished=remove_finished,
+            resume=resume,
             verbose=bool(verbose),
         )
         return self
 
     def extract(
             self,
+            *,
             remove_finished: bool = False,
             remove_top_level: bool = True,
+            resume: bool = False,
             verbose: int = 1,
     ) -> Dataset:
         """Extract downloaded dataset archive files.
@@ -1015,6 +1022,9 @@ class Dataset:
             Remove archive files after extraction. (default: False)
         remove_top_level: bool
             If ``True``, remove the top-level directory if it has only one child. (default: True)
+        resume: bool
+            Resume previous extraction by skipping existing files.
+            Checks for correct size of existing files but not integrity. (default: False)
         verbose: int
             Verbosity levels: (1) Print messages for extracting each dataset resource without
             printing messages for recursive archives. (2) Print additional messages for each
@@ -1030,6 +1040,7 @@ class Dataset:
             paths=self.paths,
             remove_finished=remove_finished,
             remove_top_level=remove_top_level,
+            resume=resume,
             verbose=verbose,
         )
         return self
diff --git a/src/pymovements/dataset/dataset_download.py b/src/pymovements/dataset/dataset_download.py
index ccf24c8..df00bed 100644
--- a/src/pymovements/dataset/dataset_download.py
+++ b/src/pymovements/dataset/dataset_download.py
@@ -32,8 +32,10 @@ from pymovements.utils.downloads import download_file
 def download_dataset(
         definition: DatasetDefinition,
         paths: DatasetPaths,
+        *,
         extract: bool = True,
         remove_finished: bool = False,
+        resume: bool = False,
         verbose: bool = True,
 ) -> None:
     """Download dataset resources.
@@ -59,6 +61,9 @@ def download_dataset(
         Extract dataset archive files. (default: True)
     remove_finished: bool
         Remove archive files after extraction. (default: False)
+    resume: bool
+        Resume previous extraction by skipping existing files.
+        Checks for correct size of existing files but not integrity. (default: False)
     verbose: bool
         If True, show progress of download and print status messages for integrity checking and
         file extraction. (default: True)
@@ -210,6 +215,7 @@ def download_dataset(
             definition=definition,
             paths=paths,
             remove_finished=remove_finished,
+            resume=resume,
             verbose=verbose,
         )
 
@@ -217,8 +223,10 @@ def download_dataset(
 def extract_dataset(
         definition: DatasetDefinition,
         paths: DatasetPaths,
+        *,
         remove_finished: bool = False,
         remove_top_level: bool = True,
+        resume: bool = False,
         verbose: int = 1,
 ) -> None:
     """Extract downloaded dataset archive files.
@@ -233,6 +241,9 @@ def extract_dataset(
         Remove archive files after extraction. (default: False)
     remove_top_level: bool
         If ``True``, remove the top-level directory if it has only one child. (default: True)
+    resume: bool
+        Resume previous extraction by skipping existing files.
+        Checks for correct size of existing files but not integrity. (default: False)
     verbose: int
         Verbosity levels: (1) Print messages for extracting each dataset resource without printing
         messages for recursive archives. (2) Print messages for extracting each dataset resource and
@@ -250,6 +261,7 @@ def extract_dataset(
                 recursive=True,
                 remove_finished=remove_finished,
                 remove_top_level=remove_top_level,
+                resume=resume,
                 verbose=verbose,
             )
 
@@ -266,6 +278,7 @@ def extract_dataset(
                     recursive=True,
                     remove_finished=remove_finished,
                     remove_top_level=remove_top_level,
+                    resume=resume,
                     verbose=verbose,
                 )
             else:
@@ -284,6 +297,7 @@ def extract_dataset(
                     recursive=True,
                     remove_finished=remove_finished,
                     remove_top_level=remove_top_level,
+                    resume=resume,
                     verbose=verbose,
                 )
             else:
diff --git a/src/pymovements/utils/archives.py b/src/pymovements/utils/archives.py
index 3f98dae..caf0f50 100644
--- a/src/pymovements/utils/archives.py
+++ b/src/pymovements/utils/archives.py
@@ -32,15 +32,19 @@ from collections.abc import Callable
 from pathlib import Path
 from typing import IO
 
+from tqdm import tqdm
+
 from pymovements.utils.paths import get_filepaths
 
 
 def extract_archive(
         source_path: Path,
         destination_path: Path | None = None,
+        *,
         recursive: bool = True,
         remove_finished: bool = False,
         remove_top_level: bool = True,
+        resume: bool = False,
         verbose: int = 1,
 ) -> Path:
     """Extract an archive.
@@ -61,6 +65,9 @@ def extract_archive(
         If ``True``, remove the file after the extraction. (default: False)
     remove_top_level: bool
         If ``True``, remove the top-level directory if it has only one child. (default: True)
+    resume: bool
+        Resume previous extraction by skipping existing files.
+        Checks for correct size of existing files but not integrity. (default: False)
     verbose: int
         Verbosity levels: (1) Print messages for extracting each dataset resource without printing
         messages for recursive archives. (2) Print additional messages for each recursive archive
@@ -90,7 +97,13 @@ def extract_archive(
         print(f'Extracting {source_path.name} to {destination_path}')
 
     # Extract file and remove archive if desired.
-    extractor(source_path, destination_path, compression_type)
+    extractor(
+        source_path,
+        destination_path,
+        compression_type,
+        resume=resume,
+        verbose=verbose,
+    )
     if remove_finished:
         source_path.unlink()
 
@@ -129,6 +142,7 @@ def extract_archive(
                 remove_finished=remove_finished,
                 remove_top_level=remove_top_level,
                 verbose=0 if verbose < 2 else 2,
+                resume=resume,
             )
 
     return destination_path
@@ -138,6 +152,9 @@ def _extract_tar(
         source_path: Path,
         destination_path: Path,
         compression: str | None,
+        *,
+        resume: bool,
+        verbose: int,
 ) -> None:
     """Extract a tar archive.
 
@@ -149,18 +166,36 @@ def _extract_tar(
         Path to the directory the file will be extracted to.
     compression: str | None
         Compression filename suffix.
+    resume: bool
+        Resume if archive was already previous extracted.
+    verbose: int
+        Print messages for resuming each dataset resource.
     """
     with tarfile.open(source_path, f'r:{compression[1:]}' if compression else 'r') as archive:
-        if sys.version_info < (3, 12):  # pragma: <3.12 cover
-            archive.extractall(destination_path)
-        else:  # pragma: >=3.12 cover
-            archive.extractall(destination_path, filter='tar')
+        for member in tqdm(archive.getmembers()):
+            if resume:
+                member_dest_path = os.path.join(destination_path, member.name)
+                if (
+                        os.path.exists(member_dest_path) and
+                        member.name[-4:] in _ARCHIVE_EXTRACTORS and
+                        member.size == os.path.getsize(member_dest_path)
+                ):
+                    if verbose:
+                        print(f'Skipping {member.name} due to previous extraction')
+                    continue
+            if sys.version_info < (3, 12):  # pragma: <3.12 cover
+                archive.extract(member.name, destination_path)
+            else:  # pragma: >=3.12 cover
+                archive.extract(member.name, destination_path, filter='tar')
 
 
 def _extract_zip(
         source_path: Path,
         destination_path: Path,
         compression: str | None,
+        *,
+        resume: bool,
+        verbose: int,
 ) -> None:
     """Extract a zip archive.
 
@@ -172,18 +207,33 @@ def _extract_zip(
         Path to the directory the file will be extracted to.
     compression: str | None
         Compression filename suffix.
+    resume: bool
+        Resume if archive was already previous extracted.
+    verbose: int
+        Print messages for resuming each dataset resource.
     """
     compression_id = _ZIP_COMPRESSION_MAP[compression] if compression else zipfile.ZIP_STORED
     with zipfile.ZipFile(source_path, 'r', compression=compression_id) as archive:
-        archive.extractall(destination_path)
-
-
-_ARCHIVE_EXTRACTORS: dict[str, Callable[[Path, Path, str | None], None]] = {
+        for member in tqdm(archive.filelist):
+            if resume:
+                member_dest_path = os.path.join(destination_path, member.filename)
+                if (
+                    os.path.exists(member_dest_path) and
+                    member.filename[-4:] in _ARCHIVE_EXTRACTORS and
+                    member.file_size == os.path.getsize(member_dest_path)
+                ):
+                    if verbose:
+                        print(f'Skipping {member.filename} due to previous extraction')
+                    continue
+            archive.extract(member.filename, destination_path)
+
+
+_ARCHIVE_EXTRACTORS = {
     '.tar': _extract_tar,
     '.zip': _extract_zip,
 }
 
-_ARCHIVE_TYPE_ALIASES: dict[str, tuple[str, str]] = {
+_ARCHIVE_TYPE_ALIASES = {
     '.tbz': ('.tar', '.bz2'),
     '.tbz2': ('.tar', '.bz2'),
     '.tgz': ('.tar', '.gz'),
@@ -195,7 +245,7 @@ _COMPRESSED_FILE_OPENERS: dict[str, Callable[..., IO]] = {
     '.xz': lzma.open,
 }
 
-_ZIP_COMPRESSION_MAP: dict[str, int] = {
+_ZIP_COMPRESSION_MAP = {
     '.bz2': zipfile.ZIP_BZIP2,
     '.xz': zipfile.ZIP_LZMA,
 }


Test patch:
diff --git a/tests/unit/dataset/dataset_download_test.py b/tests/unit/dataset/dataset_download_test.py
index ca86349..1c2ec0a 100644
--- a/tests/unit/dataset/dataset_download_test.py
+++ b/tests/unit/dataset/dataset_download_test.py
@@ -732,6 +732,7 @@ def test_dataset_extract_remove_finished_true_gaze(
             recursive=True,
             remove_finished=True,
             remove_top_level=False,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -761,6 +762,7 @@ def test_dataset_extract_rm(
             recursive=True,
             remove_finished=False,
             remove_top_level=True,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -789,6 +791,7 @@ def test_dataset_extract_remove_finished_true_both(
             recursive=True,
             remove_finished=True,
             remove_top_level=False,
+            resume=False,
             verbose=1,
         ),
         mock.call(
@@ -797,6 +800,7 @@ def test_dataset_extract_remove_finished_true_both(
             recursive=True,
             remove_finished=True,
             remove_top_level=False,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -825,6 +829,7 @@ def test_dataset_extract_remove_finished_true_precomputed(
             recursive=True,
             remove_finished=True,
             remove_top_level=False,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -853,6 +858,7 @@ def test_dataset_extract_remove_finished_false_both(
             recursive=True,
             remove_finished=False,
             remove_top_level=True,
+            resume=False,
             verbose=1,
         ),
         mock.call(
@@ -861,6 +867,7 @@ def test_dataset_extract_remove_finished_false_both(
             recursive=True,
             remove_finished=False,
             remove_top_level=True,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -886,6 +893,7 @@ def test_dataset_extract_remove_finished_false_gaze(
             recursive=True,
             remove_finished=False,
             remove_top_level=True,
+            resume=False,
             verbose=1,
         ),
     ])
@@ -914,6 +922,7 @@ def test_dataset_extract_remove_finished_false_precomputed(
             recursive=True,
             remove_finished=False,
             remove_top_level=True,
+            resume=False,
             verbose=1,
         ),
     ])
diff --git a/tests/unit/utils/archives_test.py b/tests/unit/utils/archives_test.py
index fe1bc7b..7f291c3 100644
--- a/tests/unit/utils/archives_test.py
+++ b/tests/unit/utils/archives_test.py
@@ -104,6 +104,8 @@ def fixture_archive(request, tmp_path):
         archive_path = rootpath / f'test.{compression}'
     elif compression is not None and extension is not None:
         archive_path = rootpath / f'test.{extension}.{compression}'
+    else:
+        raise ValueError(f'{request.param} not supported for archive fixture')
 
     if compression is None and extension == 'zip':
         with zipfile.ZipFile(archive_path, 'w') as zip_open:
@@ -268,7 +270,11 @@ def fixture_unsupported_archive(request, tmp_path):
     ],
 )
 def test_extract_archive_destination_path_None(
-        recursive, remove_finished, remove_top_level, expected_files, archive,
+        recursive,
+        remove_finished,
+        remove_top_level,
+        expected_files,
+        archive,
 ):
     extract_archive(
         source_path=archive,
@@ -407,7 +413,12 @@ Supported suffixes are: '['.tar', '.zip']'."""
     ],
 )
 def test_extract_archive_destination_path_not_None(
-        recursive, remove_finished, remove_top_level, archive, tmp_path, expected_files,
+        recursive,
+        remove_finished,
+        remove_top_level,
+        archive,
+        tmp_path,
+        expected_files,
 ):
     destination_path = tmp_path / pathlib.Path('tmpfoo')
     extract_archive(
@@ -437,7 +448,10 @@ def test_extract_archive_destination_path_not_None(
     ],
 )
 def test_extract_compressed_file_destination_path_not_None(
-        recursive, remove_finished, compressed_file, tmp_path,
+        recursive,
+        remove_finished,
+        compressed_file,
+        tmp_path,
 ):
     destination_filename = 'tmpfoo'
     destination_path = tmp_path / pathlib.Path(destination_filename)
@@ -495,3 +509,83 @@ def test_decompress_unknown_compression_suffix():
         _decompress(pathlib.Path('test.zip.zip'))
     msg, = excinfo.value.args
     assert msg == "Couldn't detect a compression from suffix .zip."
+
+
+@pytest.mark.parametrize(
+    ('resume'),
+    [
+        pytest.param(True, id='resume_True'),
+        pytest.param(False, id='resume_False'),
+    ],
+)
+@pytest.mark.parametrize(
+    ('recursive', 'remove_top_level', 'expected_files'),
+    [
+        pytest.param(
+            False,
+            False,
+            (
+                'toplevel',
+                os.path.join('toplevel', 'recursive.zip'),
+            ),
+            id='recursive_false_remove_finished_false',
+        ),
+        pytest.param(
+            True,
+            False,
+            (
+                'toplevel',
+                os.path.join('toplevel', 'recursive.zip'),
+                os.path.join('toplevel', 'recursive'),
+                os.path.join('toplevel', 'recursive', 'singlechild'),
+                os.path.join('toplevel', 'recursive', 'singlechild', 'test.file'),
+            ),
+            id='recursive_true_remove_finished_false',
+        ),
+    ],
+)
+@pytest.mark.parametrize(
+    ('verbose'),
+    [
+        pytest.param(True, id='verbose_True'),
+        pytest.param(False, id='verbose_False'),
+    ],
+)
+def test_extract_archive_destination_path_not_None_no_remove_top_level_no_remove_finished_twice(
+        verbose,
+        recursive,
+        remove_top_level,
+        archive,
+        tmp_path,
+        resume,
+        expected_files,
+        capsys,
+):
+    destination_path = tmp_path / pathlib.Path('tmp')
+    extract_archive(
+        source_path=archive,
+        destination_path=destination_path,
+        recursive=recursive,
+        remove_finished=False,
+        remove_top_level=remove_top_level,
+        resume=resume,
+        verbose=verbose,
+    )
+    extract_archive(
+        source_path=archive,
+        destination_path=destination_path,
+        recursive=recursive,
+        remove_finished=False,
+        remove_top_level=remove_top_level,
+        resume=resume,
+        verbose=verbose,
+    )
+    if resume and verbose:
+        assert 'Skipping' in capsys.readouterr().out
+
+    if destination_path.is_file():
+        destination_path = destination_path.parent
+
+    result_files = {str(file.relative_to(destination_path)) for file in destination_path.rglob('*')}
+
+    assert result_files == set(expected_files)