-
Notifications
You must be signed in to change notification settings - Fork 9
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
Hashing made with element and md5 checksum #168
Conversation
Somehow the objcets are the same but the hash values are different? I think I miss some key behaviour of |
It is not a misunderstanding of the from aiida.orm.nodes.caching import NodeCaching
class PseudoPotentialDataCaching(NodeCaching):
"""Class to define caching behavior of ``PseudoPotentialData`` nodes."""
def _get_objects_to_hash(self) -> list:
"""Return a list of objects which should be included in the node hash."""
return [self.element, self.md5]
class PseudoPotentialData(Data):
_CLS_NODE_CACHING = PseudoPotentialDataCaching That should make the tests pass. You have to change the |
@sphuber It works! thanks! |
bb47f4e
to
f4de94d
Compare
f4de94d
to
a7f5335
Compare
Because I saw the setter function for the element and then I was thinking that there might be different site labels for the same element using the same UPF file. |
For the subclasses it is yes, but for the |
I don't have any use case involved with subfix for element, so I am leaning to remove element from caching. |
a7f5335
to
48635ee
Compare
By default, the hash for data nodes is computed from a set of attributes that includes the version of `aiida-core` as well as the plugin package that provides the data plugin, if applicable. This means that the hash of a data node changes merely if the version of `aiida-core` or the plugin package changes, even if its content is identical. This choice was made in `aiida-core` to err on the safe side. However, for pseudopotentials, this is unnecessary and actually reduces the effectiveness of caching. If a calculation is run with pseudos that has already run before, but the version of `aiida-pseudo` is different with which the pseudo was installed, the calculation will not be cached but run again. The `PseudoPotentialData` now customizes the caching class, ensuring the hash is calculated just based on the element and the file content. This should uniquely define a pseudopotential and so should be safe enough to base the hash upon.
48635ee
to
4ba2454
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @unkcpz . Let's get this merged. The concept just makes sense. We will find out later if there really is some use-case out there that is not compatible with this change.
fixes #167
For pseudo data, all we care about for the node is the hash of the file. So we could even think of overriding the _get_objects_to_hash and just return the md5 and element.