Expose key symbols at top level

[rwb27]

Currently, there are lots of imports from quite deep within this library that should be easier to find. @julianstirling suggested adding a useful set of top-level objects to the top level __init__ module in #99 , so that

from labthings_fastapi.thing import Thing
from labthings_fastapi.descriptors import ThingProperty, ThingSetting
from labthings_fastapi.decorators import thing_action

could become

from labthings_fastapi import Thing, ThingProperty, ThingSetting, thing_action

It might even become a helpful convention to import labthings as lt and then use lt.Thing etc.

This feels like it should be simple, and all the symbols above work just fine. However, I have run into problems in the past when I tried to do this (specifically for labthings_fastapi.dependencies) because of circular references. This might limit how many things can usefully be exposed at top level.

Also, at the moment there is one labthings_fastapi module that is both client and server. If labthings_fastapi is installed without the server extra, i.e. it's being used only as a client over HTTP, adding these top level imports will break it - because labthings_fastapi.thing has a hard dependency on fastapi, for example. I suspect the resolution to that is to split the package into a client and server module - but I'd welcome opinions on what makes most sense.

Depends on #110 (now merged)

Closes #99

[rwb27]

We should use __all__ to make it explicit that we're defining an API here. ruff seems happy with either that or from .thing import Thing as Thing. I will add __all__ as that clearly defines a public API, and is supported by the linter.

ruff suggests the re-export form might be more type-friendly astral-sh/ruff#717

[rwb27]

It's probably not helpful to expend too much energy on re-exports vs __all__, but I did eventually find that the former is what's required in stub files as per PEP484. I believe mypy is now happy with either form, though there may be other type checkers that prefer one or the other.

My code currently says __all__ so I'm tempted to leave that for now - but we might change at some point if the other syntax has advantages.

[barecheck]

Barecheck - Code coverage report

Total: 88.55%

Your code coverage diff: 2.80% ▴

Uncovered files and lines

File	Lines
src/labthings_fastapi/actions/__init__.py	28-29, 146, 200-206, 258-259, 289-290, 300, 324, 366-367, 372, 380, 399-400, 425-429, 433-434, 438, 452-453, 458
src/labthings_fastapi/client/in_server.py	69-71, 176
src/labthings_fastapi/dependencies/thing_server.py	15, 25, 34
src/labthings_fastapi/descriptors/endpoint.py	19
src/labthings_fastapi/server/__init__.py	76, 99, 124, 151, 159, 181-182
src/labthings_fastapi/server/cli.py	74
src/labthings_fastapi/utilities/__init__.py	10, 79, 85

[rwb27]

This now includes all the symbols @julianstirling recommended, except NDArray. NDArray is currently a stop-gap solution, and any fix to it will almost certainly change its top-level API. I think we should add that one to top level once it's a bit more robust.

[rwb27]

I also feel a little bit like the top level namespace could get quite disorganised (e.g. once Blob is joined by PNGBlob, JPEGBlob, and the rest it becomes a lot of symbols). That's aesthetics rather than best practice, but I'd be very open to suggestions for how we keep it tidy long-term.

[julianstirling]

I also feel a little bit like the top level namespace could get quite disorganised (e.g. once Blob is joined by PNGBlob, JPEGBlob, and the rest it becomes a lot of symbols). That's aesthetics rather than best practice, but I'd be very open to suggestions for how we keep it tidy long-term.

I would consider exporting blob not Blob So then we have:

import labthings as lt

lt.blob.PNGBlob
lt.blob.JPEGBlob

We probably want to do a bit of routing around the microscope codebase. The examples and the tests to see what else we tend to use?

It would be good to make the unit tests use top level import where possible so that they check that the outward facing API is stable.

[rwb27]

I would consider exporting blob not Blob

That feels like a good solution. It might then be sensible to add an __all__ to Blob?

It would be good to make the unit tests use top level import where possible so that they check that the outward facing API is stable.

Was working on that as you wrote the comment :)

[rwb27]

