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

Hashable Configurations #789

Merged
merged 18 commits into from
May 17, 2023
Merged

Hashable Configurations #789

merged 18 commits into from
May 17, 2023

Conversation

danjujan
Copy link
Contributor

@danjujan danjujan commented May 2, 2023

Our configurations are currently not hashable because they contain mutable objects, e.g dict or list. Therefore, they cannot be used as a key in a dict or as element in a set which limits their usability.
For example, in my other PR #718 I need a mapping from Configuration -> Reports. Currently my workaround was to rewrite my own immutable configuration class to make that work: https://github.com/se-sic/VaRA-Tool-Suite/pull/718/files#diff-46ee8773a540aedfd0d1cfbb2d2721a6191d64d3868c538d282c34f85a048492R50. But that basically duplicates functionality and it would be nice to reuse our existing configuration classes for that.
However, a lot of our code relies on the mutability of configurations. Therefore, my proposal is to keep configurations mutable by default but make them immutable (freeze them) on demand. The immutables.map datatype provides these properties.

The committed code is a rough and far from perfect sketch how such a solution could look like. Apart from loosing the insertion order of configuration options it passes all tests. But I would like to collect some opinions first before pursuing this.

@danjujan danjujan requested review from vulder and boehmseb May 2, 2023 09:06
@boehmseb
Copy link
Member

boehmseb commented May 3, 2023

If hashing is the only issue, it should be easier to simply add a custom __hash__() function to the ConfigurationImpl class.
Depending on our requirements, hash(frozenset(__config_values.items())) should suffice as a hashing function.
This should be stable as long as you stay in the same python process. Also see this SO for more discussion regarding this topic: https://stackoverflow.com/questions/5884066/hashing-a-dictionary

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 91.08% and project coverage change: +0.11 🎉

Comparison is base (93e3f71) 67.43% compared to head (ae07531) 67.54%.

Additional details and impacted files
@@             Coverage Diff              @@
##           vara-dev     #789      +/-   ##
============================================
+ Coverage     67.43%   67.54%   +0.11%     
============================================
  Files           322      322              
  Lines         24459    24590     +131     
============================================
+ Hits          16493    16610     +117     
- Misses         7966     7980      +14     
Impacted Files Coverage Δ
varats-core/varats/base/configuration.py 89.59% <82.92%> (-6.87%) ⬇️
tests/base/test_configuration.py 100.00% <100.00%> (ø)
tests/mapping/test_configuration_map.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@danjujan
Copy link
Contributor Author

danjujan commented May 3, 2023

After discussing with @vulder I added a freeze and unfreeze function and deepcopy between them to guarantee immutability.
The other changes are usability improvements.

@danjujan danjujan marked this pull request as ready for review May 3, 2023 19:02
Copy link
Contributor

@vulder vulder left a comment

Choose a reason for hiding this comment

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

freezing impl looks fine to me, just suggested a bit of clean up

However, should we maybe add tests for the new FrozenConfiguration?

varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
varats-core/varats/base/configuration.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vulder vulder left a comment

Choose a reason for hiding this comment

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

freezing impl looks fine to me, just suggested a bit of clean up

However, should we maybe add tests for the new FrozenConfiguration?

Copy link
Contributor

@vulder vulder left a comment

Choose a reason for hiding this comment

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

freezing impl looks fine to me, just suggested a bit of clean up

However, should we maybe add tests for the new FrozenConfiguration?

@vulder
Copy link
Contributor

vulder commented May 10, 2023

Looks like github has some issues currently... Somehow it registered by review twice

@danjujan danjujan requested a review from vulder May 17, 2023 08:01
@vulder vulder merged commit 1eb244e into vara-dev May 17, 2023
@vulder vulder deleted the hashable-configuration branch May 17, 2023 10:47
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.

3 participants