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

chore: preserve magicgui-decorated function parameter hints with ParamSpec #600

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

tlambert03
Copy link
Member

This PR uses ParamSpec to preserve function type hints when decorated with @magicgui

before this PR:

Screen Shot 2023-10-10 at 7 24 23 PM

after:

Screen Shot 2023-10-10 at 7 24 06 PM

@tlambert03 tlambert03 changed the title chore: preserve magicgui-decorated function parameter hints chore: preserve magicgui-decorated function parameter hints with ParamSpec Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3aa4b81) 87.85% compared to head (a322599) 87.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
+ Coverage   87.85%   87.86%   +0.01%     
==========================================
  Files          39       39              
  Lines        4561     4565       +4     
==========================================
+ Hits         4007     4011       +4     
  Misses        554      554              
Files Coverage Δ
src/magicgui/type_map/_magicgui.py 96.22% <100.00%> (+0.07%) ⬆️
src/magicgui/types.py 89.47% <100.00%> (ø)
src/magicgui/widgets/_function_gui.py 93.69% <100.00%> (+0.08%) ⬆️

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

@tlambert03 tlambert03 merged commit 05c9292 into pyapp-kit:main Oct 11, 2023
34 checks passed
@tlambert03 tlambert03 deleted the better-magic-typing branch October 11, 2023 11:36
@tlambert03 tlambert03 added the bug Something isn't working label Oct 20, 2023
@multimeric
Copy link

Hmm, I think this may have broken some downstream things. I believe adding an extra generic parameter to your classes changes the inheritance interface, because inheriting classes now have to add the corresponding parameter or it will be a runtime error: TypeError: Too few arguments for <class '__main__.Foo'>; actual 1, expected 2. Happy to be convinced otherwise.

The commit in question is: 05c9292

@tlambert03
Copy link
Member Author

tlambert03 commented Oct 31, 2023

It would be helpful if you could include the code you ran with Foo in it so I can see exactly what you're trying to do/inherit.

edit: are you using FunctionGui[...] as a type hint? if so, I'd recommend just putting it in quotes or using __future__.annotations ... but show me your code and I can try to help further

@multimeric
Copy link

This was happening with a class that inherited from FunctionGui, but assuming it only had 1 type parameter. However I can't replicate this anymore. Even if I cram in far too many type parameters it doesn't throw an exception:

from magicgui.widgets import FunctionGui

class MyFunctionGui(FunctionGui[int, int, int, int, int, int]):
    pass

@tlambert03
Copy link
Member Author

tlambert03 commented Nov 1, 2023

ok... this is a complicated issue to be sure and I'm still trying to determine where you will and won't get the error:

But to provide some general high level hints:

you can always use FunctionGui without parametrizing the generic. It's optional, and only necessary for static type hinting... though it does have implications for runtime as well:

class MyFunctionGui(FunctionGui): ...

Since this is only necessary for static type hinting, and not part of the actual subclass inheritance requirements, you can also guard your subclass behind TYPE_CHECKING

from typing import TYPE_CHECKING, Generic, TypeVar

T = TypeVar("T")


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

if TYPE_CHECKING:
    mybase = MyType[int]
else:
    mybase = MyType


class MySub(mybase):
    ...

in the weeds...

I admit I'm a bit amused/confused that FunctionGui[int, int, int, int, int, int] isn't throwing a TypeError ... it has to do with the inheritance of FunctionGui itself. Note that when you call SomeClass[] you're using the __class_getitem__ method: and that will hold the key as to why you sometimes do/don't see that error. This script kinda helps to show you why:

from typing import Generic, MutableSequence, TypeVar

from magicgui.widgets import FunctionGui

T = TypeVar("T")

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


fgui_cgi = FunctionGui.__class_getitem__
mt_cgi = MyType.__class_getitem__

print("FunctionGui", fgui_cgi, fgui_cgi.__module__)
print("MyType", mt_cgi, mt_cgi.__module__)

class MyType2(MutableSequence[T]): ...

print("MyType2", MyType2.__class_getitem__)

prints:

FunctionGui <bound method GenericAlias of <class 'magicgui.widgets._function_gui.FunctionGui'>> types
MyType <built-in method __class_getitem__ of type object at 0x7fa58769b980> None
MyType2 <bound method GenericAlias of <class '__main__.MyType2'>>

So simply subclassing from MutableSequence[T], and probably many other things, gives you a __class_getitem__ from the types module that doesn't care how many parameters it gets (unlike the default __class_getitem__ from the typing module, which gives you the error you saw:
https://github.com/python/cpython/blob/main/Lib/typing.py#L291


bringing it back to something concrete. Why don't you tell me more specifically where you're running into this error. link to a repo if you have one... and I'll try to help you come up with a static typing solution you're happy with, but for runtime, none of this is necessary. magicgui does need to be able to update its static typing interface, and there's really no way to deprecate that. So this change isn't going to be reverted... but there are ways that you can support both old/new magicgui at runtime and achieve any static typing you want to (though for that you'll need to run mypy assuming certain versions of magicgui).

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants