[v1.27.0] Refactor: IPFS download #480
Conversation
Karrenbelt
force-pushed
the
fix/ipfs_download_retries
branch
from
e884054 to
91284fa
Compare
Codecov ReportBase: 90.30% // Head: 90.30% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## tests/ipfs_download #480 +/- ##
=======================================================
- Coverage 90.30% 90.30% -0.01%
=======================================================
Files 352 352
Lines 28702 28695 -7
=======================================================
- Hits 25920 25912 -8
- Misses 2782 2783 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Karrenbelt
force-pushed
the
fix/ipfs_download_retries
branch
from
724d2ce to
a9f6b4c
Compare
Karrenbelt
changed the title
DavidMinarsch
changed the title
|
This will need to wait till the morning before it's ready. When 489 is in I'll sync this one up. |
DavidMinarsch
changed the base branch from
tests/ipfs_download_retries
to
main
Karrenbelt
changed the title
| # 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() |
There was a problem hiding this comment.
added explicit check for the apparent assumption. Relates to this original piece of code
open-aea/plugins/aea-cli-ipfs/aea_cli_ipfs/ipfs_utils.py
Lines 350 to 358 in 67ac9a1
I refactored and introduced move_to_target_dir to encapsulate this logic.
There was a problem hiding this comment.
nice refactor.
but i dont like the mix of functionalities in download method.
i think we need quite simple download with retry method
ans some high level method that uses simple download and applies all the package related things
download method should know nothing about packages, files and how handle data downloaded.
There was a problem hiding this comment.
agree (as per #501), it's mostly that I try to limit the amount of changes in a single PR. Now with the nested function it should be easier to achieve in a future refactor. The issue with the refactor is that the fix_path (which is ALWAYS true) would need to be removed from the function arguments (and hence constitute a breaking change).
We could add a DeprecationWarning for version 2.0 for anyone that passes the kwarg. How we could do this is: 1. change the default to None (we want to detect anyone passing a boolean explicitly) and raise the warning in case it is not None.
| if os.path.exists(os.path.join(target_dir, hash_id)): # pragma: nocover | ||
| raise DownloadError(f"{hash_id} was already downloaded to {target_dir}") |
There was a problem hiding this comment.
no longer raising here. ipfshttpclient.Client.get also overwrites if the path already exists: if your local is different, your local is wrong.
Furthermore, if we fix_path and effectively remove the content up one directory, this check becomes useless.
|
|
||
| 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) |
There was a problem hiding this comment.
renaming a file or directory to one that already exists will throw an error.
| 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) | ||
|
|
There was a problem hiding this comment.
this tests is no longer relevant
|
|
||
| with pytest.raises(DownloadError, match="was already downloaded to"): | ||
| os.mkdir(Path(tmp_dir) / "some") | ||
| ipfs_tool.download("some", tmp_dir, attempts=5) |
Karrenbelt
force-pushed
the
fix/ipfs_download_retries
branch
from
13e12ef to
017afa2
Compare
|
failures
|
| else: | ||
| shutil.copy(download_path, downloaded_path) | ||
| break | ||
| return move_to_target_dir(Path(tmp_dir) / hash_id) |
There was a problem hiding this comment.
suppose move_to_target_dir(Path(tmp_dir) / hash_id) should not fail and can be extracted outside attempts loop
if file system operation fails, probably it's not recoverable
| # 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() |
There was a problem hiding this comment.
nice refactor.
but i dont like the mix of functionalities in download method.
i think we need quite simple download with retry method
ans some high level method that uses simple download and applies all the package related things
download method should know nothing about packages, files and how handle data downloaded.
DavidMinarsch
dismissed stale reviews from solarw and themself
The base branch was changed.
Proposed changes
Refactor and clean up
IPFSTool.download.partially addresses #501, which may be closed once this PR is accepted.
Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to agents-aea?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.developbranch (left side). Also you should start your branch off ourdevelop.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...