After update to 0.7.0 magicgui stop handling optional return

[Czaki]

Describe the bug A clear and concise description of what the bug is.

Up to version 0.6.1, it is possible to annotate Optional[ImageData], and it works without problems.

Since 0.7.0 the https://github.com/pyapp-kit/magicgui/blob/b675ca9a3d0c24629730a92947a91799816e10e0/src/magicgui/widgets/_function_gui.py#L342 does not return add_layer callback

Is this a real bug change or an intentional change that needs to be handled on the napari side?

To Reproduce Steps to reproduce the behavior:

Use test form this PR with magicgui 0.6.1 and 0.7.2 https://github.com/napari/napari/pull/5595

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

• OS: [e.g. Windows, Mac]
• backend: [e.g. Qt 5.13.0]
• magicgui version [e.g. 0.1.0]

[tlambert03]

thanks @Czaki, good find. I'd say that calling a callback that was registered for Type when someone annotates with Optional[Type] was an "unintended feature", which was never tested explicitly. It came as a side effect before of vendoring pydantic's type wrapper (which removes the outer Optional), which was since removed in #448

I think this line here was allowing for Optional[Type] to still call the callback.

It's something of a tricky situation I guess, since I wouldn't have knowingly opted to provide implicit support for calling a callback that says it needs a Thing when someone says their function returns an Optional[Thing] (at the very least, without checking that return_value is not None...)

So, we've got a few options now (let me know what you prefer):

1. napari can explicitly opt in to supporting Optional[ImageData] by registering both ImageData and Optional[ImageData] in this line here: https://github.com/napari/napari/blob/c77e001bd7ed21303aba7a547aafdf3c90b1fed6/napari/types.py#L143-L148
2. I can add back in this accidental feature to magicgui, meaning that when anyone uses register_type(Type, return_callback=my_func), then my_func may indeed be potentially called even without an instance of Type ... that feels icky
3. I can adjust register_type to make it that anything that registers a return callback for Optional[Type] is implicitly registering support for Type... meaning that napari would still have to make a change in napari/types.py... but that they could register just Optional[ImageData].
4. I modify FunctionGui to check whether the return value is not None... and only call callbacks registered for the non-optional form in that case

my preference is probably number three... is that ok with you?

[Czaki]

I can adjust register_type to make it that anything that registers a return callback for Optional[Type] is implicitly registering support for Type... meaning that napari would still have to make a change in napari/types.py... but that they could register just Optional[ImageData].

I also like this option. But maybe it should not be done in a way that will force napari to support only latest magicgui (but maybe a set off if will be enough).

I will first implement 1 to test it But I like conversion to 3.

[tlambert03]

But maybe it should not be done in a way that will force napari to support only latest magicgui (but maybe a set off if will be enough).

yeah, I agree... whatever solution we come up with should not require napari to bump it's version requirement

[Czaki]

yeah, I agree... whatever solution we come up with should not require napari to bump it's version requirement

but it may be just if magicgui.__version__ < "0.7.x": register_optional_variant()

[Czaki]

Also type annotation should accept Types generic (Optional)

Currently mypy:

napari/types.py:158: note:     def [_T <: type] register_type(type_: _T, *, widget_type: Optional[Union[str, Union[Type[Widget], Type[WidgetProtocol]]]] = ..., return_callback: Optional[Callable[[FunctionGui[Any], Any, type], None]] = ..., **options: Any) -> _T
napari/types.py:158: note:     def register_type(type_: None = ..., *, widget_type: Optional[Union[str, Union[Type[Widget], Type[WidgetProtocol]]]] = ..., return_callback: Optional[Callable[[FunctionGui[Any], Any, type], None]] = ..., **options: Any) -> Callable[[_T], _T]

[tlambert03]

just thinking aloud here as I think about this topic (and will probably make a new issue for this). In retrospect, I think the return_callback option to register_type was probably an overreach of scope for magicgui in the first place. While it definitely allows some cool behavior in napari, this sort of magical processing of the return value seems better suited to in-n-out, or perhaps implemented directly in napari for this specific use case of adding something to the viewer with a callback connection:

# napari instantiates the widget somewhere
wdg = some_factory()

# connect callback:
if satisfies_some_condition(wdg.return_annotation):
    wdg.called.connect(_mgui.add_layer_data_to_viewer)

# add it to the viewer
viewer.window.add_dock_widget(wdg)

that will definitely require a gradual deprecation, but as I think about the complexities of processing return annotation types (e.g. do we support subtypes? etc...) i think it's probably better to remove that feature rather than elaborate upon it