Thing Setting syntax and implementation overhaul

[julianstirling]

This PR changes how settings are handled so that they are now identical to properties, except that they are saved to disk when the setter is called.

Problems this PR solves

• ThingSettings are not saving reliably
• ThingSettings are not obviously and explicitly defined
• ThingSettings require quite a bit of extra code to interact with them as a dictionary
• ThingSettings sometimes return a ReactiveDict to the API causes an error with Pydantic

Closes #107
By removing Reactive dict

Closes #108
Settings now save on update, a unit test is added to check this.

Closes #109
By implementing the agreed upon syntax

Closes #111
Settings now reliably save when set. If an internal value is set in a dictionary setting they are not saved. However, this behaviour is documented in the docstring for ThingSetting and the associated decorator. There is also a unit test for this behaviour which states that the documentation should be updated if this is changed somehow in the future.

Closes #112
Default settings are now available during initialisation but they are not loaded from disk until added to the server.

Other things in this MR

• A regex was added (and associated test) for the correct path format when adding a thing to the server. This fixes a TODO about a potential security issue. - This was done because I had some issues with entering paths during unit test writing
• A new NotConnectedToServerError exception was added, and the message to the user was updated for when properties/events are used without a server. Previously the message just said something about a blocking portal which is not clear to someone who doesn't know the underlying LabThings-FastAPI codebase. The new message explicitly says that it cannot emit the event and asks if the Thing is connected to the server.

High level implementation details

Iteration 1

Click to show details of iteration 1 that is now superseeded

Summary

This MR would make properties have an option to be persistent, saving to disk when the setter is called.

A new decorator

A new decorator @persistent_thing_property is created, this acts exactly as @thing_property except that it saves to disk. This could be @thing_property(persistent=True) except that making arguments for decorators is a bit messy, and making optional arguments is even messier and quite non-standard unless I have missed something.

Add attributes to PropertyDescriptor

Two attributes are added:

• persistent a boolean that sets whether the Property should save to disk
• save_location This is the save path (Optional!). It starts as None. If it is None saving would not happen, if it is set, saving would. Currently this just logs that it should save:

Changes to add_thing on the server

Check through each attribute using class_attributes utility function that already exists. If a property is found, then it is checked whether it is persistent. If it is the save location is added to the descriptors save_location.

Not added yet!
Read from disk and set the property from values on disk

Issues to consider

• Directly updating the descriptor at runtime seems like bad thing to do to pass in the save_location. It is unclear to me whether there were two objects of the same class whether it would break. Ideas welcome.
• Concurrency. Probably rather than just save directly in the descriptor, the server should pass some sort of class that controls the property files that can ensure that they are run sequentially.
• Whether the setter should warn/error if persistent and the save location is None. This would mean that we would need a 3rd state to say that the Thing was in the setup mode as the server will need to set the properties loaded from disk without raising the warnings/errors

Iteration 2

Summary

This MR would make a decorator and a descriptor for settings. Settings would be a type of property that is persistent. i.e. if you make something a setting it is also a property

A new decorator

A new decorator @thing_setting is created, this acts exactly as @thing_property except that it saves to disk.

SettingDescriptor

A subclass of ProperyDescriptor on __set__ it called the save settings method that is added to the main Thing class

Changes to Thing

• Thing now has a property (python one not a Thing Property) with the save path, that is passed to it on by the server when it is added to the server.
• Thing has a property (again not a Thing Property) called settings, which is a list of the names of its Thing Settings. From this a simple comprehension can get create a settings dictionary: {name: getattr(self, name) for name in self.settings}
• A save_settings method that is called whenever a Thing Setting is updated. It will save to file, but currently it logs. It now has the full settings dictionary, using the above comprehension without requiring the dictionary to be explicitly created and used by anyone creating a Thing

Not added yet!
A way to read settings from disk when started up

Issues to consider

• Whether the setter should warn/error if persistent and the save location is None. This would mean that we would need a 3rd state to say that the Thing was in the setup mode as the server will need to set the properties loaded from disk without raising the warnings/errors. This is probably easy enough to do now the setting is in the Thing compared to iteration 1.

[rwb27]

I like the idea of having a descriptor to represent settings, and a decorator might well be an intuitive way to do this. Currently, Properties can work in a few different ways, but I think persistent properties (or settings as I've referred to them) ought to work as "dumb" properties - i.e. there's no getter method, they just return the value.

I made #109 to discuss the syntax for this. I have said "setting" instead of "persistent property", but everything's up for discussion.

I note that this implementation has the same problem as the current ThingSettings mechanism, in that it won't work properly until the Thing is attached to a ThingServer. We might want to think through that one too in #112.

[rwb27]

I would be tempted to keep self.thing_settings but change it to something closer to a regular dictionary, then have the setting descriptors trigger saving when they are written to - that would, I think, be in line with your suggestion of having an object to look after loading/saving that can avoid concurrency issues?

[julianstirling]

The problem with the dictionary is that it requires that the person writing the code that is the property to read from the dictionary, write to the dictionary, and call saving. And to use the string of the property name in the dictionary otherwise the settings don't match. It feels less like a framework feature and something that is implemented each time

I can see that saving it as a private variable isn't exactly perfect either. I can see it may be good to use dictionary under the hood, but I can't see how to do this with decorators.

[julianstirling]

Right. I have updated the MR and the description. I have moved the implementation details I had before into a collapsed Iteration 1 section and then re-written a new set of details for iteration 2.

I think this is somewhat a nice halfway house between the ideas:

• I have moved back to "setting", and away from "persistent property"
• The Thing still can save a whole dictionary of all of its settings which is good for concurrency, however the dictionary isn't explicitly created by the person writing a Thing, it is created as a subset of built in __dict__ based on a list of settings when saving.
• The Thing itself is given the path for saving the settings when added to the server
• This should also solve Be clear that thing_settings are not available during __init__ #112, it will just be that if it isn't added to the server it will use default values.

[barecheck]

Barecheck - Code coverage report

Total: 85.75%

Your code coverage diff: 1.73% ▴

Uncovered files and lines

File	Lines
src/labthings_fastapi/client/in_server.py	69-71, 176
src/labthings_fastapi/descriptors/action.py	33, 235-236, 239, 263
src/labthings_fastapi/descriptors/property.py	21, 46, 48, 69, 71, 102-103, 226-227
src/labthings_fastapi/example_things/__init__.py	125
src/labthings_fastapi/server/__init__.py	76, 99, 124, 151, 159, 181-182
src/labthings_fastapi/thing.py	34-35, 194, 247, 288, 295

[julianstirling]

Tests and docs added. I think this is ready now.

[rwb27]

This looks good to me. I have a nagging suspicion it might break some of our picamera2 code, because I remember some slightly funny things happening with those settings. However, I am sure we will be able to work around it.

Also, we previously talked about reorganising the namespace a bit, so that Thing, ThingSetting, etc. could be imported directly from the toplevel labthings_fastapi import. I'd be broadly in favour of changing the API as few times as possible, so we might want to do that before this gets released.

[julianstirling]

Also, we previously talked about reorganising the namespace a bit, so that Thing, ThingSetting, etc. could be imported directly from the toplevel labthings_fastapi import. I'd be broadly in favour of changing the API as few times as possible, so we might want to do that before this gets released.

Yeah, I agree. I think we reorganise the namespace to get most of what we expect to import into the top level namespace before we release. We may want to call this v0.1.0? In the the hope that it is a largely stable namespace?