Skip to content

docs: documentation overhaul#142

Merged
tlambert03 merged 16 commits intopyapp-kit:mainfrom
tlambert03:docs
Nov 3, 2023
Merged

docs: documentation overhaul#142
tlambert03 merged 16 commits intopyapp-kit:mainfrom
tlambert03:docs

Conversation

@tlambert03
Copy link
Copy Markdown
Member

@tlambert03 tlambert03 commented Nov 2, 2023

general docs overhaul, with better full-api rendering

@lucyleeow, as one who probably looks/works with the docs here most, I'd be grateful for any feedback you have. I like the general layout better, and it's much more automated, (it does expose a few places where docstrings could be expanded, but happy to follow up on that in another PR)

you can preview the build here: https://app-model--142.org.readthedocs.build/

@tlambert03 tlambert03 changed the title Docs: documentation overhaul docs: documentation overhaul Nov 2, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9463e67) 100.00% compared to head (0ce61b9) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #142   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1761      1761           
=========================================
  Hits          1761      1761           
Files Coverage Δ
src/app_model/_app.py 100.00% <ø> (ø)
src/app_model/backends/qt/_qaction.py 100.00% <ø> (ø)
src/app_model/backends/qt/_qkeymap.py 100.00% <100.00%> (ø)
src/app_model/expressions/_expressions.py 100.00% <ø> (ø)
src/app_model/types/__init__.py 100.00% <ø> (ø)
src/app_model/types/_action.py 100.00% <ø> (ø)
src/app_model/types/_keys/_key_codes.py 100.00% <ø> (ø)
src/app_model/types/_keys/_keybindings.py 100.00% <100.00%> (ø)
src/app_model/types/_menu_rule.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started drafting notes for some docs for using app-model in napari and I swear some of it is word for word what you've written here.

I don't fully get contexts but app-model does not keep a registry of contexts? I guess this is why it only updates from an empty context?

# by updating from an empty context, anything that declares a "constant"
# enablement expression (like `'False'`) will be evaluated, allowing any
# menus that are always on/off, to be shown/hidden as needed.
# Everything else will fail without a proper context.
# TODO: as we improve where the context comes from, this could be removed.

Maybe a note about this and future plans?