Similar to blob, are there other things that might want to live in their own namespace? I'd consider the dependencies ("BlockingPortal", "InvocationID", "InvocationLogger", "CancelHook", "GetThingStates", "raw_thing_dependency", "direct_thing_client_dependency") and maybe the outputs/types (currently just MJPEGStreamDescriptor but hopefully likely to grow)

[julianstirling]

I think I agree with that.

lt.outputs.MJPEGStreamDescriptor seems fine.
lt.dependencies.raw_thing_dependency is also not that long

[rwb27]

I've finally found the issue I had trying to do this before: if I pull things up from submodules of dependencies into the __init__ in dependencies (so I could do from lt.dependencies import Whatever), I create a circular reference. That's because .thing depends on .dependencies.action_manager, and .dependencies.raw_thing depends on .thing. I don't think I ever properly worked this through before, but what's happening is that adding imports to .dependencies creates a circular import chain.

That isn't a problem for blob, and might just go away as and when we do a bit of rearranging. It's probably not critical to have all the dependencies in one submodule, they might be more logically grouped by function e.g. ActionManagerDep with ActionManager in the actions submodule.

[julianstirling]

I've finally found the issue I had trying to do this before: if I pull things up from submodules of dependencies into the __init__ in dependencies (so I could do from lt.dependencies import Whatever), I create a circular reference. That's because .thing depends on .dependencies.action_manager, and .dependencies.raw_thing depends on .thing. I don't think I ever properly worked this through before, but what's happening is that adding imports to .dependencies creates a circular import chain.

I don't understand the relationship between .thing and Whatever do you have a branch with the bug and the actual error message?

[julianstirling]

Right I think I get what you mean looking at it. I didn't realise that there is a thing.py in dependencies as well as a thing.py at the top level.

I think somewhat the pain comes from laying out the in packages, but each sub_packages but each sub_package defines a type of object like "descriptors" or "decorators" this means that there is a lot of circular dependency between packages. Running PyDeps on the code gives a quite scary dependency graph:

It feels like it is probably best to think about the functionality from a heiarchical point of view for organisation before we try to tidy up the name space. This about the package as sub packages and which need to depend on each other (for anything more than typing). My thought is if the code was divided into 5ish functional sub-packaged such as with clear dependency logic this may help:

I don't understand the internals well enough to know if this dependency logic would try. But trying to work out the depencency logic I bounce around in and out of sub-packages such as dependencies multiple times so it is no wonder there are circular dependencies.

I think it seems like this is becoming a bigger task than we initially thought, and getting the new settings logic released is key to progress. So I'd suggest we park this and try and release v0.0.10 with the new setting logic

[rwb27]

I'd really like to organise the dependencies - but I am a bit stumped as to the right thing to do. I think it would be clear to group them, so they are lt.dependencies.WhateverType, but that would imply they can also be imported as from labthings_fastapi.dependencies import WhateverType which I can't do without a fair bit of refactoring because of the circular dependency it would create. Doing something in the top level __init__ like from .dependencies import convenience_imports as dependencies would create a confusing situation where it looks like symbols can be imported from labthings_fastapi.dependencies but actually they can't. I anticipate @julianstirling will agree doing something confusing like that is unhelpful but it's worth checking!

Also, because dependency type hints appear in lots of function arguments, there probably is an argument for keeping them short - but it's also helpful to know they are dependencies. Should we consider a naming convention, like ending them all in Dep?

An option worth considering would be adding a submodule labthings_fastapi.deps which could import everything. The implementations of the dependencies can stay where they are for now, we get a convenient way to import them without a messy namespace, and I think it makes things clear (if we have a module docstring) that the symbols are dependencies. I'll do that for now, but it's worth considering if it's the right answer.

[rwb27]

An alternative option for dependencies would be to move the implementation of direct_thing_client_dependency and raw_thing_dependency outside of dependencies and into, for example, a submodule dedicated to inter-thing interaction. That would probably then encourage people to do something like:

import labthings_fastapi as lt
from labthings_fastapi.dependencies import WhateverDepsINeed

This would keep type hints short but hopefully also make it clear that they are dependencies.

I think the deps module does the same thing, so I'm going to leave that in place for now, much though I dislike abbreviations. deps does have a module level docstring directing people to the documentation.

[rwb27]

