Simplify property and add type hints

[rwb27]

This PR significantly reorganises how properties work. The functionality is split into separate classes with a common base, and there are fewer, better-defined ways for a property to work.

• DataProperty is a variable-like property. It may not have a getter/setter, and functions just like a variable. You may specify whether it can be written to from client code, that's currently its only option.
• FunctionalProperty uses getter/setter. It works pretty much exactly like Python's property.
• Both these classes are used via a common function, lt.property. See example below.
• Documentation-related stuff (remembering the name, generating a title/description) is moved to a separate base class. In due course, this could also be used for actions.
• lt.setting functions just like lt.property, but the values are remembered in the settings file for the Thing. The implementation of saving/loading settings has not changed.

The two ways for defining properties now look like:

import labthings_fastapi as lt

class Counter(lt.Thing):
    "A counter that knows what's remaining."

    count: int = lt.property(default=0, readonly=True)
    "The number of times we've increnented the counter."

    target: int = lt.property(default=10)
    "The number of times to increment before we stop."

    @lt.property
    def remaining(self) -> int:
        "The number of steps remaining."
        return self.remaining - self.count

    @remaining.setter
    def set_remaining(self, value: int) -> None:
        self.target = self.count + value

I am happy with this syntax. It's not exactly what I wanted, but I think it is the best compromise between elegance and practicality. The two quirks I would like to draw attention to are:

• Both default and default_factory are keyword-only arguments. This enables mypy to distinguish between use as a decorator and use as a field, so that both cases are typed correctly.

  An earlier commit allowed default to be the (first and only) positional argument: it worked fine, but it confused mypy. We care about type checking in code that defines Things, so it's worth adopting the form that mypy can understand.

• When using @lt.property or @lt.setting as a decorator, mypy will raise an error if the same name is used for the getter and setter functions, because doing so means the descriptor is overwritten (with itself). The descriptor works in both cases.

There are quite a few things remaining to do before this is ready to be merged. These include:

• Implement BaseProperty.model to ensure non-pydantic types can be wrapped in a RootModel when needed.
• Allow settings with getters and setters
• Add a way to attach an on_set function to a DataProperty. This would allow validation and synchronisation of DataProperty instances, which is required because settings are now DataProperty instances. This can be postponed, because getters/setters are back in.
• Get all tests passing
• Get mypy passing
• Write tests for typing, ensuring mypy raises the right errors if properties are used incorrectly, and that it can infer the right types.
• Support docstrings-after-attributes. Currently, data properties have no docstring.
• Consider the module names carefully for thing_property and base_descriptor
• Test openflexure-microscope-server with this branch.
• Add tests to explore whether there's anything that might happen as a result of setters being named differently to getters.
• Revisit inheriting from property just to make sure that doesn't solve one of our problems.
• Add a documentation page on settings. moved to Add a tutorial page on settings #171.

[julianstirling]

One thing it would be nice to consider that is causing us a bit of an issue right now in the OpenFlexure Server development is making a setting readonly, or setting limits (so the UI can validate).

Settings need to have a setter, because that setter is used for loading from disk. This should be quite easy if the setting is a descriptor type. But any read-only setting with complex logic that requires a getter and a setter needs a way to communicate that the setting it is read only.

It would be good to think of a way where the code to specify this is very explicit. I think for a ThingProperty it is easy. Don't set a setter and it should be read only, this matches Python's built in behaviour. I was wondering if for some of the values required for dataschema could just be set based on a dataclass/model?

Making a contrived example where setters and getters are needed to "process data", in reality this would probably be hardware interaction:

MIN_LEN = 4
class Announcer(lt.Thing):

    repeat: int = lt.setting(default=1, minimum=1)

    self._message = "I can"

    @lt.setting
    def message (self) -> str:
        "My current message."
        return "I do proclaim that " + " ".join([self._message]*self.repeat)

    @message.setter
    def message(self, value: str) -> None:
        self._message = value.strip()

    @message.meta_info
    def message(self) -> lt.PropertyInfo:
        return PropertyInfo(
            readonly=True,
        )

    self._incoming_message = "I cannot"

    @lt.setting
    def incoming_message (self) -> str:
        "Your current message."
        return "You have proclaimed that " + " ".join([self._message]*self.repeat)

    @incoming_message.setter
    def incoming_message(self, value: str) -> None:
        value = value.strip()
        if len(value) < MIN_LEN
            raise ValueError("Can't set that!")
        self._incoming_message = value

    @incoming_message.meta_info
    def incoming_message(self) -> lt.PropertyInfo:
        return PropertyInfo(
            min_length=MIN_LEN,
        )

