Readonly settings #169

richardjgowers · 2023-04-06T11:22:12Z

Adds Settings.frozen_copy and Settings.unfrozen_copy to create immutable/mutable versions of Settings object. These freezes are recursive and apply to all contained Settings too.

Protocols now freeze their Settings on creation. This should stop mistakes around modifying a Protocol's Settings in place.

richardjgowers · 2023-04-06T11:51:51Z

This seems to be a terrible idea... I'm getting weird errors where other Settings objects now think they're readonly when they're not.

codecov · 2023-04-06T12:49:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c12bba6) 99.21% compared to head (8f1476d) 99.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   99.21%   99.22%   +0.01%     
==========================================
  Files          36       36              
  Lines        1911     1938      +27     
==========================================
+ Hits         1896     1923      +27     
  Misses         15       15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dwhswenson

I think we should use the pydantic functionality to make settings immutable. That way we raise an error if a user mutates something.

I think that

>>> protocol.settings.mysetting
'foo'
>>> protocol.settings.mysetting = "bar"
>>> protocol.settings.mysetting
'foo'

is much riskier for users than

>>> protocol.settings.mysetting
'foo'
>>> protocol.settings.mysetting = "bar"
TypeError: 'Settings' is immutable and does not support type assignment

richardjgowers · 2023-04-10T09:01:17Z

@dwhswenson I tried that originally.

I had this in place:

f07790f

But you ended up with very strange behaviour, where "brand new" Settings objects were readonly, see here:

https://github.com/OpenFreeEnergy/gufe/actions/runs/4628556994/jobs/8187777193#step:7:886

I never quite figured it out, but it felt like the readonly was applying to all instances of the class, rather than a given instance of the class

richardjgowers · 2023-04-11T10:23:20Z

@dwhswenson thanks for the improvement. I'm waiting on merging this as it causes a few headaches downstream in openfe. In TestResultClient I get a lot of: E TypeError: Object of type DummySpecificSettings is not JSON serializable from within self._store_gufe_tokenizable and I've not had time to dig through and see what's happening there.

richardjgowers · 2023-04-14T17:25:08Z

todo: add unfreeze

pep8speaks · 2023-12-18T15:50:18Z

Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file gufe/protocols/protocol.py:

Line 92:80: E501 line too long (106 > 79 characters)

Comment last updated at 2024-01-25 10:42:08 UTC

dwhswenson

Mostly LGTM -- requesting small additional tests that aren't testing nested behavior, mainly so that no future refactors accidentally break things.

dwhswenson · 2024-01-18T17:37:03Z

gufe/tests/test_alchemicalnetwork.py

@@ -13,8 +13,8 @@
 class TestAlchemicalNetwork(GufeTokenizableTestsMixin):

    cls = AlchemicalNetwork
-    key = "AlchemicalNetwork-8c6df17d7ecf5902e2e338984cc11140"
-    repr = "<AlchemicalNetwork-8c6df17d7ecf5902e2e338984cc11140>"
+    key = "AlchemicalNetwork-d1035e11493ca60ff7bac5171eddfee3"


Just checking: is this changing the gufe key because you've changed the fixture to use non-None settings? (Otherwise, nothing in this PR should affect gufe keys, right?)

yep it's this

dwhswenson · 2024-01-18T17:37:52Z

gufe/tests/test_models.py

@@ -53,3 +53,45 @@ def test_invalid_constraint(value, good):
    else:
        with pytest.raises(ValueError):
            _ = OpenMMSystemGeneratorFFSettings(constraints=value)
+
+
+class TestFreezing:


Current tests check that it works we do settings.subsettings.item = .... Could you add a test that does settings.subsettings = ... ? (example: you attempt to replace the entire thermo_settings in one go).

richardjgowers · 2024-01-19T10:55:04Z

gufe/protocols/protocol.py

-        """The full settings for this ``Protocol`` instance."""
-        return self._settings
+        """A copy of the full settings for this ``Protocol`` instance.  This is read only"""
+        return self._settings.frozen_copy()


this is probably overzealous and causes a copy on each access right?

dwhswenson

With the extra test added, LGTM!

type[A] is the correct way to indicate A or a subclass thereof

settings are immutable once inside a Protocol

must use Settings now

setting __config__.allow_mutation was giving bizarre behaviour temporarily retreat from that madness and instead `Protocol.settings` returns a deepcopy it ain't perfect but it'll stop some things

tests for freezing behaviour

avoids copy on each access, probably saves some time in a `Protocol.settings` heavy access pattern

richardjgowers requested review from dwhswenson and IAlibay April 6, 2023 11:23

richardjgowers force-pushed the readonly_settings branch from f07790f to 3c74d7c Compare April 6, 2023 11:23

