This PR addresses issues found in https://github.com/valory-xyz/open-autonomy/pull/1701 #518
Conversation
| else: | ||
| perform_load_aea_package( | ||
| dir_=package_dir, | ||
| author=package_dir.parent.parent.name, | ||
| package_type_plural=PackageType(package_type).to_plural(), | ||
| package_name=package_dir.name, | ||
| ) |
There was a problem hiding this comment.
This addresses https://github.com/valory-xyz/open-autonomy/actions/runs/3819359169/jobs/6496927078#step:7:9939 failure. We load agent packages in same manner as we load the component packages before running the tests
There was a problem hiding this comment.
Where is the test for this branch? Why was it not required before?
There was a problem hiding this comment.
We did not load the agent package before because there was no code part of the agent component similar to other components so we could run the test without loading the agent package.
When I was recreating this issue locally I found out that this issue only occurs in one specific condition that is if you run the tests in a tox env and all packages available. When I ran tests for agents only by modifying the test command they ran without any issue and when I ran the same command we run in the tox env normally they ran without any issues as well.
So my assumption is since we're loading the components other than the agents the references in the sys.modules does not contain references to the packages.author.components and not the packages.author.agents which is the reason why the pytest was throwing import error.
Now this did not happen before, this started happening after the release of 1.27.0 and we haven't changed anything in the test command except for the #504 which does not look like could've caused the issue so this is still a mystery for me why this started happening suddenly
| error_msg = f"Expected a single directory, found: {paths}" | ||
| raise DownloadError(error_msg) |
There was a problem hiding this comment.
The downloaded object can be either a file or a directory, which can be wrapped under a wrapper node. So this gives 4 possibilities for a downloaded object. A file, file wrapped, dir and dir wrapped. We need a better mechanism to detect these 4 and filter out aea component packages. #519
| if len(paths) == 1: | ||
| (package_path,) = paths | ||
| else: | ||
| package_path = download_tmp_path |
There was a problem hiding this comment.
Continuing from #518 (comment), this will fix the issue but we need to make this method more robust in terms of working with different types of download objects
There was a problem hiding this comment.
@angrybayblade @Karrenbelt
i think it's better to get rid of package terms in ipfs download tool.
the main purpose is to download:
- a single file
- a directory
and doesnt matter what is actually inside.
wrapped single file is a directory actually. with one file inside and it's filename
a directory downloaded by hash stored (default ipfs cli behaviour) as <target dir><hash>/<dir content> sometimes we want to have <target dir>/<dir contet> without dir with name <hash>
quite simple logic. content related operations should be done on the upper level.
my conclusion:
make ipfstool as simple as possible (download dir, file, retries)
content related things should be implemented somewhere else.
There was a problem hiding this comment.
Please open an issue for this so when we're working #519 we work on this afterwards
| @@ -23,7 +23,7 @@ | |||
| __title__ = "open-aea" | |||
| __description__ = "Open Autonomous Economic Agent framework (without vendor lock-in)" | |||
| __url__ = "https://github.com/valory-xyz/open-aea.git" | |||
| __version__ = "1.27.0" | |||
| __version__ = "1.27.0.post1" | |||
| LIBP2P_CERT_NOT_BEFORE = "2023-01-01" | ||
| LIBP2P_CERT_NOT_AFTER = "2024-01-01" |
There was a problem hiding this comment.
Date range update for the certificates
| update_aea_version_range(package_configuration=package_config) | ||
| assert not package_config.aea_version_specifiers.contains( | ||
| str(get_current_aea_version()) | ||
| ) | ||
|
|
||
| assert str(get_current_aea_version()) in package_config.aea_version | ||
| update_aea_version_range(package_configuration=package_config) | ||
| assert package_config.aea_version_specifiers.contains( | ||
| str(get_current_aea_version()) | ||
| ) |
There was a problem hiding this comment.
Instead of performing a strict check we perform a range check here. This check is failing because the current version of open-aea is 1.27.0.post1 and update_aea_version_range creates the version range as >=1.27.0, <2.0.0. We were checking if the exact version version (1.27.0.post1) was in the version range string or not and that's why the test failed. Now we're performing a check in the range.
Issue for the same #523
Codecov ReportBase: 92.90% // Head: 92.89% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
==========================================
- Coverage 92.90% 92.89% -0.01%
==========================================
Files 353 353
Lines 28799 28801 +2
==========================================
Hits 26756 26756
- Misses 2043 2045 +2
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. |
Proposed changes
This PR
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.