Merge pull request #480 from valory-xyz/fix/ipfs_download_retries

[v1.27.0] Refactor: IPFS download

docs/api/plugins/aea_cli_ipfs/ipfs_utils.md

@@ -277,15 +277,15 @@ Remove dir added by it's hash.
 #### download
 
 ```python
-def download(hash_id: str, target_dir: str, fix_path: bool = True, attempts: int = 5) -> str
+def download(hash_id: str, target_dir: Union[str, Path], fix_path: bool = True, attempts: int = 5) -> str
 ```
 
 Download dir by its hash.
 
 **Arguments**:
 
-- `hash_id`: str. hash of file to download
-- `target_dir`: str. directory to place downloaded
+- `hash_id`: str. hash of file or package to download
+- `target_dir`: Union[str, Path]. directory to place downloaded
 - `fix_path`: bool. default True. on download don't wrap result in to hash_id directory.
 - `attempts`: int. default 5. How often to attempt the download.
 

docs/package_list.md

@@ -33,9 +33,9 @@
 | connection/valory/test_libp2p/0.1.0                           | `bafybeigbkr22vy4bxfl3hohjzwxj76wn5y77ssrsdeqqiu3a7yrpkqpfle` |
 | protocol/fetchai/tac/1.0.0                                    | `bafybeiaew226n32rwp3h57zl4b2mmbrhjbyrdjbl2evnxf2tmmi4vrls7a` |
 | skill/fetchai/erc1155_client/0.28.0                           | `bafybeiecwwk7nesipstq7acvrkjv2byze27t2n6jlnxwggl3ppcbybuoum` |
-| skill/fetchai/erc1155_deploy/0.30.0                           | `bafybeiapnneoqjkb7wzzbdsvrs6jga6kstmiw6rkqgvbwk4jmwyisc2rbi` |
+| skill/fetchai/erc1155_deploy/0.30.0                           | `bafybeihmtiouapqz35is2hx6x5k6nrdnnjtz3uoywe34oyrxqg7na6u4wu` |
 | skill/fetchai/error/0.17.0                                    | `bafybeib7nhokw3bc46oxuk5mjazan42evipowmka2ikfcs6drcdz4mwkjm` |
 | skill/fetchai/fipa_dummy_buyer/0.2.0                          | `bafybeiha4jultg5srhr2ijplvubeo7esv4raq2cjlggmyzcaimop2ggg2m` |
-| skill/fetchai/generic_buyer/0.26.0                            | `bafybeicxrsrb3bg3c3uziojqzlocw74tp665q6of34zvcprl6ffccq6klu` |
+| skill/fetchai/generic_buyer/0.26.0                            | `bafybeial7pz6vebq6mq7v2aiagmqirto3jffurfsysjmd2ujhheqwwogoa` |
 | skill/fetchai/generic_seller/0.27.0                           | `bafybeiefbhuiax2bqgmrztugtii36ob2hw7tsreoyqnsrqxsqz36bdfjfy` |
 | skill/fetchai/task_test_skill/0.1.0                           | `bafybeidv77u2xl52mnxakwvh7fuh46aiwfpteyof4eaptfd4agoi6cdble` |

plugins/aea-cli-ipfs/aea_cli_ipfs/ipfs_utils.py

@@ -18,6 +18,7 @@
 #
 # ------------------------------------------------------------------------------
 """Ipfs utils for `ipfs cli command`."""
+
 import logging
 import os
 import shutil
@@ -26,7 +27,7 @@
 import tempfile
 import time
 from pathlib import Path
-from typing import Dict, IO, List, Optional, Set, Tuple, cast
+from typing import Dict, IO, List, Optional, Set, Tuple, Union, cast
 
 import ipfshttpclient  # type: ignore
 import requests
@@ -309,68 +310,55 @@ def remove_unpinned_files(self) -> None:
     def download(
         self,
         hash_id: str,
-        target_dir: str,
+        target_dir: Union[str, Path],
         fix_path: bool = True,
         attempts: int = 5,
     ) -> str:
         """
         Download dir by its hash.
 
-        :param hash_id: str. hash of file to download
-        :param target_dir: str. directory to place downloaded
+        :param hash_id: str. hash of file or package to download
+        :param target_dir: Union[str, Path]. directory to place downloaded
         :param fix_path: bool. default True. on download don't wrap result in to hash_id directory.
         :param attempts: int. default 5. How often to attempt the download.
         :return: downloaded path
         """
 
-        if not os.path.exists(target_dir):  # pragma: nocover
-            os.makedirs(target_dir, exist_ok=True)
+        def move_to_target_dir(download_path) -> str:
+            """Move downloaded content to target directory"""
+
+            if download_path.is_file():
+                shutil.copy(download_path, target_dir)
+                return str(target_dir)
+
+            # else it is a directory containing a single package path
+            package_path = download_path
+            if fix_path:
+                # assumption is it contains one nested directory: the package
+                paths = list(download_path.glob("*"))
+                assumption_is_valid = len(paths) == 1 and paths[0].is_dir()
+                if not assumption_is_valid:  # pragma: no cover
+                    error_msg = f"Expected a single directory, found: {paths}"
+                    raise DownloadError(error_msg)
+                package_path = paths.pop()
 
