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

Callable protocols not being parsed correctly #636

Closed
MiguelMonteiro opened this issue Nov 28, 2024 · 8 comments · Fixed by #637 or #645
Closed

Callable protocols not being parsed correctly #636

MiguelMonteiro opened this issue Nov 28, 2024 · 8 comments · Fixed by #637 or #645
Labels
bug Something isn't working

Comments

@MiguelMonteiro
Copy link
Contributor

🐛 Bug report

Even though bugs related with calling protocol inheritance were resolved in this PR it seem there are still some issues relating to parsing callable protocols.

To reproduce


2. Manually constructing a parser

from typing import Protocol, Sequence

from jsonargparse import ArgumentParser


class CallableInterface(Protocol):
    def __call__(self, items: list[float]) -> list[float]: ...


class ImplementsCallableInterface1:
    def __init__(self, batch_size: int):
        self.batch_size = batch_size

    def __call__(self, items: list[float]) -> list[float]:
        return items


def main(input_args: Sequence[str] | None = None) -> None:
    # parser and config
    parser = ArgumentParser(description="CLI for training models", exit_on_error=False)
    parser.add_argument("--c", type=CallableInterface)
    args = parser.parse_args(input_args)
    print(args)


if __name__ == "__main__":
    main(["--c", "CallableInterface"])

results in the following error message:

argparse.ArgumentError: Parser key "c":
  Invalid or unsupported input: class=<class 'str'>, method_or_property=__call__

and asking for the help string gives:

argparse.ArgumentError: --c.help: Class "CallableInterface" is not a subclass of CallableInterface

Note that in the tests this exact interface, ImplementsCallableInterface1 is recognized as being a valid instantiation of the CallableInterface.

Expected behavior

The parser should recolonize CallableInterface and be able to instantiate it.

Environment

  • jsonargparse version: 4.34.0
  • Python version: 3.11
  • How jsonargparse was installed: uv
  • OS: macOS
@MiguelMonteiro MiguelMonteiro added the bug Something isn't working label Nov 28, 2024
@mauvilsa
Copy link
Member

Thank you for reporting!

Some comments:

  • I suppose you meant main(["--c", "ImplementsCallableInterface1"]), instead of CallableInterface.
  • If a protocol is given, such as CallableInterface, technically it is a class that implements the protocol. But it doesn't make sense, since protocols are supposed to not have implementations, and can't even be instantiated. So in this case the parser should fail.
  • You provided only a name, instead of a full import path. With normal class inheritance it is trivial to get all the subclasses of the base class, and thus resolve a simple name to its full import path. But given a protocol, there is no easy way to know which classes implement it. This would require loop through all imported modules a check all classes. This would be prohibitively slow, so this will not be supported. This means that for the example above to work, the arguments need to be --c=__main__.ImplementsCallableInterface1 --c.batch_size=1.

@mauvilsa
Copy link
Member

These problems should be fixed with #637.

@MiguelMonteiro
Copy link
Contributor Author

Hello, thanks for the feedback, According to the documentation implementing protocols by explicitly inheriting from the protocol as the base class is allowed https://typing.readthedocs.io/en/latest/spec/protocol.html#explicitly-declaring-implementation
I understand that checking all classes and functions to see which implement the protocol in not feasible but for classes which derive explicitly it's maybe not to hard to fix?

@mauvilsa
Copy link
Member

mauvilsa commented Dec 9, 2024

Yes, for normal inheritance from a protocol it could be supported.

@MiguelMonteiro
Copy link
Contributor Author

Thank you!

@MiguelMonteiro
Copy link
Contributor Author

thanks for the fix on #637 it is working now, the only thing that does not work is the help string where the subclass is still not recognized.

@MiguelMonteiro
Copy link
Contributor Author

I found the source of the issue. When getting the help string the following check is performed this fails in is_subclass because TypeError: Instance and class checks can only be used with @runtime_checkable protocols however the error message gets printed as something different TypeError: --c.help: Class "ImplementsCallableInterface1" is not a subclass of CallableInterface

@mauvilsa
Copy link
Member

I found the source of the issue. When getting the help string the following check is performed this fails in is_subclass because TypeError: Instance and class checks can only be used with @runtime_checkable protocols however the error message gets printed as something different TypeError: --c.help: Class "ImplementsCallableInterface1" is not a subclass of CallableInterface

This should be fixed with #645. Also I added a test in which a class inherits from a protocol, resolving the name to its class path.

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