Add an interface to turn objects read-only #1162

pmrv · 2023-07-10T10:15:23Z

As we discussed in #1157 it be nice to have some way of blocking users from changing values or calling methods after a certain point in time. This is implemented here with some doc test examples. For now it locks this recursively, but I have a vague fear that this might be not be super performant. I'll have to do some tests. I'll also move over at least DataContainer and Server in this PR.

@liamhuber Do you remember why HasStoredTraits implements its own read_only logic instead of reusing the existing one from DataContainer?

Sub classes that use sentinel on __setattr__ need to special case when the attribute to set is `read_only` or `_read_only`. Failure to do so would result in being unable to unlock the object. `sentinel` now handles this. Additionally adds a `lock_method`. Default is `error`, but can be set to emit a warning and allow modification instead.

This requires a few supers where subclasses were previously not diligent about it

stale · 2023-08-12T12:00:33Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

pmrv · 2023-10-04T13:16:57Z

Current failure seems to be related to #1200

pmrv · 2023-10-05T13:51:35Z

Functionality and documentation is done from my side. There's some tests as this is now used by DataContainer and Server, but I can add additional specific tests after we've discussed whether we want this in a pyiron meeting.

liamhuber · 2023-10-06T23:58:47Z

@pmrv, this hasn't escaped my notice, I just didn't manage to get this far today!

liamhuber

Really nice, Marvin. For specific application here, I only have the one nit to refactor:rename unlocked() to unlock(). Otherwise lgtm, tests can be written, and I think this is useful in base.

Zooming out a bit though, I think this is an elegant solution to the very generic problem of transforming python attributes to being read-only. I think this would be an excellent candidate for the standard-library-only toolbox discussed in #1208. Here it would just be a matter of purging reference to HasGroups, which could then be re-introduced here in pyiron_base by defining _iterate_lockable_subs and a new class inheriting from the generic tool that overrides _on_lock and _on_unlock. Unlike Singleton, which is a breeze to re-implement as needed, what you have here is a bit more complex and might really be useful to the community as a standalone tool (or at least stand-with-other-lightweight-tools-in-a-nice-toolbox)

liamhuber · 2023-10-10T23:15:37Z

pyiron_base/interfaces/lockable.py

+            object.__setattr__(self, "_lock_method", method)
+        self.read_only = True
+
+    def unlocked(self) -> _UnlockContext:


nit: I think this should be unlock to match the verb tense of lock

I named it asymmetrical on purpose, because lock and unlocked are not symmetric either.

What I wanted to avoid is that some code unlocks an object and then crashes (or forgets it) before it can relock it. So obj.unlocked() is a context manager that does the relocking.

Clever. I approve. But I was too dense to pick up on it on the first read, so maybe a comment or docstring to clarify intent is good.

liamhuber · 2023-10-10T23:16:48Z

pyiron_base/interfaces/lockable.py

+        for it in _iterate_lockable_subs(self):
+            it.lock()


Suggested change

for it in _iterate_lockable_subs(self):

it.lock()

pass

maybe (see main review comment)

liamhuber · 2023-10-10T23:17:16Z

pyiron_base/interfaces/lockable.py

+        for it in _iterate_lockable_subs(self):
+            it.read_only = False


Suggested change

for it in _iterate_lockable_subs(self):

it.read_only = False

pass

maybe (see main review comment)

liamhuber · 2023-10-10T23:17:57Z

pyiron_base/interfaces/lockable.py

+def _iterate_lockable_subs(lockable_groups):
+    """
+    Yield sub nodes and groups that are lockable.  Recurse into groups.
+
+    If the given object is not a :class:`HasGroups` yield nothing.
+    """
+    if not isinstance(lockable_groups, HasGroups):
+        return
+
+    subs = lockable_groups.list_all()
+    for n in subs["nodes"]:
+        node = lockable_groups[n]
+        if isinstance(node, Lockable):
+            yield node
+
+    for g in subs["groups"]:
+        group = lockable_groups[g]
+        yield group
+        yield from _iterate_lockable_subs(g)


pyiron_base-specific

pyiron_base/interfaces/lockable.py

pmrv · 2023-12-18T10:02:55Z

Integration tests are currently not passing, because base wants pint==0.23, but atomistics still requires <=0.22. I assume this will solve itself when atomistics is updated, but will go ahead merge this here anyway.

jan-janssen · 2023-12-19T15:38:00Z

Integration tests are currently not passing, because base wants pint==0.23, but atomistics still requires <=0.22. I assume this will solve itself when atomistics is updated, but will go ahead merge this here anyway.

