Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Magic factory #117

Merged
merged 25 commits into from
Jan 23, 2021
Merged

Magic factory #117

merged 25 commits into from
Jan 23, 2021

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Jan 21, 2021

As discussed on zulip, this adds a @magic_factory decorator that, instead of returning a FunctionGui, returns a MagicFactory instance (basically just a partial subclass that we can type check) which will return the actual FunctionGui instance when called.

still need to add tests and take another pass through it, but this can be evaluated if you want @jni

update (1/21)

Here is where the API has settled and what it can do:

from magicgui import magicgui, magic_factory
from magicgui.widgets import FunctionGui, Slider, SpinBox
from magicgui._magicgui import MagicFactory

# the magicgui decorator creates a single widget instance
@magicgui
def func():
    pass

assert isinstance(func, FunctionGui)

# the magic_factory decorator creates a factory function
# that must be called to produce the widget

@magic_factory
def factory(x: int):
    pass

assert isinstance(factory, MagicFactory)
widget = factory()
assert isinstance(widget, FunctionGui)
assert isinstance(widget.x, SpinBox)

# While the `MagicFactory` will remember the kwargs provided
# to the `magic_factory` decorator, you can also override them
# individually when creating the widget

widget2 = factory(x={'widget_type': 'Slider'})
assert isinstance(widget2.x, Slider)

self-referencing also works with the @magic_factory decorator (meaning that inside the body of the function, the name of the function will be pointing to the ultimate widget instance)... with the main caveat being that this only works when decorating a module-level function:

# this works

@magic_factory
def factory(x: int):
    assert isinstance(factory, FunctionGui)
    print(factory)

widget = factory()
widget()  # prints "<FunctionGui factory(x: int = 0)>"

this, however, does not work:

def outer():
    @magic_factory
    def inner(x: int):
        assert isinstance(inner, FunctionGui)
        print(factory)
    return inner

factory = outer()
assert isinstance(factory, MagicFactory)
widget = factory()
widget()  # AssertionError

personally, I'm just fine with this caveat.

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #117 (7c8f17b) into master (813e9a7) will increase coverage by 0.08%.
The diff coverage is 93.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   90.23%   90.31%   +0.08%     
==========================================
  Files          25       25              
  Lines        2416     2437      +21     
==========================================
+ Hits         2180     2201      +21     
  Misses        236      236              
