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.

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

Updates __eq__ implementation on the data types #445

Merged
merged 26 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
7cd21b5
fix: PackageVersion.__eq__
Karrenbelt Nov 16, 2022
064b845
fix: __eq__ PublicId, PackageId & Dependency
Karrenbelt Nov 16, 2022
cdbb001
fix: check_dependencies -> .without_hash()
Karrenbelt Nov 16, 2022
3b27e78
chore: linters
Karrenbelt Nov 16, 2022
0b33f7a
fix: remove _package_hash from PublicId.__eq__
Karrenbelt Nov 17, 2022
77474ff
Revert "fix: check_dependencies -> .without_hash()"
Karrenbelt Nov 17, 2022
36cf832
fix: deflection in __lt__ method
Karrenbelt Nov 17, 2022
05dbb00
fix: add functools.total_ordering to PublicId and PackageId
Karrenbelt Nov 17, 2022
f564c43
chore: linters
Karrenbelt Nov 17, 2022
09f7179
fix: error message in code and type in test
Karrenbelt Nov 17, 2022
4c118f4
tests: package_version_strategy
Karrenbelt Nov 17, 2022
75549e5
fix: test_self_type_comparison
Karrenbelt Nov 17, 2022
45e1c7a
chore: linters
Karrenbelt Nov 17, 2022
85c0c74
fix: delegate comparison to PackageVersionLike
Karrenbelt Nov 17, 2022
3b5a411
fix: remove `sorted` from DependencyNotFound.print_error
Karrenbelt Nov 17, 2022
64f19d0
fix: remove `sorted` from DependencyTree
Karrenbelt Nov 17, 2022
15b128c
fix: test_package_version_lt
Karrenbelt Nov 17, 2022
4784ac1
fix: remove sorted() from queue initialization
Karrenbelt Dec 17, 2022
24d3fd1
Merge branch 'tests/test_data_types' into fix/data_types__eq__
Karrenbelt Dec 17, 2022
f269b13
Merge branch 'fix/data_types__eq__' into fix/data_types__lt__
Karrenbelt Dec 17, 2022
779c951
Merge branch 'fix/data_types__lt__' into fix/delegate_to_PackageVersi…
Karrenbelt Dec 17, 2022
97e612a
tests: fix test_type_comparison
Karrenbelt Dec 18, 2022
1e08814
chore: linters
Karrenbelt Dec 18, 2022
f4aea8d
fix: sorted(roots, key=str)
Karrenbelt Dec 18, 2022
9276d3a
Merge pull request #448 from valory-xyz/fix/delegate_to_PackageVersio…
DavidMinarsch Dec 19, 2022
7281b27
Merge pull request #447 from valory-xyz/fix/data_types__lt__
DavidMinarsch Dec 19, 2022
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
8 changes: 4 additions & 4 deletions aea/cli/check_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ def __init__(

def print_error(self) -> None:
"""Print the error message."""
sorted_expected = list(map(str, sorted(self.expected_dependencies)))
sorted_missing = list(map(str, sorted(self.missing_dependencies)))
expected = list(map(str, self.expected_dependencies))
missing = list(map(str, self.missing_dependencies))
print("=" * 50)
print(f"Package {self.configuration_file}:")
print(f"Expected: {pprint.pformat(sorted_expected)}")
print(f"Missing: {pprint.pformat(sorted_missing)}")
print(f"Expected: {pprint.pformat(expected)}")
print(f"Missing: {pprint.pformat(missing)}")
print("=" * 50)


Expand Down
99 changes: 36 additions & 63 deletions aea/configurations/data_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
SERVICE,
SKILL,
)
from aea.exceptions import enforce
from aea.helpers.base import (
IPFSHash,
IPFSHashOrStr,
Expand Down Expand Up @@ -86,6 +85,8 @@ class PackageVersion:

_version: PackageVersionLike

__slots__ = ("_version",)

Comment on lines +88 to +89
Copy link
Author

Choose a reason for hiding this comment

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

such that a generic solution could be implemented that is equal for all classes in this module

def __init__(self, version_like: PackageVersionLike) -> None:
"""
Initialize a package version.
Expand Down Expand Up @@ -114,18 +115,15 @@ def __str__(self) -> str:

def __eq__(self, other: Any) -> bool:
"""Check equality."""
return isinstance(other, PackageVersion) and self._version == other._version
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance's __eq__.
return all(getattr(self, s) == getattr(other, s) for s in self.__slots__)

def __lt__(self, other: Any) -> bool:
"""Compare with another object."""
enforce(
isinstance(other, PackageVersion),
f"Cannot compare {type(self)} with type {type(other)}.",
)
other = cast(PackageVersion, other)
if self.is_latest or other.is_latest:
return self.is_latest < other.is_latest
return str(self) < str(other)
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
return self._version < other._version


class PackageType(Enum):
Expand Down Expand Up @@ -211,6 +209,7 @@ def __str__(self) -> str:
PackageIdPrefix = Tuple[ComponentType, str, str]


@functools.total_ordering
class PublicId(JSONSerializable):
"""This class implement a public identifier.

Expand Down Expand Up @@ -451,46 +450,20 @@ def __repr__(self) -> str:
return f"<{self}>"

def __eq__(self, other: Any) -> bool:
"""Compare with another object."""
return (
isinstance(other, PublicId)
and self.author == other.author
and self.name == other.name
and self.version == other.version
)
"""Check equality."""
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance's __eq__.
return all(getattr(self, s) == getattr(other, s) for s in self.__slots__[:-1])
Comment on lines +453 to +456
Copy link
Author

@Karrenbelt Karrenbelt Nov 17, 2022

Choose a reason for hiding this comment

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

including the _package_hash will affect the functionality in the following 4 places, which is why I chose not to include it now (it was not included in the original comparison either). We may choose to open an issue on this if we want to change this later.

  • eject

    open-aea/aea/cli/eject.py

    Lines 204 to 206 in 77474ff

    reverse_reachable_dependencies = reachable_nodes(
    reverse_dependencies, {package_id.without_hash()}
    )
  • remove

    open-aea/aea/cli/remove.py

    Lines 193 to 226 in 77474ff

    def get_item_dependencies_with_reverse_dependencies(
    self, item: PackageConfiguration, package_id: Optional[PackageId] = None
    ) -> Dict[PackageId, Set[PackageId]]:
    """
    Get item dependencies.
    It's recursive and provides all the sub dependencies.
    :param item: the item package configuration
    :param package_id: the package id.
    :return: dict with PackageId: and set of PackageIds that uses this package
    """
    result: defaultdict = defaultdict(set)
    for dep_package_id in self._get_item_requirements(
    item, self._ignore_non_vendor
    ):
    if package_id is None:
    _ = result[dep_package_id.without_hash()] # init default dict value
    else:
    result[dep_package_id.without_hash()].add(package_id.without_hash())
    if not self.is_present_in_agent_config(
    dep_package_id.without_hash()
    ): # pragma: nocover
    continue
    dep_item = self.get_item_config(dep_package_id)
    for item_key, deps in self.get_item_dependencies_with_reverse_dependencies(
    dep_item, dep_package_id
    ).items():
    result[item_key.without_hash()] = result[item_key].union(deps)
    return result
  • check_packages
    def _add_package_type(package_type: PackageType, public_id_str: str) -> PackageId:
    return PackageId(package_type, PublicId.from_str(public_id_str))
  • registry/add
    def fetch_package(obj_type: str, public_id: PublicId, cwd: str, dest: str) -> Path:
    """
    Fetch a package (connection/contract/protocol/skill) from Registry.
    :param obj_type: str type of object you want to fetch:
    'connection', 'protocol', 'skill'
    :param public_id: str public ID of object.
    :param cwd: str path to current working directory.
    :param dest: destination where to save package.
    :return: package path
    """
    logger.debug(
    "Fetching {obj_type} {public_id} from Registry...".format(
    public_id=public_id, obj_type=obj_type
    )
    )
    logger.debug(
    "Downloading {obj_type} {public_id}...".format(
    public_id=public_id, obj_type=obj_type
    )
    )
    package_meta = get_package_meta(obj_type, public_id)
    file_url = cast(str, package_meta["file"])
    filepath = download_file(file_url, cwd)
    # next code line is needed because the items are stored in tarball packages as folders
    dest = os.path.split(dest)[0]
    logger.debug(
    "Extracting {obj_type} {public_id}...".format(
    public_id=public_id, obj_type=obj_type
    )
    )
    extract(filepath, dest)
    logger.debug(
    "Successfully fetched {obj_type} '{public_id}'.".format(
    public_id=public_id, obj_type=obj_type
    )
    )
    package_path = os.path.join(dest, public_id.name)
    return Path(package_path)

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 and add a comment in code.

Copy link
Author

Choose a reason for hiding this comment

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


def __lt__(self, other: Any) -> bool:
"""
Compare two public ids.

>>> public_id_1 = PublicId("author_1", "name_1", "0.1.0")
>>> public_id_2 = PublicId("author_1", "name_1", "0.1.1")
>>> public_id_3 = PublicId("author_1", "name_2", "0.1.0")
>>> public_id_1 > public_id_2
False
>>> public_id_1 < public_id_2
True

>>> public_id_1 < public_id_3
Traceback (most recent call last):
...
ValueError: The public IDs author_1/name_1:0.1.0 and author_1/name_2:0.1.0 cannot be compared. Their author or name attributes are different.

:param other: the object to compate to
:raises ValueError: if the public ids cannot be confirmed
:return: whether or not the inequality is satisfied
"""
if (
isinstance(other, PublicId)
and self.author == other.author
and self.name == other.name
):
return self.package_version < other.package_version
raise ValueError(
"The public IDs {} and {} cannot be compared. Their author or name attributes are different.".format(
self, other
"""Compare with another object."""
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
if not (self.author == other.author and self.name == other.name):
raise TypeError(
f"The public IDs {self} and {other} cannot be compared. Their author or name attributes are different."
)
)
return self.package_version < other.package_version

def without_hash(
self,
Expand All @@ -517,6 +490,7 @@ def __str__(self) -> str:
)


@functools.total_ordering
class PackageId:
"""A package identifier."""

Expand Down Expand Up @@ -647,16 +621,20 @@ def __repr__(self) -> str:
return f"PackageId{self.__str__()}"

def __eq__(self, other: Any) -> bool:
"""Compare with another object."""
return (
isinstance(other, PackageId)
and self.package_type == other.package_type
and self.public_id == other.public_id
)
"""Check equality."""
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance's __eq__.
return all(getattr(self, s) == getattr(other, s) for s in self.__slots__)

Comment on lines 623 to 628
Copy link
Author

Choose a reason for hiding this comment

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

__slots__ = ("_package_type", "_public_id")

def __lt__(self, other: Any) -> bool:
"""Compare two public ids."""
return str(self) < str(other)
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance.
if not self.package_type == other.package_type:
raise TypeError(
f"The package IDs {self} and {other} cannot be compared. Their package_type is different."
)
return self.public_id < other.public_id


class ComponentId(PackageId):
Expand Down Expand Up @@ -874,15 +852,10 @@ def __str__(self) -> str:
return f"{self.__class__.__name__}(name='{self.name}', version='{self.version}', index='{self.index}', git='{self.git}', ref='{self.ref}')"

def __eq__(self, other: Any) -> bool:
"""Compare with another object."""
return (
isinstance(other, Dependency)
and self._name == other._name
and self._version == other._version
and self._index == other._index
and self._git == other._git
and self._ref == other._ref
)
"""Check equality."""
if not isinstance(other, self.__class__):
return NotImplemented # Delegate comparison to the other instance's __eq__.
return all(getattr(self, s) == getattr(other, s) for s in self.__slots__)

Comment on lines 854 to 859
Copy link
Author

Choose a reason for hiding this comment

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

__slots__ = ("_name", "_version", "_index", "_git", "_ref")


Dependencies = Dict[str, Dependency]
Expand Down
2 changes: 1 addition & 1 deletion aea/helpers/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ def find_topological_order(adjacency_list: Dict[T, Set[T]]) -> List[T]:
# compute the topological order
queue: Deque[T] = deque()
order = []
queue.extendleft(sorted(roots)) # type: ignore
queue.extendleft(sorted(roots, key=str)) # type: ignore
while len(queue) > 0:
current = queue.pop()
order.append(current)
Expand Down
9 changes: 4 additions & 5 deletions aea/helpers/dependency_tree.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,10 @@ def generate(
cls.flatten_tree(dep_tree, flat_tree_dirty, 0)

for tree_level in reversed(flat_tree_dirty[:-1]):
flat_tree.append(
sorted(
{package for package in tree_level if package not in dirty_packages}
)
)
flat_tree.append([])
for package in tree_level:
if package not in dirty_packages and package not in flat_tree[-1]:
flat_tree[-1].append(package)
dirty_packages += tree_level

return flat_tree
Expand Down
35 changes: 6 additions & 29 deletions docs/api/configurations/data_types.md
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ Get the string representation.
## PublicId Objects

```python
@functools.total_ordering
class PublicId(JSONSerializable)
```

Expand Down Expand Up @@ -521,7 +522,7 @@ Get the representation.
def __eq__(other: Any) -> bool
```

Compare with another object.
Check equality.

<a id="aea.configurations.data_types.PublicId.__lt__"></a>

Expand All @@ -531,32 +532,7 @@ Compare with another object.
def __lt__(other: Any) -> bool
```

Compare two public ids.

>>> public_id_1 = PublicId("author_1", "name_1", "0.1.0")
>>> public_id_2 = PublicId("author_1", "name_1", "0.1.1")
>>> public_id_3 = PublicId("author_1", "name_2", "0.1.0")
>>> public_id_1 > public_id_2
False
>>> public_id_1 < public_id_2
True

>>> public_id_1 < public_id_3
Traceback (most recent call last):
...
ValueError: The public IDs author_1/name_1:0.1.0 and author_1/name_2:0.1.0 cannot be compared. Their author or name attributes are different.

**Arguments**:

- `other`: the object to compate to

**Raises**:

- `ValueError`: if the public ids cannot be confirmed

**Returns**:

whether or not the inequality is satisfied
Compare with another object.

<a id="aea.configurations.data_types.PublicId.without_hash"></a>

Expand Down Expand Up @@ -593,6 +569,7 @@ Get the string representation.
## PackageId Objects

```python
@functools.total_ordering
class PackageId()
```

Expand Down Expand Up @@ -795,7 +772,7 @@ Get the object representation in string.
def __eq__(other: Any) -> bool
```

Compare with another object.
Check equality.

<a id="aea.configurations.data_types.PackageId.__lt__"></a>

Expand Down Expand Up @@ -1080,7 +1057,7 @@ Get the string representation.
def __eq__(other: Any) -> bool
```

Compare with another object.
Check equality.

<a id="aea.configurations.data_types.Dependencies"></a>

Expand Down
54 changes: 52 additions & 2 deletions docs/api/package_manager/base.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ def check_dependencies(configuration: PackageConfiguration) -> List[Tuple[Packag

Verify hashes for package dependecies againts the available hashes.

<a id="aea.package_manager.base.BasePackageManager.get_package_dependencies"></a>

#### get`_`package`_`dependencies

```python
def get_package_dependencies(package_id: PackageId) -> List[PackageId]
```

Get package dependencies by package_id.

<a id="aea.package_manager.base.BasePackageManager.update_public_id_hash"></a>

#### update`_`public`_`id`_`hash
Expand All @@ -93,6 +103,16 @@ def update_dependencies(package_id: PackageId) -> None

Update dependency hashes to latest for a package.

<a id="aea.package_manager.base.BasePackageManager.get_package_config_file"></a>

#### get`_`package`_`config`_`file

```python
def get_package_config_file(package_id: PackageId) -> Path
```

Get package config file path.

<a id="aea.package_manager.base.BasePackageManager.update_fingerprints"></a>

#### update`_`fingerprints
Expand All @@ -108,10 +128,40 @@ Update fingerprints for a package.
#### add`_`package

```python
def add_package(package_id: PackageId) -> "BasePackageManager"
def add_package(package_id: PackageId, with_dependencies: bool = False, allow_update: bool = False) -> "BasePackageManager"
```

Add package.

<a id="aea.package_manager.base.BasePackageManager.add_dependencies_for_package"></a>

#### add`_`dependencies`_`for`_`package

```python
def add_dependencies_for_package(package_id: PackageId, allow_update: bool = False) -> None
```

Add dependencies for the package specified.

<a id="aea.package_manager.base.BasePackageManager.get_package_version_with_hash"></a>

#### get`_`package`_`version`_`with`_`hash

```python
def get_package_version_with_hash(package_id: PackageId) -> PackageId
```

Add package_id with hash for the package presents in registry.

<a id="aea.package_manager.base.BasePackageManager.is_package_files_exist"></a>

#### is`_`package`_`files`_`exist

```python
def is_package_files_exist(package_id: PackageId) -> bool
```

Add packages.
Check package exists in the filesystem by checking it's config file exists.

<a id="aea.package_manager.base.BasePackageManager.package_path_from_package_id"></a>

Expand Down
10 changes: 10 additions & 0 deletions docs/api/package_manager/v0.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ def update_package_hashes() -> "BasePackageManager"

Update packages.json file.

<a id="aea.package_manager.v0.PackageManagerV0.add_package"></a>

#### add`_`package

```python
def add_package(package_id: PackageId, with_dependencies: bool = False, allow_update: bool = False) -> "BasePackageManager"
```

Add package.

<a id="aea.package_manager.v0.PackageManagerV0.verify"></a>

#### verify
Expand Down
10 changes: 10 additions & 0 deletions docs/api/package_manager/v1.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ def is_dev_package(package_id: PackageId) -> bool

Check if a package is third party package.

<a id="aea.package_manager.v1.PackageManagerV1.add_package"></a>

#### add`_`package

```python
def add_package(package_id: PackageId, with_dependencies: bool = False, allow_update: bool = False) -> "PackageManagerV1"
```

Add package.

<a id="aea.package_manager.v1.PackageManagerV1.sync"></a>

#### sync
Expand Down
Loading