The getting_started is really great, logical order and clear explanations (I may be biased because it's similar to how I would have done it 😅). I also found the motivations section useful. (Also like that you can easily get links to headers now)

@lucyleeow
Copy link
Copy Markdown
Collaborator

Oh while you're here, I noticed that you can't build a QModelMenu for submenus because during teardown error results:

see: https://github.com/napari/napari/pull/4865/files#r1288031823

Is this intended?

@tlambert03
Copy link
Copy Markdown
Member Author

tlambert03 commented Nov 3, 2023

but app-model does not keep a registry of contexts? I guess this is why it only updates from an empty context?

Not yet. It's still up to the end-user to manage their contexts. That was always planned, but just something I wanted to consider further. I do think that a top level Application.context would be good and will add and document in a follow up.

I don't fully get contexts

I think that the vscode extensions docs still provide some of the best general motivation on this... (and of course https://napari.org/stable/guides/contexts_expressions.html)

but the basic idea is that we need a declarative way to express conditions, so that we can set up various things to happen based on those conditions. In many ways, it overlaps with the concept of signals and slots... but without exposing as much of the underlying business logic to the plugin. For example, you could just literally pass your entire QMainWindow to a plugin and say "go nuts, connect whatever callbacks you want to any of the signals you can find in here"... and then you probably wouldn't need contexts. But, as you can imagine, that could very easily get unruly, and it also exposes a ton of stuff that you now can no longer easily change without breaking usage.

Contexts let the application say: "here are the things you, the developer, can query about the state of the application". By declaring special names, like vscode does here (and like I would encourage napari to do as well, like I started here), you do this in a fully declarative and serializable way. So the developer can give you an expression “active_layer_is_rgb | all_layers_same_shape” using variable names that are present in your context, and you (the application) connect the proper signals to make sure that the necessary stuff happens when that expression becomes true/false (for example, making a menu item visible or not, or a button enabled or disabled, or anything like that)

that napari document above was my attempt to capture that in the context of napari, and I'll try to express it here as well but probably in a follow up PR.

edit: it would be great to know more specifically what is still confusing about contexts. If, after reading that doc, any questions still come to mind, i'd love to hear them! I don't expect it to be immediately obvious, but i need a little guidance about which bits in general are confusing (whether it's the high level "why do we need this in the first place?" or a low level "how does this get implemented")

@tlambert03
Copy link
Copy Markdown
Member Author

tlambert03 commented Nov 3, 2023

Oh while you're here, I noticed that you can't build a QModelMenu for submenus because during teardown error results:

exceptions are almost never intended 😂 please open issues when you find stuff like this! I'm not following napari issues so won't see stuff like that unless you ping me or open an issue.. but I'm more than happy to look into it when you encounter anything unexpected in app-model!

@tlambert03
Copy link
Copy Markdown
Member Author

going to merge this as an improvement over what we already have. Will follow up with @lucyleeow's improvement suggestions in new PRs

@tlambert03 tlambert03 enabled auto-merge (squash) November 3, 2023 15:24
@tlambert03 tlambert03 merged commit 9e1d527 into pyapp-kit:main Nov 3, 2023
@lucyleeow
Copy link
Copy Markdown
Collaborator

please open issues when you find stuff like this!

Will do thanks. We didn't want to bother you with this one as it wasn't something we needed and we weren't sure of intention. I'll add details to the issue.

@tlambert03
Copy link
Copy Markdown
Member Author

I don't find it a bother :) I like knowing if there's something that the tests aren't catching here!

@lucyleeow
Copy link
Copy Markdown
Collaborator

lucyleeow commented Nov 8, 2023

it would be great to know more specifically what is still confusing about contexts

Thank you for the background, I understood at a high level but the details are useful to know!

I was more confused about the implementation. For some of the ContextNamespace and Context stuff, I can guess what things are doing but I don't know whats happening 'under the hood'/don't understand the syntax.

  • What does the ['LayerSel'] syntax mean? And the first item num_selected_layers, is this like a pydantic field / class attribute?
class LayerListSelectionContextKeys(ContextNamespace['LayerSel']):
    """Available context keys relating to the selection in a LayerList.

    Consists of a default value, a description, and a function to retrieve the
    current value from `layers.selection`.
    """

    num_selected_layers = ContextKey(
        0,
        trans._("Number of currently selected layers."),
        _len,
    )
        # TODO: figure out how to move this context creation bit.
        # Ideally, the app should be aware of the layerlist, but not vice versa.
        # This could probably be done by having the layerlist emit events that
        # the app connects to, then the `_ctx` object would live on the app,
        # (not here)

In what way does the app know about the layerlist atm? We use app-models create_context but AFAICT we aren't connecting anything to app-model. Or is it more for when app-model has a context registry?

@DragaDoncila
Copy link
Copy Markdown
Contributor

@lucyleeow I have superficial answers to these questions but most likely @tlambert03 has more detail.

What does the ['LayerSel'] syntax mean?

The ['LayerSel'] syntax here is, I think based on its type definition here, defining/assigning a generic type for this particular ContextNamespace. I'm generally familiar with the idea of a generic, but I have to confess I don't really know how it's helpful here... Because the LayerSel type isn't a "real" type like str or int so I don't know what effect it has - is it just for type hinting?

And the first item num_selected_layers, is this like a pydantic field / class attribute?

I think this is a class attribute, as I don't see any pydantic inheritance in the Context chain. Certainly it is accessible as a class attribute on the LayerListContextKeys (rather than an instance attribute).

Where does get_context get _OBJ_TO_CONTEXT? Is it a global variable? (is it updated at runtime?)

_OBJ_TO_CONTEXT seems to just be a module level variable? If I understand its purpose, it is to map between a given instance of an object (more specifically, the ID of that object) to the current context of that object, if one has been defined. It is updated whenever a context is created - though I don't understand the benefit of just storing it on the module like that, rather than as a singleton on the app or something - I guess that's where the context registry would come in?

In what way does the app know about the layerlist atm?

Right now, I think the app does not know about the layerlist at all. It's napari's job (I think in the qtmainwindow?) to connect the right callbacks to update the context. I think that's the bit that is considered not great and that should be fixed by adding a context registry to app-model...

@lucyleeow
Copy link
Copy Markdown
Collaborator

Because the LayerSel type isn't a "real" type like str or int so I don't know what effect it has - is it just for type hinting?

I guess the question is what is it's purpose? Is it just for type hinting? The other question is a e.g., LayerListContextKeys and LayerListSelectionContextKeys are both instantiated with generic Contexts. The only difference is the events they get connected to, so is this just how their different 'input's (layers vs selected layers) are implemented? I guess I did not connect that originally.

@tlambert03
Copy link
Copy Markdown
Member Author

Is it just for type hinting?

yep, it's entirely just for type hinting. whenever you see something like this, where you have a Generic type that is parameterized by some other type, you can think of it as the outer type "taking" the inner type. Lemme try to explain some of those terms with examples:

from typing import Generic, TypeVar
T = TypeVar("T")

class MyContainer(Generic[T]):
    ...

when you see something like this, it means: MyContainer is a class that will take, contain, or otherwise deal with some other type. So, two questions immediately might be:

  • What kind of other type? In this case it varies (as indicated by the TypeVar T). As written here, it could be literally anything, but we could have made T = TypeVar("T", bound=Union[int, str]), to state that MyContainer only deals with integers or strings

  • What does it do with that type? For this you need to see where the type parameter is used... so look deeper into the class:

    class MyContainer(Generic[T]):
        def __init__(self, thing: T):
            self._thing = thing
    
        def get_thing(self) -> T:
            return self._thing

    Ah, ok this is a class that has a get_thing method that will return an instance of whatever T is...

Still weird? probably, but the next step is to "parametrize" the generic in a subclass, by replacing the type variable "T" with a concrete type:

class MyIntContainer(MyContainer[int]):
   ...

The MyContainer[int] syntax says that this subclass specifically only deals with integers. This syntax immediately implies that the thing argument to __init__ must be an integer, and it also immediately tells us that the MyIntContainer .get_thing method will return an integer.

so, going back to ContextNamespace:

class LayerListSelectionContextKeys(ContextNamespace['LayerSel']):

this tells us that LayerListSelectionContextKeys is a specific variant of ContextNamespace that deals with LayerSel types, where LayerSel is just an alias for LayerSel = Selection[Layer] (i.e. a selection of Layers). Ok, so LayerListSelectionContextKeys deals with selection sets of layers... what does it do with them? We have to look at the ContextNamespace code to find out:

class ContextNamespace(Generic[A]):
    def __init__(self, context: MutableMapping) -> None:
        ...
        self._getters: Dict[str, Callable[[A], Any]] = {}  # value getters

ok: all ContextNamespace subclasses are going to collect a bunch of getters, which are defined as functions that take in an instance of the type mentioned above and return anything (Callable[[A], Any]). Meaning, LayerListSelectionContextKeys is going to collect a bunch of functions that take a LayerSel and do something with them. (specifically, in this case, it's going to call those functions to update the value of that specific context key when necessary). Those functions, that take a LayerSel are all right here: https://github.com/napari/napari/blob/c6abeb5d6efed2c6ba46e8d415fd3cef5fdf6521/napari/_app_model/context/_layerlist_context.py#L40-L121


Why bother with all the type hinting? because when done properly it can make the developer experience really nice by alterting you to possible bugs and errors in your code before you even run anything. It can tell you that the function you're trying to use as to update one of the keys in your Context is not going to work, because it doesn't accept the right type(s).

see this bit of the napari docs for a bit more on how the type hinting here gives you type feedback in your IDE


is it more trouble than it's worth? perhaps now it is. I imagined that I would be working on this in napari for longer, and that doing this with careful type hinting would be able to provide a very nice developer experience. Now, however, I can see that it does add cognitive burden for those not already intimately familiar with Generic types and parameterization. You can feel free to either ignore it, or even remove it if it feels too cumbersome.

@tlambert03
Copy link
Copy Markdown
Member Author

btw, i think the mypy docs on generics and parameterization is better than the python docs:
https://mypy.readthedocs.io/en/stable/generics.html

@lucyleeow
Copy link
Copy Markdown
Collaborator

🤯 it makes so much more sense now, thanks!

@tlambert03
Copy link
Copy Markdown
Member Author

oh, and there's one more typing-related step:

note how all of the functions LayerSel functions (the "getters" whose types were above defined as Callable[[A], Any]) return some value. For example:

def _n_unselected_links(s: LayerSel) -> int:
    from napari.layers.utils._link_layers import get_linked_layers

    return len(get_linked_layers(*s) - s)

let's further look to see where the _n_unselected_links function is used in napari. Lower down in the file, it's passed as the third argument to ContextKey:

    num_unselected_linked_layers = ContextKey(
        0,
        trans._("Number of unselected layers linked to selected layer(s)."),
        _n_unselected_links,
    )

well, if you look at the ContextKey class itself... you'll see that it too is a generic, that is parametrized by two type variables A and T. Specifically, the third argument expects a getter function that takes an A and returns a T (Callable[[A], T]).

class ContextKey(Name, Generic[A, T]):
    def __init__(
        self,
        default_value: T | __missing = MISSING,
        description: str | None = None,
        getter: Callable[[A], T] | None = None,
        *,
        id: str = "",  # optional because of __set_name__
    ) -> None:

So now, we see how the return value of that function above might be used in type hinting. Specifically, it's used further down in the __get__ method:

class ContextKey(Name, Generic[A, T]):
    def __get__(self, obj: ContextNamespace[A], objtype: type) -> T:
        # When we get from the object, we return the current value
        ...

(note that ContextKey is a descriptor object, kinda like @property and __get__ is a special method that determines what will be returned if this name is accessed on an instance of the class in which it is defined as a class variable)

So, if you put that all together:

def _n_unselected_links(s: LayerSel) -> int:
    ...

class LayerListSelectionContextKeys(ContextNamespace['LayerSel']):
    num_unselected_linked_layers = ContextKey(
        0,
        "Number of unselected layers linked to selected layer(s).",
        _n_unselected_links,
    )
  1. LayerListSelectionContextKeys will deal with a set of ContextKeys whose getter functions all take a LayerSel as their argument
  2. _n_unselected_links is a valid getter function can take a selection of layers and return an integer (the number of selected layers)
  3. num_unselected_linked_layers is a context key (i.e. a name that has special meaning, which could be used by napari and/or plugin developers to refer declaratively to the concept of "the number of currently selected layers"). Because it's third argument is _n_unselected_links (a function that returns an int, we know that num_unselected_linked_layers will always give the user an integer).

And so if someone were to use LayerListSelectionContextKeys(). num_unselected_linked_layers (either to get or set the value), the IDE would know that it should be an integer, and mypy could give an error if they tried to do otherwise. And give you hints like this:


at least that was the motivation behind the design 🫠

@tlambert03
Copy link
Copy Markdown
Member Author

and (sorry) one more comment:

I recognize that this all is rather complicated. I want to clarify that I never intended for anyone to have to understand all of that to just use the system. My primary target for napari developers was just this final usage here:

class LayerListSelectionContextKeys(ContextNamespace):
    num_unselected_linked_layers = ContextKey(
        0,
        "Number of unselected layers linked to selected layer(s).",
        _n_unselected_links,
    )

I wanted napari developers to be able to use that relatively straightforward syntax to declare context key names, defaults, descriptions, and getter functions – and to be able to group related context keys in a ContextNamespace - and have all the rest "just work" in their IDE, giving them type hinting and allowing us to run mypy checks on everything. I definitely didn't expect more than a couple people to have to follow all of the generics and parametrization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Development

Successfully merging this pull request may close these issues.

3 participants