-
Notifications
You must be signed in to change notification settings - Fork 454
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
Union typehint in conjunction with coders causes error for PyTorchModelHubMixin #2283
Comments
Good catch @gorold, thanks for reporting! Would you like to open a PR to fix this? The line |
Sure, I could take a stab at it, but I think there's some undefined behaviours that I'd like to clarify first.
class ParentArg: ...
class ChildArg(ParentArg): ...
class Model(
nn.Module,
PyTorchModelHubMixin,
coders={ParentArg: ..., ChildArg: ...}
):
def __init__(self, a: ChildArg):
self. a = a This could be straightforward to solve, just check if
class Arg1: ...
class Arg2: ...
class Model(
nn.Module,
PyTorchModelHubMixin,
coders={Arg1: ..., Arg2: ...}
):
def __init__(self, a: Arg1 | Arg2 | int):
self. a = a This case seems to be more challenging, since we can't tell for sure which decoder we should actually use. for etype in typing.get_args(expected_type):
try:
if etype in cls._hub_mixin_coders:
_, decoder = cls._hub_mixin_coders[etype]
out = decoder(value)
except:
# do smth
return out
return value This doesn't check for subclassing though.. need to think about it a little more |
Thanks for asking the right questions @gorold! I've digged a bit more into it and I think we should:
What do you think? |
That makes sense! I'll make a PR based on this |
Describe the bug
Using a union typehint in conjunction with custom encoder/decoder raises the below errors.
Reproduction
Logs
The text was updated successfully, but these errors were encountered: