Repository: aeye-lab/pymovements
Pull request #349 (2023-05-09 12:30:55)

feat(dataset): Add verbose parameter to more dataset methods

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) Report
Patch coverage: **`100.00`**% and project coverage change: **`-0.17`** :warning:
> Comparison is base [(`ad7f461`)](https://app.codecov.io/gh/aeye-lab/pymovements/commit/ad7f461b4ae95b378954648138b64288844fad1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) 100.00% compared to head [(`59f0c0e`)](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) 99.83%.

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##              main     #349      +/-   ##
===========================================
- Coverage   100.00%   99.83%   -0.17%     
===========================================
  Files           54       54              
  Lines         1849     1858       +9     
  Branches       403      409       +6     
===========================================
+ Hits          1849     1855       +6     
- Misses           0        2       +2     
- Partials         0        1       +1     
```


| [Impacted Files](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab) | Coverage Δ | |
|---|---|---|
| [src/pymovements/dataset/dataset.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2RhdGFzZXQvZGF0YXNldC5weQ==) | `100.00% <100.00%> (ø)` | |
| [src/pymovements/dataset/dataset\_download.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL2RhdGFzZXQvZGF0YXNldF9kb3dubG9hZC5weQ==) | `100.00% <100.00%> (ø)` | |
| [src/pymovements/utils/archives.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL3V0aWxzL2FyY2hpdmVzLnB5) | `95.83% <100.00%> (-4.17%)` | :arrow_down: |
| [src/pymovements/utils/downloads.py](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab#diff-c3JjL3B5bW92ZW1lbnRzL3V0aWxzL2Rvd25sb2Fkcy5weQ==) | `100.00% <100.00%> (ø)` | |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/aeye-lab/pymovements/pull/349?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aeye-lab).

dkrako: The `int` only makes sense if there are different levels of verbosity implemented.
Otherwise it is misleading.

Moreover, the `bool` is used only for the method in `dataset_download.py` which is not part of the public API.

Patch:
diff --git a/src/pymovements/dataset/dataset.py b/src/pymovements/dataset/dataset.py
index 32ed895..cedf429 100644
--- a/src/pymovements/dataset/dataset.py
+++ b/src/pymovements/dataset/dataset.py
@@ -578,7 +578,13 @@ class Dataset:
         )
         return self
 
-    def download(self, *, extract: bool = True, remove_finished: bool = False) -> Dataset:
+    def download(
+            self,
+            *,
+            extract: bool = True,
+            remove_finished: bool = False,
+            verbose: int = 1,
+    ) -> Dataset:
         """Download dataset resources.
 
         This downloads all resources of the dataset. Per default this also extracts all archives
@@ -598,6 +604,10 @@ class Dataset:
             Extract dataset archive files.
         remove_finished : bool
             Remove archive files after extraction.
+        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.
+            (2) Print additional messages for each recursive archive extract.
 
         Raises
         ------
@@ -616,16 +626,21 @@ class Dataset:
             paths=self.paths,
             extract=extract,
             remove_finished=remove_finished,
+            verbose=bool(verbose),
         )
         return self
 
-    def extract(self, remove_finished: bool = False) -> Dataset:
+    def extract(self, remove_finished: bool = False, verbose: int = 1) -> Dataset:
         """Extract downloaded dataset archive files.
 
         Parameters
         ----------
         remove_finished : bool
             Remove archive files after extraction.
+        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 extract.
 
         Returns
         -------
@@ -636,6 +651,7 @@ class Dataset:
             definition=self.definition,
             paths=self.paths,
             remove_finished=remove_finished,
+            verbose=verbose,
         )
         return self
 
diff --git a/src/pymovements/dataset/dataset_download.py b/src/pymovements/dataset/dataset_download.py
index 3e7f5cf..0b4b20a 100644
--- a/src/pymovements/dataset/dataset_download.py
+++ b/src/pymovements/dataset/dataset_download.py
@@ -33,6 +33,7 @@ def download_dataset(
         paths: DatasetPaths,
         extract: bool = True,
         remove_finished: bool = False,
+        verbose: bool = True,
 ) -> None:
     """Download dataset resources.
 
@@ -57,6 +58,9 @@ def download_dataset(
         Extract dataset archive files.
     remove_finished : bool
         Remove archive files after extraction.
+    verbose : bool
+        If True, show progress of download and print status messages for integrity checking and
+        file extraction.
 
     Raises
     ------
@@ -86,13 +90,14 @@ def download_dataset(
                     dirpath=paths.downloads,
                     filename=resource['filename'],
                     md5=resource['md5'],
+                    verbose=verbose,
                 )
                 success = True
 
             # pylint: disable=overlapping-except
             except (URLError, OSError, RuntimeError) as error:
-                print(f'Failed to download (trying next):\n{error}')
-                # downloading the resource, try next mirror
+                # Error downloading the resource, try next mirror
+                print(f'Failed to download:\n{error}\nTrying next mirror.')
                 continue
 
             # downloading the resource was successful, we don't need to try another mirror
@@ -108,6 +113,7 @@ def download_dataset(
             definition=definition,
             paths=paths,
             remove_finished=remove_finished,
+            verbose=verbose,
         )
 
 
@@ -115,6 +121,7 @@ def extract_dataset(
         definition: DatasetDefinition,
         paths: DatasetPaths,
         remove_finished: bool = False,
+        verbose: int = 1,
 ) -> None:
     """Extract downloaded dataset archive files.
 
@@ -126,13 +133,21 @@ def extract_dataset(
         The dataset paths.
     remove_finished : bool
         Remove archive files after extraction.
+    verbose:
+        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
+        each recursive archive extract.
     """
     paths.raw.mkdir(parents=True, exist_ok=True)
 
     for resource in definition.resources:
+        source_path = paths.downloads / resource['filename']
+        destination_path = paths.raw
+
         extract_archive(
-            source_path=paths.downloads / resource['filename'],
-            destination_path=paths.raw,
+            source_path=source_path,
+            destination_path=destination_path,
             recursive=True,
             remove_finished=remove_finished,
+            verbose=verbose,
         )
diff --git a/src/pymovements/utils/archives.py b/src/pymovements/utils/archives.py
index 062d568..77d4db8 100644
--- a/src/pymovements/utils/archives.py
+++ b/src/pymovements/utils/archives.py
@@ -39,6 +39,7 @@ def extract_archive(
         destination_path: Path | None = None,
         recursive: bool = True,
         remove_finished: bool = False,
+        verbose: int = 1,
 ) -> Path:
     """Extract an archive.
     The archive type and a possible compression is automatically detected from the file name.
@@ -55,6 +56,10 @@ def extract_archive(
         Recursively extract archives which are included in extracted archive.
     remove_finished : bool
         If ``True``, remove the file after the extraction.
+    verbose:
+        Verbosity levels: (1) Print messages for extracting each dataset resource without printing
+        messages for recursive archives. (2) Print additional messages for each recursive archive
+        extract.
 
     Returns
     -------
@@ -76,6 +81,9 @@ def extract_archive(
     if destination_path is None:
         destination_path = source_path.parent
 
+    if verbose:
+        print(f'Extracting {source_path.name} to {destination_path}')
+
     # Extract file and remove archive if desired.
     extractor(source_path, destination_path, compression_type)
     if remove_finished:
@@ -100,6 +108,7 @@ def extract_archive(
                 destination_path=extract_destination,
                 recursive=recursive,
                 remove_finished=remove_finished,
+                verbose=0 if verbose < 2 else 2,
             )
 
     return destination_path
diff --git a/src/pymovements/utils/downloads.py b/src/pymovements/utils/downloads.py
index 99fb1ff..8ea1ae4 100644
--- a/src/pymovements/utils/downloads.py
+++ b/src/pymovements/utils/downloads.py
@@ -42,6 +42,7 @@ def download_and_extract_archive(
         md5: str | None = None,
         recursive: bool = True,
         remove_finished: bool = False,
+        verbose: int = 1,
 ) -> None:
     """Download and extract archive file.
 
@@ -61,6 +62,10 @@ def download_and_extract_archive(
         Recursively extract archives which are included in extracted archive.
     remove_finished : bool
         Remove downloaded file after successful extraction or decompression, 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.
+        (2) Print additional messages for each recursive archive extract.
 
     Raises
     ------
@@ -73,17 +78,18 @@ def download_and_extract_archive(
         dirpath=download_dirpath,
         filename=download_filename,
         md5=md5,
+        verbose=bool(verbose),
     )
 
     if extract_dirpath is None:
         extract_dirpath = download_dirpath
 
-    print(f'Extracting {archive_path.name} to {extract_dirpath}')
     extract_archive(
         source_path=archive_path,
         destination_path=extract_dirpath,
         recursive=recursive,
         remove_finished=remove_finished,
+        verbose=verbose,
     )
 
 
@@ -93,6 +99,7 @@ def download_file(
         filename: str,
         md5: str | None = None,
         max_redirect_hops: int = 3,
+        verbose: bool = True,
 ) -> Path:
     """Download a file from a URL and place it in root.
 
@@ -108,6 +115,8 @@ def download_file(
         MD5 checksum of downloaded file. If None, do not check.
     max_redirect_hops : int, optional
         Maximum number of redirect hops allowed.
+    verbose : bool
+        If True, show progress bar and print info messages on downloading file.
 
     Returns
     -------
@@ -127,27 +136,34 @@ def download_file(
 
     # check if file is already present locally
     if _check_integrity(filepath, md5):
-        print('Using already downloaded and verified file:', filepath)
+        if verbose:
+            print('Using already downloaded and verified file:', filepath)
         return filepath
-    print(f'Downloading {url} to {filepath}')
+
+    if verbose:
+        print(f'Downloading {url} to {filepath}')
 
     # expand redirect chain if needed
     url = _get_redirected_url(url=url, max_hops=max_redirect_hops)
 
     # download the file
     try:
-        _download_url(url=url, destination=filepath)
+        _download_url(url=url, destination=filepath, verbose=verbose)
 
     except OSError as e:
         if url[:5] == 'https':
-            url = url.replace('https:', 'http:')
             print('Download failed. Trying https -> http instead.')
-            print(f'Downloading {url} to {filepath}')
-            _download_url(url=url, destination=filepath)
+            url = url.replace('https:', 'http:')
+
+            if verbose:
+                print(f'Downloading {url} to {filepath}')
+            _download_url(url=url, destination=filepath, verbose=verbose)
         else:
             raise e
 
     # check integrity of downloaded file
+    if verbose:
+        print(f'Checking integrity of {filepath.name}')
     if not _check_integrity(filepath=filepath, md5=md5):
         raise RuntimeError(f'File {filepath} not found or download corrupted.')
 
@@ -215,7 +231,7 @@ class _DownloadProgressBar(tqdm):  # pylint: disable=inconsistent-mro
         return self.update(b * bsize - self.n)  # also sets self.n = b * bsize
 
 
-def _download_url(url: str, destination: Path) -> None:
+def _download_url(url: str, destination: Path, verbose: bool = True) -> None:
     """Download file from URL and save to destination.
 
     Parameters
@@ -224,8 +240,10 @@ def _download_url(url: str, destination: Path) -> None:
         URL of file to be downloaded.
     destination : Path
         Destination path of downloaded file.
+    verbose : bool
+        If True, show progressbar.
     """
-    with _DownloadProgressBar(desc=destination.name) as t:
+    with _DownloadProgressBar(desc=destination.name, disable=not verbose) as t:
         urllib.request.urlretrieve(url=url, filename=destination, reporthook=t.update_to)
         t.total = t.n
 


Test patch:
diff --git a/tests/dataset/dataset_download_test.py b/tests/dataset/dataset_download_test.py
index 3e97395..da383b6 100644
--- a/tests/dataset/dataset_download_test.py
+++ b/tests/dataset/dataset_download_test.py
@@ -126,12 +126,14 @@ def test_dataset_download_both_mirrors_fail(mock_download_file, tmp_path, datase
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
         mock.call(
             url='https://another_example.com/test.gz.tar',
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
     ])
 
@@ -150,12 +152,14 @@ def test_dataset_download_first_mirror_fails(mock_download_file, tmp_path, datas
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
         mock.call(
             url='https://another_example.com/test.gz.tar',
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
     ])
 
@@ -176,6 +180,7 @@ def test_dataset_download_file_not_found(mock_download_file, tmp_path, dataset_d
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
     ])
 
@@ -194,6 +199,7 @@ def test_dataset_download_no_extract(mock_download_file, tmp_path, dataset_defin
             dirpath=tmp_path / 'downloads',
             filename='test.gz.tar',
             md5='52bbf03a7c50ee7152ccb9d357c2bb30',
+            verbose=True,
         ),
     ])
 
@@ -208,7 +214,7 @@ def test_dataset_extract_remove_finished_true(
 
     paths = pm.DatasetPaths(root=tmp_path, dataset='.')
     dataset = pm.Dataset(dataset_definition, path=paths)
-    dataset.extract(remove_finished=True)
+    dataset.extract(remove_finished=True, verbose=1)
 
     mock_extract_archive.assert_has_calls([
         mock.call(
@@ -216,6 +222,7 @@ def test_dataset_extract_remove_finished_true(
             destination_path=tmp_path / 'raw',
             recursive=True,
             remove_finished=True,
+            verbose=1,
         ),
     ])
 
@@ -238,6 +245,7 @@ def test_dataset_extract_remove_finished_false(
             destination_path=tmp_path / 'raw',
             recursive=True,
             remove_finished=False,
+            verbose=1,
         ),
     ])
 
diff --git a/tests/utils/downloads_test.py b/tests/utils/downloads_test.py
index e6502e3..d9133aa 100644
--- a/tests/utils/downloads_test.py
+++ b/tests/utils/downloads_test.py
@@ -32,12 +32,19 @@ from pymovements.utils.downloads import download_and_extract_archive
 from pymovements.utils.downloads import download_file
 
 
-def test_download_file(tmp_path):
+@pytest.mark.parametrize(
+    'verbose',
+    [
+        pytest.param(False, id='verbose_false'),
+        pytest.param(True, id='verbose_true'),
+    ],
+)
+def test_download_file(tmp_path, verbose):
     url = 'https://github.com/aeye-lab/pymovements/archive/refs/tags/v0.4.0.tar.gz'
     filename = 'pymovements-0.4.0.tar.gz'
     md5 = '52bbf03a7c50ee7152ccb9d357c2bb30'
 
-    filepath = download_file(url, tmp_path, filename, md5)
+    filepath = download_file(url, tmp_path, filename, md5, verbose=verbose)
 
     assert filepath.exists()
     assert filepath.name == filename
@@ -68,7 +75,14 @@ def test_download_file_404(tmp_path):
         download_file(url, tmp_path, filename, md5)
 
 
-def test_download_file_https_failure(tmp_path):
+@pytest.mark.parametrize(
+    'verbose',
+    [
+        pytest.param(False, id='verbose_false'),
+        pytest.param(True, id='verbose_true'),
+    ],
+)
+def test_download_file_https_failure(tmp_path, verbose):
     url = 'https://github.com/aeye-lab/pymovements/archive/refs/tags/v0.4.0.tar.gz'
     filename = 'pymovements-0.4.0.tar.gz'
     md5 = '52bbf03a7c50ee7152ccb9d357c2bb30'
@@ -78,7 +92,7 @@ def test_download_file_https_failure(tmp_path):
         side_effect=OSError(),
     ):
         with pytest.raises(OSError):
-            download_file(url, tmp_path, filename, md5)
+            download_file(url, tmp_path, filename, md5, verbose=verbose)
 
 
 def test_download_file_http_failure(tmp_path):
@@ -157,26 +171,46 @@ def test_download_and_extract_archive(tmp_path):
         assert hashlib.md5(file_bytes).hexdigest() == md5
 
 
-def test_download_and_extract_archive_extract_dirpath_None(tmp_path, capsys):
+@pytest.mark.parametrize(
+    'verbose',
+    [
+        pytest.param(False, id='verbose_false'),
+        pytest.param(True, id='verbose_true'),
+    ],
+)
+def test_download_and_extract_archive_extract_dirpath_None(tmp_path, capsys, verbose):
     url = 'https://github.com/aeye-lab/pymovements/archive/refs/tags/v0.4.0.tar.gz'
     download_filename = 'pymovements-0.4.0.tar.gz'
     md5 = '52bbf03a7c50ee7152ccb9d357c2bb30'
     extract_dirpath = None
 
     # extract first time
-    download_and_extract_archive(url, tmp_path, download_filename, extract_dirpath, md5)
+    download_and_extract_archive(
+        url, tmp_path, download_filename, extract_dirpath, md5, verbose=verbose,
+    )
     out, _ = capsys.readouterr()
-    assert out == 'Downloading '\
-        'https://github.com/aeye-lab/pymovements/archive/refs/tags/v0.4.0.tar.gz'\
-        f" to {os.path.join(tmp_path, 'pymovements-0.4.0.tar.gz')}\n"\
-        f'Extracting pymovements-0.4.0.tar.gz to {tmp_path}\n'
+
+    if verbose:
+        assert out == 'Downloading '\
+            'https://github.com/aeye-lab/pymovements/archive/refs/tags/v0.4.0.tar.gz'\
+            f" to {os.path.join(tmp_path, 'pymovements-0.4.0.tar.gz')}\n" \
+            f'Checking integrity of pymovements-0.4.0.tar.gz\n'\
+            f'Extracting pymovements-0.4.0.tar.gz to {tmp_path}\n'
+    else:
+        assert out == ''
 
     # extract second time to test already downloaded and verified file
-    download_and_extract_archive(url, tmp_path, download_filename, extract_dirpath, md5)
+    download_and_extract_archive(
+        url, tmp_path, download_filename, extract_dirpath, md5, verbose=verbose,
+    )
     out, _ = capsys.readouterr()
-    assert out == f'Using already downloaded and verified file: '\
-        f"{os.path.join(tmp_path, 'pymovements-0.4.0.tar.gz')}"\
-        f'\nExtracting pymovements-0.4.0.tar.gz to {tmp_path}\n'
+
+    if verbose:
+        assert out == f'Using already downloaded and verified file: '\
+            f"{os.path.join(tmp_path, 'pymovements-0.4.0.tar.gz')}"\
+            f'\nExtracting pymovements-0.4.0.tar.gz to {tmp_path}\n'
+    else:
+        assert out == ''
 
 
 def test_download_and_extract_archive_invalid_md5(tmp_path):