This would very explicitly allow the a setting to inform the UI to set a min length. As it is better to have a UI that doesn't allow a bad value than to just throw and error if incorrect.

It also allows the settings describe that it is readonly despite having the setter for loading from disk. The other way I can think of doing it is having a separate decorator for the setter if it is to otherwise be read only:

MIN_LEN = 4
class Announcer(lt.Thing):

    repeat: int = lt.setting(default=1, minimum=1)

    self._message = "I can"

    @lt.setting
    def message (self) -> str:
        "My current message."
        return "I do proclaim that " + " ".join([self._message]*self.repeat)

    @message.set_from_cache
    def message(self, value: str) -> None:
        self._message = value.strip()

    self._incoming_message = "I cannot"

    @lt.setting
    def incoming_message (self) -> str:
        "Your current message."
        return "You have proclaimed that " + " ".join([self._message]*self.repeat)

    @incoming_message.setter
    def incoming_message(self, value: str) -> None:
        value = value.strip()
        if len(value) < MIN_LEN
            raise ValueError("Can't set that!")
        self._incoming_message = value

    @incoming_message.meta_info
    def incoming_message(self) -> lt.PropertyInfo:
        return PropertyInfo(
            min_length=MIN_LEN,
        )

---

Just a note (I know this is early days), but the remaining example in the description is quite confusing. It seems to be calling itself recursively on read but setting a different property on save?

[rwb27]

One thing it would be nice to consider that is causing us a bit of an issue right now in the OpenFlexure Server development is making a setting readonly, or setting limits (so the UI can validate).

Settings need to have a setter, because that setter is used for loading from disk. This should be quite easy if the setting is a descriptor type. But any read-only setting with complex logic that requires a getter and a setter needs a way to communicate that the setting it is read only.

I've not yet fully worked through settings - that's a todo I should make more explicit. I think, though, that I am very keen to enforce variable-like behaviour for settings - and I think that means settings should not be implemented with getters and setters. I should write my thinking about this down more carefully than I'm likely to do in this reply - but I think the key point is that settings need to return, when they are read, the same value that goes in when they are written to. If that's not the case, the whole thing gets very confusing, and strange behaviour results.

Taking your two examples, which I realise you have said are quite contrived, if we are to save those to disk then reload them, the values get longer each time. The first time we load up an Announcer, the message will be "I do proclaim that I can.", but after shutting down and reloading it will be "I do proclaim that I do proclaim that I can."` and so on.

In various other channels, we've described two scenarios where it's important to run code as a result of interacting with a setting:

1. Validation: it's important to be able to check inputs are valid and prevent the setting from being changed to a value that is not valid.
2. Synchronisation: if a setting controls a bit of hardware, we need to be able to update the hardware when the setting changes.

I think both of these could be implemented with some sort of event handler, that for now I will call on_set. I'd propose to implement it along the lines of:

1. Some code tries to write to a setting
2. on_set is called, with the new value as its argument.
3. If on_set raises an exception, an error code is returned (if it was called over HTTP) or the exception propagates (if called from Python). The stored value will not be changed, because clearly something went wrong.
4. If on_set completes successfully, we update the stored value, and the next time the property is read, this is the value that will be returned.

That should work fine for validation. I prefer it to a setter because it's not responsible for storing the value: I think this makes it clearer that the value is managed externally.

The mechanism described above would also work for well-behaved hardware, by which I mean that the hardware will accept the value that's written to it, and it won't change subsequently. However, this doesn't work nicely for hardware that may accept a value for a setting, but then quietly modify it.

A possible solution would be for on_set to return the value of the setting. That way, if we have a less well behaved piece of hardware like the Raspberry Pi camera, our on_set function might look like:

def CameraWithFunnySetting(lt.Thing):
    exposure_time: float = lt.setting(10)
    
    @on_set("exposure_time")
    def set_exposure_time(self, new_value: float) -> float:
        """Set the exposure time, and check the value actually used."""
        self._hardware.set_exposure(new_value)
        time.sleep(0.1)
        return self._hardware.read_exposure()

Returning the value would allow the code in on_set to modify the value so that the setting contains a value that's correct. However, it may then fall foul of the behaviour I described at the start - a round-trip might not result in no changes.

In the not-totally-uncommon case where there is a set of valid values and we pick the closest, the code above makes sense: the value is modified the first time it's written, but on subsequent writes (e.g. loading/saving when the server is restarted) there isn't any change. We could even check for that - making sure on_set doesn't modify the value when it's loaded from disk.

The aim of simplifying properties was to remove confusing behaviour, and I think properties do look much better after this change. It would be possible to have a FunctionalSetting descriptor that works similarly to FunctionalProperty (i.e. with getter/setter) but I am still not sure this is a good idea.

Ultimately, if custom getter/setter code is required, I think there is a very reasonable fallback, which is to use a FunctionalProperty and a separate setting that behaves as a simple variable. This then makes it very clear that the simple setting is what's loaded/saved, and there is no hard requirement on the property to follow any particular sort of behaviour in order to make sense. For the relatively small number of settings that need custom handling, I think this would be my preferred way of implementing them.

I thought we'd talked through this in a call a week or two ago, but perhaps my memory of that is not the same as yours. Probably what we need is to figure out how it would change code that uses settings, and I'm guessing the critical example is the camera code in the openflexure microscope. Perhaps it's worth writing a few more examples and thinking through them. I might take a stab at revising your two examples, just in case it's helpful to see how I'd approach them.

Just a note (I know this is early days), but the remaining example in the description is quite confusing. It seems to be calling itself recursively on read but setting a different property on save?

Ah, that was a typo. This is what I get for writing example code that's not tested... There should not be any recursion!

[rwb27]

from pydantic import RootModel, Field
import labthings_fastapi as lt

MIN_LEN = 4
class Announcer(lt.Thing):

    repeat: int = lt.setting(default=1)

    self._message: str = lt.setting(default="I can", readonly=true)

    @lt.property
    def message (self) -> str:
        "My current message."
        return "I do proclaim that " + " ".join([self._message]*self.repeat)

    @message.setter
    def message(self, value: str) -> None:
        self._message = value.strip()

    @message.readonly = True

    self._incoming_message = lt.setting(default="I cannot")

    @lt.property
    def incoming_message (self) -> str:
        "Your current message."
        return "You have proclaimed that " + " ".join([self._incoming_message]*self.repeat)

    @incoming_message.setter
    def incoming_message(self, value: str) -> None:
        value = value.strip()
        if len(value) < MIN_LEN
            raise ValueError("Can't set that!")
        self._incoming_message = value

Some things I will take away from that example:

• Simple constraints on values (minimum length, minimum value) would be nice, and should be communicated to the client. It's possible this actually happens already if the constraints are added in a pydantic-compatible way, but it would be nice to be able to specify this cleanly. I think passing **kwargs from lt.setting to pydantic.Field might do this quite well - that would permit repeats: int = lt.setting(default=1, ge=1), for example. Those constraints should show up in the JSON Schema and in the Thing Description, though the latter will want testing.
• The constraints could already be added with annotated types from pydantic. I understand you dislike these intensely, and I agree they are super ugly. The syntax you've suggested, though, is very close to what pydantic.Field does, so hopefully that is a useful way forward.
• Making a property read-only from the client is already possible, by setting the readonly attribute. Again, that will want testing/examples but I think it should work.
• It might be nice to be able to have a setting that isn't visible at all over the network (so _message can't be read or written to via HTTP). That should be fine to add, I think.

The main thing I changed is that it's _message and not message that is the setting. I appreciate there may be examples where such a swap isn't possible, but perhaps it's already a useful illustration of how I am thinking it could work?

[julianstirling]

I've not yet fully worked through settings - that's a todo I should make more explicit. I think, though, that I am very keen to enforce variable-like behaviour for settings - and I think that means settings should not be implemented with getters and setters.

I have found so far most settings I have needed to use getters and setters in the microscope code base. If getters and setters are not allowed then we need to make sure that the descriptors are poweful enogh for all use cases. For example we have a lot of things where the information is held in other classes so we need to be able to iterate through them on get and set. Without getters and setters we would need a complex signalling mechanism that would go wrong.

[rwb27]

I've not yet fully worked through settings - that's a todo I should make more explicit. I think, though, that I am very keen to enforce variable-like behaviour for settings - and I think that means settings should not be implemented with getters and setters.

I have found so far most settings I have needed to use getters and setters in the microscope code base. If getters and setters are not allowed then we need to make sure that the descriptors are poweful enogh for all use cases. For example we have a lot of things where the information is held in other classes so we need to be able to iterate through them on get and set. Without getters and setters we would need a complex signalling mechanism that would go wrong.

I should look through those and figure out if there's a way it would work nicely, or if my assumptions are just wrong. I'm aware of the ones in the camera - I will have a look in the microscope codebase for others, but might miss stuff if it's in branches. Pointing me at examples of where you've used getters/setters for settings would be helpful.

[julianstirling]

I've not yet fully worked through settings - that's a todo I should make more explicit. I think, though, that I am very keen to enforce variable-like behaviour for settings - and I think that means settings should not be implemented with getters and setters.

I have found so far most settings I have needed to use getters and setters in the microscope code base. If getters and setters are not allowed then we need to make sure that the descriptors are poweful enogh for all use cases. For example we have a lot of things where the information is held in other classes so we need to be able to iterate through them on get and set. Without getters and setters we would need a complex signalling mechanism that would go wrong.

I should look through those and figure out if there's a way it would work nicely, or if my assumptions are just wrong. I'm aware of the ones in the camera - I will have a look in the microscope codebase for others, but might miss stuff if it's in branches. Pointing me at examples of where you've used getters/setters for settings would be helpful.

I would just do a search of @lt.thing_setting on the v3 branch. But I would strongly recommend not stopping the use of setters and getters for settings for three reasons:

• It breaks parity between properties and settings. The nice thing about setting is you change a property to a setting and it saves. If we property have ways of being created that settings don't then it makes it much harder to make something persist.
• Settings tend to be more complex. There is often a complex functional data that needs to persist, often that is less important than being read over HTTP. Setting provides a very uniform way of saving state information. If settings have to be dumb then we will find ourselves in situations where we need to create other ways of saving state information.
• It gives us fewer ways to work around the fact that the permutation space of LabThings has not been explored in anger. I have found a number of times that I try to create something and due to how the date is captured and sent it causes a recursion error, or if the data type is vaguely know and so the a clear base model is not know, then it doesn't seriealise and deserialise correctly. Currently each time this happens we can create an issue in LabThings to improve it, but we have the options of switching to a getter and a setter to carry on with our development. If this is lost then we loose will not be able to proceed until there is a LabThings-FastAPI release

[julianstirling]

Here is an example that I see becoming more and more common as we try to make the algorithms that Things use for certain tasks configurable:

BackgroundDetection is now done with one of many BackgroundDetector objects, each object is a different instance of a different class. These each have a defined their own data structure (via a BaseModel) for their settings, and for the information they save about the background. This should be very flexible for adding new algorithms without changing any Thing code.

On init we create every dackground detector, but their settings and data are persistent, so we load the settings from disk and find each instance in turn and pass them their settings and data. This is something we will also need to do for path planners that are needed to allow us to switch how we plan the path for a scan, without creating a new Thing for each type of scan.

We need this working very soon as we need to scan over a pre-defined grid for the urine microscopy.

Just like with the namespace changes and the last changes to ThingSettings, before we merged them into the main branch on LabThings we created an example branch on the server to check how it performs in a more complex situation. As we add more and more unit and integration tests to the microscope this will be our best resource for checking that any large changes can support the behaviour we need.

[rwb27]

I would just do a search of @lt.thing_setting on the v3 branch.

Of course - that is something I'll do, along with testing this branch together with a branch of the microscope server. Thanks for highlighting the background detect example though, that is a really useful one.

I am persuaded that trying to eliminate functional settings in this PR will cause a jam, so I'll add some multiple inheritance and work around it.

I will move discussion of whether functional settings are a good idea to an issue thread. I think it really depends on how we see the relationship between individual settings and the settings file. This changed significantly in #110 and I think we need to revisit some assumptions to make it clear how settings ought to work. Once that's well-defined, we can make sure the implementation matches the intention.

[rwb27]

I've now written a first typing test - this runs mypy over the definition of some Thing subclasses, and uses a combination of assert_type statements (which raise an error if the inferred type is not as expected) and # type: ignore statements (which will fail if the expected error is not present) to check typing should work as expected.

This helped me spot a couple of errors, and I think it's a really useful way to test the package. I don't think we need anything more complicated in the short term.

There is, however, one error I can't get rid of (without adding a # type: ignore[no-redef] which I'd really rather not do in every .Thing definition). The basic problem is that mypy treats builtins.property.setter as a special case, and there's no way to ask for that behaviour to apply to our decorator. I am not sure how we fix this, which is frustrating. It's a well known issue, with (as far as I can see) no obvious solution. For a much better description that I've written, see:

python/typing#1102

I feel like it really ought to be possible to type properties rigorously, but it's frustrating that we'd currently need to add an ignore, either on the line or globally.

[rwb27]

I feel like it really ought to be possible to type properties rigorously, but it's frustrating that we'd currently need to add an ignore, either on the line or globally.

I should correct myself: it is possible for this to work properly - we just need to deviate slightly from what @property does and use an alternative name for the setter. The following code is fine:

import labthings_fastapi as lt

class MyThing(Thing):
    @lt.property
    def the_answer(self) -> int:
        return 42

    @the_answer.setter
    def set_the_answer(self, val: int) -> None:
        if val != 42:
            raise ValueError("Don't be ridiculous.")

This isn't as beautiful a fix as I'd like, but I think it's OK, and it is probably as good as we're likely to get in the short term. It allows rigorous type checking and is familiar enough syntax that it's clear - we just need to make sure there's an explanation somewhere obvious, like the docstring for setter.

[julianstirling]

That's a shame! Perhaps a conventions such as a leading underscore, with a docstring highlighting it in examples.

import labthings_fastapi as lt

class MyThing(Thing):

    @lt.property
    def the_answer(self) -> int:
        return 42

    @the_answer.setter
    def _the_answer(self, val: int) -> None:
        """Note the leading underscore! Blah blah explanation"""
        if val != 42:
            raise ValueError("Don't be ridiculous.")

It may be possible to write a MyPy plugin to get around this for thing_properties

[rwb27]

It may be possible to write a MyPy plugin to get around this for thing_properties

I've had a look through the plugin hooks that are provided, and (while I've not attempted to actually write anything) I don't think this is possible. If it is possible, it is a big job, and one one that I think makes sense for us to tackle soon.

[rwb27]

I've also realised (thanks to my typing checks) that the descriptors don't have their type inferred properly when defined with a getter and setter. This can be resolved in one of two ways:

1. Use a separate function for decorator-style and field-style usage, so we'd have:

   class MyThing(lt.Thing):
       myprop1: int = lt.property_field(0)
   
       @lt.property
       def myprop2(self) -> int:
           return 0

2. Make default a keyword-only argument.

I've gone for option 2, on the grounds that it probably is helpful to be explicit that the value is a default, and I think it looks better to call both things property.

[rwb27]

conventions such as a leading underscore

A convention is helpful for sure - I'd favour set_ over simply _ as a prefix because it's easier to spot, and because having something called whatever and something else called _whatever is a common pattern already - especially when defining properties...

[julianstirling]

I have tested MR !348 of the OpenFlexure Microscope which uses the new syntax from this branch (and with this branch checked out). Everything works as expected.

The code looks much much cleaner with the new syntax which is great.

@rwb27; this is still in draft. What do we need to get it in?

[rwb27]

Thanks very much for testing it - I finally made a start on a new test microscope, but it's currently only 2/3 built. Hopefully up and running on Monday.

I've reviewed the todos and was able to tick off several. There are three outstanding checkboxes, which are:

• Discuss module names - I think base_descriptor is fine for now, but I am tempted to rename thing_property to properties. This would match actions and avoids having a module name that's the same as one of the symbols I've removed (I have already had one case of old code giving a confusing "module may not be executed" error because of a @lt.thing_property I'd not updated.
• Make a settings tutorial page: should bash that out Monday - though I'd also happily make an issue, merge this, and work on more docs at the maintain-a-thon.
• Revisit inheriting from property: I haven't checked whether inheriting from property would allow us to name the getter and setter the same, as is done for property. This is another one that could be moved to an issue - it's a potential future enhancement but I think this could happily be merged without it.

[rwb27]

OK, I realised that re-testing whether having FunctionalProperty inherit from builtins.property was actually really easy, thanks to the typing_tests I've introduced. I have added builtins.property as a base class of FunctionalProperty and I still get an error that the property is being redefined if the getter and setter have the same name.

It's possible some tools will work better if FunctionalProperty inherits from builtins.property, but mypy does not seem to be such a tool. I think, therefore, I can tick that box but not make any code changes.

Inheriting from builtins.property causes a bunch of problems (for example, it would reintroduce a deleter function which we don't want, and require several extra type: ignore statements). It might be possible to get around these, but unless there's a strong reason to do so I don't think it's worthwhile. Technically, FunctionalProperty is not compatible with builtins.property as a subclass, because it's more strictly typed in terms of the value (builtins.property uses a lot of Any and only works because mypy treats it as a special case).

I've added a test for the no-redef error in typing_tests so if mypy changes in the future to allow this, we'll be alerted. From what I've read, though, it's unlikely. The typed Python community seem to dislike the redefinitions required by builtins.property and that pattern is tolerated, rather than endorsed...

[julianstirling]

Thanks for the updates