Refactor descriptors, decorators and actions

[rwb27]

This PR introduces one API change: thing_action is now action. This is largely for consistency with property. We dropped the thing prefix when we started encouraging import labthings_fastapi as lt so it becomes @lt.action which doesn't need a prefix.

I've also rearranged code between some modules: actions is a single-file module that includes actions/__init__.py and the action-related descriptor and decorator. InvocationModel is moved into its own top-level module invocations.

fastapi_endpoint is moved into a top level endpoints module which clears the way to get rid of descriptors and decorators because decorators and descriptors are now combined with the code they belong with.

invocations might in the future gain the Invocation Thread subclass and/or ActionManager (which might want to be renamed InvocationManager as that's what it actually manages), but that's left for the future.

There is a minor change in functionality, because EndpointDescriptor and ActionDescriptor are now subclasses of BaseDescriptor. This means that:

• The title, name, and description properties are all now consolidated in BaseDescriptor.
• __doc__ is set in __init__ to the docstring of the decorated function (and will then be picked up by description from there, so the behaviour is unchanged).
• We take the name of the class attribute, rather than the name of the function being decorated. This was always the intended behaviour, and in normal usage as a decorator the two are identical.
• We will look for attribute-style docstrings (a string literal following an assignment).

If used as a decorator, nothing changes. If, however, someone does something strange, we might see differences:

class OddThing(lt.Thing):
    def do_something(self):
        """A docstring."""
        print("You probably can't read this")

    strange = lt.action(do_something)
    """An attribute docstring"""

The code above would be valid before and after this change (give or take lt.thing_action being renamed), but before we'd have an action called do_something with A docstring as its description, while afterwards we'd have an action called strange with An attribute docstring. This is not something I expect anybody will encounter, and I don't believe it occurs anywhere in the microscope codebase.

To consider before merge:

• Discuss whether it's better to move Invocation, possibly and ActionManager into invocations. May as well only have one merge commit that moves that code?
• If we move ActionManager can we rename it InvocationManager?
• Should I add a backwards-compatibility wrapper lt.thing_action that calls lt.action and raises a DeprecationWarning?

[barecheck]

Barecheck - Code coverage report

Total: 95.04%

Your code coverage diff: -0.05% ▾

Uncovered files and lines

File	Lines
src/labthings_fastapi/actions.py	163-164, 415-416, 544-545, 558, 583-584, 685, 689, 861-862, 865, 868, 916
src/labthings_fastapi/client/in_server.py	128, 331
src/labthings_fastapi/endpoints.py	105
src/labthings_fastapi/example_things/__init__.py	168
src/labthings_fastapi/outputs/blob.py	369, 455, 574, 660
src/labthings_fastapi/thing.py	252, 258, 283-285, 317

[julianstirling]

To the bigger picture questions/points:

Not one of the questions, but I think fastapi_endpoint isn't a good name. It is describing what back end library it is using not the feature. It is like if FastAPI prepended starlette to a bunch of feature names. I would call it either http_endpoint or just endpoint.

Discuss whether it's better to move Invocation, possibly and ActionManager into invocations. May as well only have one merge commit that moves that code?

If this tidying doesn't affect the public API then it seems a good idea to do it at some point. I think having two separate PRs would be better as there is no good reason to minimise merge commits and making this any bigger will make review very hard. I would rather merge this and do it as a separate PR.

If we move ActionManager can we rename it InvocationManager?

Makes sense to me,this should be an internal only change? As the downstream libraries don't talk to this directly?

Should I add a backwards-compatibility wrapper lt.thing_action that calls lt.action and raises a DeprecationWarning?

I wouldn't we haven't done this for property vs thing_property. It is a simple find and replace, and there are not a lot of LabThings-fastapi projects in the wild. It isn't like dependencies where the actual code to changes are large and will want to be done gradually.

[rwb27]

Thanks - that all sounds very sensible to me. My rationale for moving Invocation and ActionManager now rather than in the future is that I don't think it changes the size of the PR very much: the code is getting moved either way, it's just a question of where.

However, you correctly point out that it's an internal-only rearrangement. Similarly, downstream code should not be aware of ActionManager so we can rename it in the future.

Point taken about warnings/compatibility - I will happily leave it as-is.

I actually considered renaming endpoint but thought not to do it as I'd already scope-crept here. I would be happy to rename it, as you say fastapi is not a useful prefix. I've wondered about creating different decorators for the different HTTP verbs, but I think fastapi_endpoint is already used sufficiently seldom that this isn't a change I need to rush in for v0.0.12.

So, happily I think we've concluded "no" to the three things I suggested could be added here.

I may as well rename fastapi_endpoint for v0.0.12 because I think we both agree it makes sense. That can be a future, trivial PR though.

[rwb27]

I think this is probably good for review now. I think I should probably just be OK with merging a 0.05% drop in coverage, I can add tests later, and it's a good idea to get in to the habit of not being too obsessive about coverage, as you've pointed out before.

[julianstirling]

I actually considered renaming endpoint but thought not to do it as I'd already scope-crept here. I would be happy to rename it, as you say fastapi is not a useful prefix. I've wondered about creating different decorators for the different HTTP verbs, but I think fastapi_endpoint is already used sufficiently seldom that this isn't a change I need to rush in for v0.0.12.

So, happily I think we've concluded "no" to the three things I suggested could be added here.

I may as well rename fastapi_endpoint for v0.0.12 because I think we both agree it makes sense. That can be a future, trivial PR though.

Yeah, I think a future PR makes sense for that.

I think I would probably keep it as one decorator, just because it is something rarely used and if someone isn't used to HTTP a lt.get isn't very descriptive and lt.get_endpoint sounds like it is trying to "get and endpoint".

[julianstirling]

This looks great. I much prefer the new flatter structure.

It is also great to see that the tests only needed lt.thing_action changed to lt.action and nothing else. I think lt.action is the correct syntax and it should be a simple find and replace to update code.

[rwb27]

For the record, I made an updated dependency graph. It's messier than I like, but I think it's a big improvement on previous iterations. I have excluded the dependencies submodule as it will be removed soon.

Code to generate

* Install `pydeps` * Install graphviz (at system level) * Search and replace in VSCode `^if TYPE_CHECKING:(\n\s*from .*)*\n\n` to remove typing imports * `pydeps src/labthings_fastapi -T svg -x fastapi pydantic anyio jsonschema starlette numpy typing_extensions httpx uvicorn labthings_fastapi.dependencies labthings_fastapi.deps --max-bacon=4 --cluster --min-cluster-size=1 --keep-target-cluster --rmprefix labthings_fastapi. --no-show`