Sorry, I hadn't seen your graphs before I made the last commits. That does look scary!

I think this PR creates a reasonable API for people writing Things, and paves the way for rearrangements to happen under the hood. As such, it might be worth merging it soon and releasing v0.0.10, then refactoring.

I don't think your proposed structure will work quite like that, but I'm sure something along those lines will be possible, with a bit of thought. Splitting some of the client code into its own module will help there.

I've not yet addressed the issue of broken client-only installs (#120). I'll suggest a temporary fix for that and we can figure out how to split the client code for the next release.

[julianstirling]

I've been doing a bit of reading about dependencies, it is something that I don't fully understand but I have had far fewer issues. Looking at advice there seems to be bits around the place including:

• Deeply nested packages are harder to avoid loops
• Relative imports except with single . at the subpackage level hide dependency loops as it is less explicit what is importing what.
• Relative imports with .. in __init__.py files are particularly problematic.

Personally I try to avoid putting code in __init__.py files except imports. This is less because it something I can back up with a good reason on why it is worse at run/import time. But just because having editor tabs where half the file names are __init__.py annoys me.

One interesting thing to look at is pandas. Pandas is really deeply nested, but there are two things they do to make the API clear:

1. Internal things seem to generally import from padas.x.y import z
2. The main subpackages that are imported like core are have and empty __init__.py, and then a file called api.py with an __all__. This way the top-level __init__.py doesn't import things from .core it imports from pandas.core.api and as the __init__.py files for core is empty I think this avoids the dependency loops.

As for reviewing this. I would like to play with how it works on the microscope repo before we merge.

[rwb27]

I'm not totally sure I follow what you mean with relative imports "except with single ." - is there a link or two you could share so we are reading the same things?

I have already tried out the current imports on the test suite, and it certainly tidies things up. Clearly we'll need to reorganise the microscope imports before or after this goes in, so it makes sense to test it out to avoid releasing an API that gets binned again; if we like it, it's not wasted work.

Reorganising the code is a bigger job, for a future PR. I think, though, it is worth having an API we can keep stable while we refactor, even if it only lasts a few versions - hence this PR.

I see the value of the Pandas solution, with "summary" imports that aren't in __init__. I guess that's a lot like what I did in deps, but it would have been .dependencies.api which certainly keeps the code tidier. That said, I think the long-term home of the dependency code is probably not a dependencies submodule at which point putting the convenience import somewhere else is better.

I'll close by making another suggestion: rather than putting these symbols in the top level, we could do what matplotlib does and have a module exposing the API, e.g. from labthings_fastapi import thing_toolkit as lt. That would sidestep #120 and avoid any circular dependencies, while still tidying up imports around the microscope and example code. If we decide to move it later (including to top level) it's a simple search and replace.

[julianstirling]

I'm not totally sure I follow what you mean with relative imports "except with single ." - is there a link or two you could share so we are reading the same things?

As in within the same sub-package is normally fine:

from .foo import bar

But using it to access a different sub-package isn't recommended:

from ..foo import bar

I'll close by making another suggestion: rather than putting these symbols in the top level, we could do what matplotlib does and have a module exposing the API, e.g. from labthings_fastapi import thing_toolkit as lt. That would sidestep #120 and avoid any circular dependencies, while still tidying up imports around the microscope and example code. If we decide to move it later (including to top level) it's a simple search and replace.

Oh, I think this could be nice! Because hopefully it creates a bit of a separation between those writing a server, who need Things, Thing Severs, and property decorators, and those just using LabThings to talk to an existing server:

from labthings_fastapi import server_api as lt

OR

from labthings_fastapi import client_api as lt

[julianstirling]

Two suggestion with which !296 on the microscope works in simulation.

[julianstirling]

I'm just looking at what is needed to merge this. The examples, and the docs need updating as well as the test. As for the examples. There are some quite old camera examples which are probably going out of sync with the codebase. It probably feels better to point to the v3 development for more complex examples than to have more complex examples that don't work?

[rwb27]

I'm just looking at what is needed to merge this. The examples, and the docs need updating as well as the test. As for the examples. There are some quite old camera examples which are probably going out of sync with the codebase. It probably feels better to point to the v3 development for more complex examples than to have more complex examples that don't work?

yes, I think I mentioned that in a previous MR as well. I can delete the camera examples now they are superseded by the openflexure server. No point duplicating effort there. Those examples can always come back if we want them later.

[bprobert97]

Wanted to add my thoughts about the dependency structure conversation. Python dependencies are a complicated nightmare at the best of times!

But using it to access a different sub-package isn't recommended:
from ..foo import bar

Absolutely agree with this point! This can get messy, and is quite difficult to debug. Sub-packages should not have relative (..) visibility of anything outside of itself.

The dependency map tells me that we have way too many packages. I think the hierarchical approach could be a good one to start with. I also like the idea of splitting out the server and client APIs into separate packages as they serve very distinct and unique purposes.

Much like the pandas solution, I'd also advise keeping the init.py files as empty as possible. As said here:

A commonly seen issue is adding too much code to init.py files. When the project complexity grows, there may be sub-packages and sub-sub-packages in a deep directory structure. In this case, importing a single item from a sub-sub-package will require executing all init.py files met while traversing the tree.

In terms of exactly how we restructure the code, I think a useful exercise to do would be to identify which functions/imports are the main culprits for circular imports, and move them into their own sub-package/packages, like Richard said here:

I'll close by making another suggestion: rather than putting these symbols in the top level, we could do what matplotlib does and have a module exposing the API, e.g. from labthings_fastapi import thing_toolkit as lt. That would sidestep #120 and avoid any circular dependencies, while still tidying up imports around the microscope and example code. If we decide to move it later (including to top level) it's a simple search and replace.

We need to try and keep each sub-package as decoupled from others as possible, so grouping them by function/use case (as per Julian's diagram) could be a good way to do this.

[rwb27]

Thanks @bprobert97 and @julianstirling, that all makes a lot of sense. Thinking through how it works, I think mostly it can be structured so sub packages don't have too many interdependencies. I think typing makes it harder, and there are certainly likely to be types that require from .. imports in many places. Perhaps part of the problem is that I've made my modules quite small, and maybe I just need to use longer files - but I've never seen that recommended as a solution!

I have not been particularly diligent about using the TYPE_CHECKING flag for imports, and doing that will result in a lot fewer dependencies (at least as detected by hard import statements). However, that might require some changes to how we use FastAPI dependencies. I suspect said changes might bring us more in line with how they are intended to work, so they are probably no bad thing...

[rwb27]

I think it would be good to decide how we want to proceed with this PR, it's getting quite a long discussion already. I think the sensible options are:

1. Merge more or less as-is, allowing key symbols to be imported from the top level module (import labthings_fastapi as lt)
2. Change this PR to create an API module (from labthings_fastapi import thing_toolkit as lt)
3. Close and don't merge this PR, and restructure the API after we've refactored the code

I think I might prefer (2), but I don't have a strong opinion. I'm also not trying to exclude other options, I just haven't thought of any...

[rwb27]

Also: clearly the refactoring exercise will conflict horribly with everything else, so we'll want to make sure there are not too many other open PRs when it happens...

[julianstirling]

I think it would be good to decide how we want to proceed with this PR, it's getting quite a long discussion already. I think the sensible options are:

1. Merge more or less as-is, allowing key symbols to be imported from the top level module (`import labthings_fastapi as lt`)

2. Change this PR to create an API module (`from labthings_fastapi import thing_toolkit as lt`)

3. Close and don't merge this PR, and restructure the API after we've refactored the code

I think I might prefer (2), but I don't have a strong opinion. I'm also not trying to exclude other options, I just haven't thought of any...

I'd go for 1. It certainly improves things, and then we can then carefully reorganise the code base behind so that the the same imports work.

I do think the docs and examples need updating before it goes in.

[bprobert97]

I think 1 is the simplest answer for now - it makes improvements that we can build on. More documentation would also be great! Do we need to open an issue for the rest of the refactoring changes?

[rwb27]

I think this is ready for re-review @julianstirling @bprobert97.

[bprobert97]

Looks good to me!

[julianstirling]

Awesome. Thank you Richard.