It seems like there was more wrong than just the pint dependency as now the tests fail on pyiron_atomistics - pyiron/pyiron_atomistics#1265 - another indication that we should not merge broken code with main.

pmrv added the enhancement New feature or request label Jul 10, 2023

pmrv force-pushed the lock branch from 635fc18 to fa219ac Compare July 10, 2023 10:18

pmrv mentioned this pull request Jul 10, 2023

Forbid changing the job.server.run_mode after job.run #1157

Open

pmrv added 4 commits July 10, 2023 12:59

Add an interface to turn objects read-only

8c5cd1b

More docstrings

1b3849e

Remove ABC

4dbc385

Use lockable in Server

63e5bf5

pmrv force-pushed the lock branch from a487b8e to 63e5bf5 Compare July 10, 2023 11:15

pmrv added 4 commits July 11, 2023 00:41

Fix/Add doctests; fix Server init

cc0965d

Use Lockable in DataContainer

e26c89a

Lock server together with input

e0a9572

This requires a few supers where subclasses were previously not diligent about it

stale bot added the stale label Aug 12, 2023

stale bot closed this Sep 17, 2023

pmrv reopened this Oct 4, 2023

stale bot removed the stale label Oct 4, 2023

Merge branch 'main' into lock

e413082

Merge branch 'main' into lock

be44cd5

pmrv added the format_black reformat the code using the black standard label Oct 5, 2023

pyiron-runner and others added 3 commits October 5, 2023 12:35

Format black

eb637ad

Add LockedWarning and update docstring

1aa8f6a

Allow to pass lock method to data container

f9a0fd2

pmrv marked this pull request as ready for review October 5, 2023 13:50

pmrv requested review from liamhuber and jan-janssen October 5, 2023 13:50

Allow None as lock method

7d0522b

pmrv added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Oct 9, 2023

Format black

c46f1bd

liamhuber reviewed Oct 10, 2023

View reviewed changes

pmrv commented Dec 15, 2023

View reviewed changes

pyiron_base/interfaces/lockable.py Outdated Show resolved Hide resolved

pmrv commented Dec 15, 2023

View reviewed changes

pyiron_base/interfaces/lockable.py Outdated Show resolved Hide resolved

Docstring fixes

28a6ea3

pmrv added format_black reformat the code using the black standard integration Start the integration tests with pyiron_atomistics/contrib for this PR and removed format_black reformat the code using the black standard labels Dec 15, 2023

Format black

b4b4884

pmrv merged commit c46434e into main Dec 18, 2023
20 of 25 checks passed

delete-merged-branch bot deleted the lock branch December 18, 2023 10:03

jan-janssen mentioned this pull request Dec 19, 2023

Upgrade pyiron_base to 0.6.14 pyiron/pyiron_atomistics#1265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an interface to turn objects read-only #1162

Add an interface to turn objects read-only #1162

pmrv commented Jul 10, 2023

stale bot commented Aug 12, 2023

pmrv commented Oct 4, 2023

pmrv commented Oct 5, 2023

liamhuber commented Oct 6, 2023

liamhuber left a comment

liamhuber Oct 10, 2023

pmrv Oct 26, 2023

pmrv Oct 26, 2023

liamhuber Oct 26, 2023

liamhuber Oct 10, 2023

liamhuber Oct 10, 2023

liamhuber Oct 10, 2023

pmrv commented Dec 18, 2023

jan-janssen commented Dec 19, 2023

	for it in _iterate_lockable_subs(self):
	it.read_only = False
	pass

Add an interface to turn objects read-only #1162

Add an interface to turn objects read-only #1162

Conversation

pmrv commented Jul 10, 2023

stale bot commented Aug 12, 2023

pmrv commented Oct 4, 2023

pmrv commented Oct 5, 2023

liamhuber commented Oct 6, 2023

liamhuber left a comment

Choose a reason for hiding this comment

liamhuber Oct 10, 2023

Choose a reason for hiding this comment

pmrv Oct 26, 2023

Choose a reason for hiding this comment

pmrv Oct 26, 2023

Choose a reason for hiding this comment

liamhuber Oct 26, 2023

Choose a reason for hiding this comment

liamhuber Oct 10, 2023

Choose a reason for hiding this comment

liamhuber Oct 10, 2023

Choose a reason for hiding this comment

liamhuber Oct 10, 2023

Choose a reason for hiding this comment

pmrv commented Dec 18, 2023

jan-janssen commented Dec 19, 2023