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

Caching: Rename get_hash to compute_hash #6347

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 15, 2024

The get_hash method was actually recomputing the hash and not just returning the hash stored in the extras that was computed when the node got stored. The get_hash and _get_hash methods are renamed to compute_hash and _compute_hash, respectively. This allows get_hash to be reimplemented to simply return the hash stored in the extras.

Although technically speaking this is changing the implementation of public facing API, if users even use get_hash() it will have been to retrieve the hash for an already stored node, which is therefore equal to just returning the pre-computed hash stored in the extras. So effectively this should not be a breaking change.

The `get_hash` method was actually recomputing the hash and not just
returning the hash stored in the extras that was computed when the node
got stored. The `get_hash` and `_get_hash` methods are renamed to
`compute_hash` and `_compute_hash`, respectively. This allows `get_hash`
to be reimplemented to simply return the hash stored in the extras.

Although technically speaking this is changing the implementation of
public facing API, if users even use `get_hash()` it will have been to
retrieve the hash for an already stored node, which is therefore equal
to just returning the pre-computed hash stored in the extras. So
effectively this should not be a breaking change.
Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

LGTM. I am wondering, how does this interact with nodes that are stored but not sealed? (such as running Calcjob)

@sphuber
Copy link
Contributor Author

sphuber commented Apr 15, 2024

LGTM. I am wondering, how does this interact with nodes that are stored but not sealed? (such as running Calcjob)

Attributes that can be changed after a node is stored have to be part of _updatable_attributes in order to not incur the ModificationNotAllowed exception and these are automatically excluded from the hash calculation. So these should have no impact whatsoever.

@sphuber sphuber marked this pull request as ready for review April 15, 2024 20:50
@sphuber sphuber merged commit b544f7c into aiidateam:main Apr 15, 2024
19 checks passed
@sphuber sphuber deleted the fix/rename-get-hash-to-compute-hash branch April 15, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants