napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.21k stars 422 forks source link

Accessor/Processor framework #4532

Closed tlambert03 closed 2 years ago

tlambert03 commented 2 years ago

🧰 Task

inspired by some conversations with @ctrueden, I'm going to start working on a more general "accessor/processor" framework.

more concretely, this is a generalization of what we've already implemented with type hints in magicgui. If someone annotates a function parameter as napari.Viewer, we "inject" into that function the viewer in which the magicgui widget is embedded. If they annotate their function as returning ImageData, we call viewer.add_image for them behind the scenes.

I think this is a nice pattern that:

  1. lets us allow people to write functions that don't implicitly depend on state – that is we aren't asking users to call current_viewer() to get the current viewer, they simply declare that their function requires a Viewer, and we find one for them. Or, they return a numpy array, and by nature of annotating it as ImageData, we have an idea of what to do with it. This is functional, and perhaps easier to test over time.
  2. Should be extensible. While we can certainly start with our own builtin accessors/processors, it is very easy to imagine this as an extension/plugin point. Some plugin or user could provide a mapping of type hint -> accessor/processor function and customize this behavior. Similar to magicgui.register_type, accessor/processors for non-napari types could be registered.
  3. Could be context dependent: a function annotated as def func(viewer: napari.Viewer): ... needn't always receive the current viewer. A context manager could temporarily override the mapping from type->accessor function.

The need for this started to churn in my brain at the CZI hackathon, in thinking about plugin command contributions, which are likely going to be arbitrary functions that take "some stuff" and return "some stuff". We discussed some simple rules (e.g. "if a function returns ImageData we just add it to the viewer), but this would add a level of abstraction that allows those rules to be customized or context-dependent.

Much of the logic in napari.utils._magicgui already does something like this, so PRs in the near future will mostly focus on extracting & generalizing that logic to functions outside of a magicgui context.

@jni, @DragaDoncila, @sofroniewn, @alisterburt, @andy-sweet ... curious to get your thoughts. will open some concrete PRs soon, but decided to make this task as a meta-issue to track progress


Related PRs:

alisterburt commented 2 years ago

this seems powerful - glad to hear you see a path forward which opens up alternative 'processors'

Some plugin or user could provide a mapping of type hint -> accessor/processor function and customize this behavior. Similar to magicgui.register_type, accessor/processors for non-napari types could be registered.

This sounds like something that could be used for plugin interop, pretty exciting!

we "inject" into that function the viewer in which the magicgui widget is embedded. If they annotate their function as returning ImageData, we call viewer.add_image for them behind the scenes.

Do we want to rely on type annotations of custom types here? Would defining something as a 'processor' in a plugins manifest (with a reference to the python name) similar to 'command' be more flexible/similar to what we have there? The idea that 'plugin code should just be python code' motivated this question, just want to be sure that if we're moving away from that towards more custom types it's a conscious choice

DragaDoncila commented 2 years ago

I really like this idea and am +1 on using it as a general framework for "when you give me I perform ". I agree with Alister that a command pointing to a python_name whose type annotations determine a variety of side effects (like layers added to the Viewer) is fairly magic, especially once you get into developer defined behavior, but I think it's no less magic than magicgui. I'd be curious about these commands could be documented for the user as well, to make it clear what side effects will/may occur.

tlambert03 commented 2 years ago

Do we want to rely on type annotations of custom types here? Would defining something as a 'processor' in a plugins manifest (with a reference to the python name) similar to 'command' be more flexible/similar to what we have there? The idea that 'plugin code should just be python code' motivated this question, just want to be sure that if we're moving away from that towards more custom types it's a conscious choice

Super good/important questions... I don't really see this as a move away from the "just python" code philsophy, but rather more as a way to try to satisfy napari's/@jni's desire to have something that A) doesn't explicitly have to depend on napari and/or directly use a napari viewer.add_image API while still B) achieving the semantic flexibility we need to have when someone says "hey, here's a numpy array, do something with it". Much of this discussion eventually makes it back to the image-types idea. Let me elaborate...

how do we get semantics/intention from the user/plugin

This question comes up all the time, particularly in the context of plugins. If we want to allow users/plugins to provide functions take a layer and return a layer, we need to decide what the glue in the middle is going to be that composes those things and impart intentionality/semantics to them. We could...

  1. Do our best to handle truly pure python code. For instance, if someone gave us skimage.filters.gaussian, what could we go on to know that it expects a dense array like an image? Lacking type hints there, should we guess based on the parameter name image? I think everyone agrees that's no good. Even if it were annotated with image: np.ndarray, how do we know that it wouldn't do anything meaningful with Points layer data? That's also np.ndarray.

    And what about the return value? Lacking a type hint, I guess we could check the type of the thing given to us and try to do something smart with it... but if it's just a np.ndarray, it lacks the appropriate semantics for us to know whether it's an image, surface, points, etc...

  2. Tell people to explicitly construct and return a napari Layer for us, then we can simply viewer.add_layer.

    from napari.layers import Image
    from skimage.filters import gaussian
    
    def my_func(data):
        return Image(gaussian(data))

    Here, we require no type hints at all, all the semantics are included in the object type returned by the function. That works nicely, but requires an import from napari, which everyone has generally shied away from.

  3. Tell plugins to import napari and explicitly add their results to some stateful current viewer:

    import napari
    
    def my_func(layer):
        if v := napari.viewer.current_viewer():
            v.add_image(gaussian(layer.data))

    This would be how VScode handles this sort of thing, asking extension developers to directly use the vscode api to retrieve the current window and manipulate it somehow. Every time this approach comes up, it's met with a pretty visceral "noooo" :joy: (and I can definitely see the aversion), so I'm assuming no one wants that.

  4. Rely on "type annotations of custom types" as @alisterburt put it. These would be things like napari.layers.Image, if a function wanted the layer object itself (rather napari aware), or something like napari.types.ImageData if it simply wanted a numpy-like array that had "image-like" data. Same on the return side... if it just gives us a numpy array back, we either need them to give us a specific class type back, or annotate their function with a "generally agreed upon" type annotation that ultimately just means "numpy array", but with some injected semantics (like napari.types.ImageData).

    if TYPE_CHECKING:
        from napari.types import ImageData
    
    def my_func(data: ImageData) -> ImageData:
        return gaussian(data)

This last one seems to be the one that everyone likes best, striking a balance between being "just python" while still providing a place to inject some layer-specific semantics. Type annotations can be strings, and don't require actual imports.

As stated in the comments in napari.types, things like ImageData and friends are really just "intentionality placeholders". We can't use type aliases (i.e. ImageData = np.ndarray) since they don't resolve to unique entities that can be checked, so we use NewType. These NewTypes could be extracted to an independent library (like image-types), and once PEP 646 drops, we might even be able to drop that concept and just support Array hints with a given shape/dtype.


@draga: I agree with Alister that a command pointing to a python_name whose type annotations determine a variety of side effects (like layers added to the Viewer) is fairly magic, especially once you get into developer defined behavior, but I think it's no less magic than magicgui

I definitely agree. The magic scares me a bit as well. But i'd give the same answer as above here. If we want to avoid magic, we need to bite the bullet and have people directly import and explicitly use napari APIs and types. Given that that proposal is always rejected, I don't see much of an alternative.

Given that, the ability to modify or extend the behavior of such a system (by declaring accessors/processors) at least means that we can still have some flexibility and context awareness when actually executing the semantics of the associated type hints.

@alisterburt: Would defining something as a 'processor' in a plugins manifest (with a reference to the python name) similar to 'command' be more flexible/similar to what we have there?

I don't see this framework as an alternative to that concept. It's more of a dynamic way to mark something at runtime (i.e. with the @inject_napari_dependencies decorator). It's something that an end-user could use without having to make a napari.yaml plugin manifest. We could of course imagine adding a processors field to the manifest as well, but it would likely just use this same mechanism internally to actually perform the injection/processing.

DragaDoncila commented 2 years ago

If we want to avoid magic, we need to bite the bullet and have people directly import and explicitly use napari APIs and types. Given that that proposal is always rejected, I don't see much of an alternative.

Just want to clarify that I fully support this level of magic. Not only are plugin developers likely already familiar with the concept, through magicgui, but it also provides the best balance between explicitly coding common side effects (like adding layers to viewer) and them happening completely implicitly. I just think we should document clearly so that the plugin developer isn't surprised by actions happening "outside of their control" (actually within their control, through type annotations)

alisterburt commented 2 years ago

@tlambert03 thank you for taking the time to write such a wonderfully thorough reply! Seeing the different approaches to the 'where do the semantics come from' laid out so clearly is extremely useful.

  1. Do our best to handle truly pure python code.

Totally agree with your assessment here - complete lack of awareness of napari/data semantics is too hard/magic to be reasonable

That leaves us with:

  1. Tell people to explicitly construct and return a napari Layer for us
  2. Tell plugins to import napari and explicitly add their results to some stateful current viewer
  3. Rely on "type annotations of custom types"

I don't like 2 because reusing plugin code becomes more challenging. I can imagine a world where 3 is good if the viewer that people have easy access to is actually a sort of proxy limited to whatever subset of the API we can safely allow plugin devs to use.