IAlibay requested a review from mikemhenry April 6, 2023 11:52

dwhswenson requested changes Apr 7, 2023

View reviewed changes

dwhswenson mentioned this pull request Apr 10, 2023

Improved readonly settings #177

Merged

richardjgowers force-pushed the readonly_settings branch from 6f81b96 to 2b610dd Compare April 11, 2023 10:21

richardjgowers changed the title ~~Readonly settings~~ [WIP] Readonly settings Apr 11, 2023

dwhswenson mentioned this pull request Apr 11, 2023

Use JSON_HANDLER classes in ResultClient storage OpenFreeEnergy/openfe#334

Merged

richardjgowers added this to the Release 1.0.0 milestone Apr 11, 2023

richardjgowers force-pushed the readonly_settings branch from 2b610dd to 38e3e90 Compare December 18, 2023 15:50

richardjgowers changed the title ~~[WIP] Readonly settings~~ Readonly settings Dec 18, 2023

richardjgowers requested a review from dwhswenson December 18, 2023 15:56

dwhswenson requested changes Jan 18, 2024

View reviewed changes

richardjgowers commented Jan 19, 2024

View reviewed changes

richardjgowers requested a review from dwhswenson January 19, 2024 16:52

dwhswenson approved these changes Jan 22, 2024

View reviewed changes

richardjgowers added 7 commits January 25, 2024 10:41

tweaked typing info for settings

fa35f09

type[A] is the correct way to indicate A or a subclass thereof

fixes #138

baf4829

settings are immutable once inside a Protocol

fixup mypy woes

ee6653d

make benzene_variants fixture use Settings

0231dc7

more test fixups, but this isn't right currently

db02942

fixes #138

999a81d

settings are immutable once inside a Protocol

another test fix up

6e48494

must use Settings now

richardjgowers and others added 11 commits January 25, 2024 10:41

new plan on readonly settings

b70dec9

setting __config__.allow_mutation was giving bizarre behaviour temporarily retreat from that madness and instead `Protocol.settings` returns a deepcopy it ain't perfect but it'll stop some things

revert trying to be clever about type annotations

9cde60a

Improved readonly settings

2a25f28

don't need special-casing for _is_frozen

51b463f

Update models.py

958db64

fix type annotations on settings

c7d8e6d

update AlchemicalNetwork hashes

b75ea44

adds Settings.unfrozen_copy

28e49d2

tests for freezing behaviour

copy of test fixups

da8df08

make Protocol.settings return read-only view

a70d92d

avoids copy on each access, probably saves some time in a `Protocol.settings` heavy access pattern

added test for setting subsection of frozen settings fails as expected

8f1476d

richardjgowers force-pushed the readonly_settings branch from d771889 to 8f1476d Compare January 25, 2024 10:41

richardjgowers enabled auto-merge (squash) January 25, 2024 10:42

richardjgowers merged commit 51e6812 into main Jan 25, 2024
10 checks passed

richardjgowers deleted the readonly_settings branch January 25, 2024 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readonly settings #169

Readonly settings #169

richardjgowers commented Apr 6, 2023 •

edited

Loading

richardjgowers commented Apr 6, 2023

codecov bot commented Apr 6, 2023 •

edited

Loading

dwhswenson left a comment

richardjgowers commented Apr 10, 2023

richardjgowers commented Apr 11, 2023

richardjgowers commented Apr 14, 2023

pep8speaks commented Dec 18, 2023 •

edited

Loading

dwhswenson left a comment

dwhswenson Jan 18, 2024

richardjgowers Jan 19, 2024

dwhswenson Jan 18, 2024

richardjgowers Jan 19, 2024

dwhswenson left a comment

Readonly settings #169

Readonly settings #169

Conversation

richardjgowers commented Apr 6, 2023 • edited Loading

richardjgowers commented Apr 6, 2023

codecov bot commented Apr 6, 2023 • edited Loading

Codecov Report

dwhswenson left a comment

Choose a reason for hiding this comment

richardjgowers commented Apr 10, 2023

richardjgowers commented Apr 11, 2023

richardjgowers commented Apr 14, 2023

pep8speaks commented Dec 18, 2023 • edited Loading

Comment last updated at 2024-01-25 10:42:08 UTC

dwhswenson left a comment

Choose a reason for hiding this comment

dwhswenson Jan 18, 2024

Choose a reason for hiding this comment

richardjgowers Jan 19, 2024

Choose a reason for hiding this comment

dwhswenson Jan 18, 2024

Choose a reason for hiding this comment

richardjgowers Jan 19, 2024

Choose a reason for hiding this comment

dwhswenson left a comment

Choose a reason for hiding this comment

richardjgowers commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading

pep8speaks commented Dec 18, 2023 •

edited

Loading