-        if os.path.exists(os.path.join(target_dir, hash_id)):  # pragma: nocover
-            raise DownloadError(f"{hash_id} was already downloaded to {target_dir}")
+            package_path.rename(target_dir / package_path.name)
+            return str(package_path)
+
+        target_dir = Path(target_dir)
+        target_dir.mkdir(exist_ok=True, parents=True)
 
-        downloaded_path = str(Path(target_dir) / hash_id)
         while attempts:
             attempts -= 1
             try:  # download to tmp_dir in case of midway download failure
                 with tempfile.TemporaryDirectory() as tmp_dir:
                     self.client.get(hash_id, tmp_dir)
-                    download_path = Path(tmp_dir) / hash_id
-                    if download_path.is_dir():
-                        shutil.copytree(download_path, downloaded_path)
-                    else:
-                        shutil.copy(download_path, downloaded_path)
-                    break
+                    return move_to_target_dir(Path(tmp_dir) / hash_id)
             except ipfshttpclient.exceptions.StatusError as e:
                 logging.error(f"error on download of {hash_id}: {e}")
                 time.sleep(1)
-        else:
-            raise DownloadError(f"Failed to download: {hash_id}")
-
-        package_path = None
-        if os.path.isdir(downloaded_path):
-            downloaded_files = os.listdir(downloaded_path)
-            if len(downloaded_files) > 0:
-                package_name, *_ = os.listdir(downloaded_path)
-                package_path = str(Path(target_dir) / package_name)
-
-        if package_path is None:
-            package_path = target_dir
-
-        if fix_path and Path(downloaded_path).is_dir():
-            # self.client.get creates result with hash name
-            # and content, but we want content in the target dir
-            try:
-                for each_file in Path(downloaded_path).iterdir():  # grabs all files
-                    shutil.move(str(each_file), target_dir)
-            except shutil.Error as e:  # pragma: nocover
-                if os.path.isdir(downloaded_path):
-                    shutil.rmtree(downloaded_path)
-                raise DownloadError(f"error on move files {str(e)}") from e
-
-        if os.path.isdir(downloaded_path):
-            shutil.rmtree(downloaded_path)
-        return package_path
+
+        raise DownloadError(f"Failed to download: {hash_id}")
 
     def publish(self, hash_id: str) -> Dict:
         """

plugins/aea-cli-ipfs/tests/test_aea_cli_ipfs.py

@@ -238,14 +238,6 @@ def new_callable(*_, **__) -> None:
 
         return patch("ipfshttpclient.Client.get", new_callable=lambda: new_callable)
 
-    def test_ipfs_download_target_path_exists(self) -> None:
-        """Test aea ipfs download target_path exists."""
-
-        Path(self.target_dir, self.some_ipfs_hash).mkdir(parents=True)
-        expected = f"{self.some_ipfs_hash} was already downloaded"
-        with pytest.raises(click.ClickException, match=expected):
-            self.run_cli(*self.args, catch_exceptions=False, standalone_mode=False)
-
     @pytest.mark.parametrize("is_dir", [False, True])
     def test_ipfs_download_success(self, is_dir: bool) -> None:
         """Test aea ipfs download."""

plugins/aea-cli-ipfs/tests/test_utils.py

@@ -19,9 +19,7 @@
 
 """Test ipfs utils."""
 
-import os
 import re
-from pathlib import Path
 from tempfile import TemporaryDirectory
 from unittest.mock import Mock, patch
 
@@ -217,7 +215,7 @@ def test_tool_download() -> None:
     with patch.object(
         ipfs_tool, "client", client_mock
     ), TemporaryDirectory() as tmp_dir, patch(
-        "os.path.isdir", return_value=True
+        "pathlib.Path.is_file", return_value=True
     ), patch(
         "shutil.copy"
     ), patch(
@@ -228,13 +226,10 @@ def test_tool_download() -> None:
         "shutil.rmtree"
     ):
         with pytest.raises(DownloadError, match="Failed to download: some"):
-            ipfs_tool.download("some", tmp_dir, attempts=5)
+            with patch("time.sleep"):
+                ipfs_tool.download("some", tmp_dir, attempts=5)
         assert client_mock.get.call_count == 5
 
         client_mock.get = Mock()
 
         ipfs_tool.download("some", tmp_dir, attempts=5)
-
-        with pytest.raises(DownloadError, match="was already downloaded to"):
-            os.mkdir(Path(tmp_dir) / "some")
-            ipfs_tool.download("some", tmp_dir, attempts=5)