Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This PR addresses issues found in https://github.com/valory-xyz/open-autonomy/pull/1701 #518

Merged
merged 16 commits into from
Jan 4, 2023
Merged

This PR addresses issues found in https://github.com/valory-xyz/open-autonomy/pull/1701 #518

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5e12b4d
fix: IPFS plugin path issue
angrybayblade Jan 3, 2023
39acbf4
fix: load agent packages before running tests
angrybayblade Jan 3, 2023
b7bea86
fix: copyright headers
angrybayblade Jan 3, 2023
6ab74a5
fix: generate latest protocols
angrybayblade Jan 3, 2023
ec5d419
chore: hashes
angrybayblade Jan 3, 2023
16c6cf0
chore: generate test protocols
angrybayblade Jan 3, 2023
7688d89
chore: hashes
angrybayblade Jan 3, 2023
4270ca9
chore: copyright headers
angrybayblade Jan 3, 2023
6fedfca
fix: package manager fingerprint check test
angrybayblade Jan 4, 2023
772df27
chore: copyright
angrybayblade Jan 4, 2023
0a9f855
fix: cert dates for libp2p connections
angrybayblade Jan 4, 2023
2c94de2
chore: copyright
angrybayblade Jan 4, 2023
7427cd5
chore: bump open-aea and ipfs cli plugin to post releases
angrybayblade Jan 4, 2023
040cda5
chore: copyright
angrybayblade Jan 4, 2023
49181bc
fix: update aea version specifier test
angrybayblade Jan 4, 2023
b77ab2d
chore: copyright
angrybayblade Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion aea/cli/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
from aea.cli.utils.context import Context
from aea.cli.utils.decorators import check_aea_project
from aea.cli.utils.package_utils import get_package_path
from aea.components.base import load_aea_package
from aea.components.base import load_aea_package, perform_load_aea_package
from aea.configurations.base import ComponentConfiguration
from aea.configurations.constants import (
AEA_TEST_DIRNAME,
Expand Down Expand Up @@ -437,6 +437,13 @@ def load_package(
)
configuration.directory = package_dir
load_aea_packages_recursively(configuration, package_path_finder, root_packages)
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,
)
Comment on lines +440 to +446
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the test for this branch? Why was it not required before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


test_package_dir = package_dir / AEA_TEST_DIRNAME
enforce(
Expand Down
17 changes: 8 additions & 9 deletions plugins/aea-cli-ipfs/aea_cli_ipfs/ipfs_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,29 +324,28 @@ def download(
:return: downloaded path
"""

def move_to_target_dir(download_tmp_path) -> str:
def move_to_target_dir(download_tmp_path: Path) -> str:
"""Move downloaded content to target directory"""
if download_tmp_path.is_file():
# file is ok!
# Downloaded object is a file and can be moved easily
shutil.move(str(download_tmp_path), str(target_dir))
return str(target_dir)

if fix_path:
# identify name of package
paths = list(download_tmp_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)
Comment on lines -339 to -340
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

package_path = paths.pop()
if len(paths) == 1:
(package_path,) = paths
else:
package_path = download_tmp_path
Comment on lines +337 to +340
Copy link
Author

@angrybayblade angrybayblade Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please open an issue for this so when we're working #519 we work on this afterwards


# move content of directory with hashid to target dir
shutil.move(str(package_path), str(target_dir))
return str(target_dir / package_path.name)
return str(Path(target_dir, package_path.name))

# move directory with hash id to target dir
shutil.move(str(download_tmp_path), str(target_dir))
return str(target_dir / download_tmp_path.name)
return str(Path(target_dir, download_tmp_path.name))

target_dir = Path(target_dir)
target_dir.mkdir(exist_ok=True, parents=True)
Expand Down