I'm now fully on board with why 4. is best of the options here and pretty excited about it!! will add to the discussion on image types soon :)

ctrueden commented 2 years ago
  1. Tell plugins to import napari and explicitly add their results to some stateful current viewer

I would like to add my voice here against (3). The original ImageJ assumes it is a standalone GUI application, and so plugins do everything explicitly like constructing and showing dialogs and images, which are built on GUI toolkits directly. So then there is no way to use the associated algorithm code without these side effects happening too. Whereas with ImageJ2 we have a declarative model with typed inputs and outputs and pre- and post-processing plugin chains that are customizable, so the code can be used in different contexts.

I think there is great value in having a layer in between "pure Python" and "running in the napari GUI" because otherwise you will always be writing "glue code" on top of standard Python functions to adapt the inputs&outputs to the napari context. Or more realistically: people will not bother layering their code and will just implement one function that does it all and then cannot be used except in a graphical napari context. Something like accessors+processors gives pure Puthon functions reasonable default behavior in the napari/GUI context while leaving the door open for alternative contexts such as execution headless, cloud, cluster, or embedded in other applications. E.g. with ImageJ2, commands can be executed from other tools like OMERO or KNIME or napari, but the pre- and post-processing of course behave differently for it to work.

alisterburt commented 2 years ago

@ctreuden good point - I hadn't thought of 3 as an explicit opportunity for plugin developers not to layer their code, super useful!

andy-sweet commented 2 years ago

Lots of interesting discussion, but I had a higher level question: is this framework intended to be only for plugin developers? Or for napari developers too?

I think the framework sounds great for plugin devs, as it's a formalization of some of the behavior we already have and for the reasons discussed above.

For napari devs, I'm less sure, mostly because I haven't touched many areas of the code where I feel like this would be a big win. Maybe in places we've thought about relying on dependency injection previously?

tlambert03 commented 2 years ago

Lots of interesting discussion, but I had a higher level question: is this framework intended to be only for plugin developers? Or for napari developers too?

definitely not just plugins. I see this being handy for end-users as well as internally (actually, likely internally first, then for plugins)

I think the most relevant non-plugin, end-user example is magicgui. Magicgui is not a plugin, it's just a tool that can be used by plugins... but the little mini dependency injection and return type processing framework in magicgui seems to be going pretty well as a pattern. So if we can make that sort of "takes a thing, gives a thing" accessible outside of the context of a widget, I think that would be a win.

Internally, I can also see this as being useful for context switching. For example, we were all a little apprehensive about adding the current_viewer() function, though the need was obvious. The fact that the accessor pattern here has context management actually adds a nice little level of abstraction in there that would let us control exactly which viewer/layer instance some function receives

tlambert03 commented 2 years ago

is this framework intended to be only for plugin developers? Or for napari developers too?

another point, this dependency injection thingy is already used/important in our action_manager. so formalizing/extending it here would help the action_manager be the centralized thing that controls all abstract callables that take a thing and do a thing

brisvag commented 2 years ago

Great discussion! I have (almost) nothing to add that wasn't already said; looking forward to seeing the progress.

once PEP 646 drops, we might even be able to drop that concept and just support Array hints with a given shape/dtype.

Actually, this probably won't be enouugh either, cause there's still ambiguity between, for example, a 3x3 image vs 3 points.

sofroniewn commented 2 years ago

catching up here - I'm broadly on board. I like the idea of formalizing and generalizing the pattern we're already using for magicgui and think that's probably the place to start implementation wise (I havn't looked at #4543 yet, but it sounds like that's the path there).

Something I like is that it creates the opportunity for us to explicitly adjust the return types of the accessors before injecting them.

Curious how these ideas sound to you @tlambert03

Should be extensible. While we can certainly start with our own builtin accessors/processors, it is very easy to imagine this as an extension/plugin point.

minor point, but I guess my only concern here is plugin interoperability, and we might want to start with only internal ones until we're really confident in this pattern

tlambert03 commented 2 years ago

Curious how these ideas sound to you @tlambert03

yep! All of those things would be much easier with this sort of abstraction.

brisvag commented 2 years ago

Finally I could imagine doing something extra with backwards compatibility here - maybe there is a plugin that wants the 0.4.16 viewer and a plugin that wants the 0.5.0 viewer and we are clever enough to know.

Oh wow, that's amazing! Just stick @Czaki's nme on it and we're good to go :D

tlambert03 commented 2 years ago

this has been implemented in in-n-out ... and used in #4784.

the purpose of this issue has been served, closing