tvalentyn commented on code in PR #28564:
URL: https://github.com/apache/beam/pull/28564#discussion_r1434204041
##########
sdks/python/apache_beam/transforms/environments.py:
##########
@@ -125,9 +125,17 @@ def __init__(self,
dict(resource_hints) if resource_hints else {})
def __eq__(self, other):
+ equal_artifacts = True
+ for first, second in zip(self._artifacts, other._artifacts):
+ # do not compare type payload since it contains a unique hash.
Review Comment:
This comparison is brittle because:
1) we rely on a particular ordering of self._artifacts. It sounds like the
reason you are changing it is that you have two essentially equal environments
that include an artifact with identical payload/sha256 but different source
file location. The source file location is affected by temp folder that
introduces randomness. This randomness will be reflected in
`artifact.SerializeToString()`, which is used as a key to `sort
self._artifacts` (line 121). Because of that, there is no guarantee that when
you iterate over `self._artifacts` and `other._artifacts`, the lists will be in
a consistent order.
2) ignoring entire type.payload might backfire. I would rather use a
different key for staged files, than `x.SerializeToString()`. For local files,
it could be evaluated based on sha256 and destination name but ignore the
source location. Then, you can use such key when sorting artifacts, and when
comparing them. Having such key would mean that we'd have to decode
ArtifactInformation, and potentially compare different artifact types/roles
differently. We could still fall back to `x.SerializeToString()`.
Alternatively you could adjust requirement generation to use the same temp
directory for generating the pip freeze over the course of the python process
or the test. I think we could do something to prevent the issue you are dealing
with here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