Impacted Files Coverage Δ
magicgui/widgets/_function_gui.py 96.18% <88.88%> (+0.91%) ⬆️
magicgui/_magicgui.py 96.61% <96.36%> (-3.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 813e9a7...d3a6f4d. Read the comment docs.

@jni
Copy link
Contributor

jni commented Jan 21, 2021

Ha, the failing tests in 3.7:

  E     File "/home/runner/work/magicgui/magicgui/magicgui/_magicgui.py", line 27
  E       def __call__(self, /, *args, **keywords) -> FunctionGui:
  E                          ^
  E   SyntaxError: invalid syntax

You're just too damn clever, @tlambert03! 😂 At any rate that seems like a super unnecessary use of /?

@jni
Copy link
Contributor

jni commented Jan 21, 2021

@tlambert03 as I started pondering about this in my head, I had the dreadful feeling that it wouldn't work, and indeed that feeling has been realised:

    mode = start_affinder._call_button.text  # can be "Start" or "Finish"
AttributeError: 'MagicFactory' object has no attribute '_call_button'

All of the "reflexive" tricks we do no longer work... Not sure how to deal with it though. =\

@tlambert03
Copy link
Member Author

You're just too damn clever, @tlambert03! 😂 At any rate that seems like a super unnecessary use of /?

Hah, good point, I actually just copied that straight from functools.partial.__call__ but forgot about that!

@tlambert03
Copy link
Member Author

All of the "reflexive" tricks we do no longer work... Not sure how to deal with it though. =\

Ohhhh nooo :( forgot about that stuff. My first thought is that maybe we try to use a modified locals() in magic_factory, that forwards just the name of the function? (But haven't tried to actually do that)

@tlambert03
Copy link
Member Author

ok, @jni, I think I've got a solution I'm happy with, read the update in the main post for details... the main point is that self-reference will now work fine for factories decorated at the module level, but not for factories decorated in local scopes. (which is fine with me... I'm not going to bother at the moment detecting that probably-rare scenario to raise a warning).

If you want to see the guts of how I did that see the context manager used here and defined here

@tlambert03
Copy link
Member Author

@brisvag, you may also want to look at the API in the updated first post. Let me know if you have suggestions or questions.

@brisvag
Copy link
Contributor

brisvag commented Jan 21, 2021

That looks great! Overriding kwargs is perfect 👍

What I would like to be able to do, and tried to (poorly) describe on zulip was something a bit more complex: accepting arbitrary amounts and combinations of kwargs at runtime. After thinking about it a bit more, I think it's a very different thing from what's happening here, and might be entirely different from the goal of magicgui. Or it might be already possible, and I'm thinking about it the wrong way.
Here's some mockup of what I envisioned:

@magicgui
def factory(**kwargs: Union[int, float]):
    for k, v in kwargs.items():
        if isinstance(v, int):
            # do something
        if isinstance(v, float):
            # do something else

factory(x=1, y=2.0)

And this would generate a widget equivalent to:

@magicgui
def factory(x: int, y: float):
    # stuff

@tlambert03
Copy link
Member Author

(chatted with @brisvag on zulip and have a better idea of how we might help there... probably not the factory)

@tlambert03
Copy link
Member Author

I'm not going to bother at the moment detecting that probably-rare scenario to raise a warning

oh I bothered... got a nice warning now :)

magicgui/_magicgui.py Outdated Show resolved Hide resolved
magicgui/_magicgui.py Outdated Show resolved Hide resolved
Comment on lines +55 to +58
if function is None:
return inner_func
else:
return inner_func(function)
Copy link
Contributor

Choose a reason for hiding this comment

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

In what situations does function=None make sense? Is this that weird trick that could be avoided with toolz.curry? =)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we've had this discussion before! 😂 ... but I don't want to depend on toolz

Copy link
Member Author

Choose a reason for hiding this comment

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

... and function=None makes sense when you provide parameters to the decorator:

@magicgui(call_button=True)  # magicgui is being called with function=None
def function():
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine. 🤷 I think the toolz logic ends up so much cleaner, but I'll accept this for now. 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree. looks much better that way.

Comment on lines 217 to 224
if isinstance(function, FunctionType):
if "<locals>" in function.__qualname__:
freevars = function.__code__.co_freevars
if function.__name__ in freevars:
warn(
"Self-reference detected in MagicFactory function created "
"in a local scope. FunctionGui references will not work."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just so insane. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I would suggest adding a comment above the freevars = line to explain wth you are doing here. I would have exactly zero idea if I hadn't been the cause of this thing. 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

image

??

call_button: Union[bool, str] = False,
auto_call: bool = False,
result_widget: bool = False,
main_window: Literal[True],
Copy link
Contributor

Choose a reason for hiding this comment

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

Q for my own education: does Literal work if it's not actually a literal? ie will this code pass type checking?

main_win = True
f = magic_factory(func, main_window=main_win)()
f.<something_main_function_gui_provides>()

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even worse, something where main_win is determined from external input...

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, that works

Copy link
Contributor

Choose a reason for hiding this comment

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

Wowza.

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
Copy link
Contributor

@jni jni left a comment

Choose a reason for hiding this comment

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

@tlambert03 all of my comments are for clarification, I'll let you use your judgement regarding which/how to address. Amazing PR. 😀

@tlambert03 tlambert03 merged commit b81fbf4 into pyapp-kit:master Jan 23, 2021
@tlambert03 tlambert03 deleted the magic-factory branch January 23, 2021 01:26
@tlambert03 tlambert03 restored the magic-factory branch January 23, 2021 01:26
@tlambert03 tlambert03 deleted the magic-factory branch January 23, 2021 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants