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.

Sign up for GitHub

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

Jump to bottom

Callable protocols inheritance broken #595

Open
MiguelMonteiro opened this issue Oct 14, 2024 · 7 comments · May be fixed by #599
Open

Callable protocols inheritance broken #595

MiguelMonteiro opened this issue Oct 14, 2024 · 7 comments · May be fixed by #599
Labels
bug Something isn't working

Comments

@MiguelMonteiro
Copy link
Contributor

🐛 Bug report

Callable protocols (implement __call__) seems to be parser incorrectly in comparison to "normal" protocols.

To reproduce

Example that works

from dataclasses import dataclass
from typing import Protocol

from jsonargparse import ArgumentParser


class BaseGood(Protocol):
    def predict(self, a: int) -> int: ...


@dataclass
class SpecializedGood(BaseGood):
    number: int = 5

    def predict(self, a: int) -> int:
        return a + 5


def main() -> None:
    parser = ArgumentParser(description="CLI for training models", exit_on_error=False)
    parser.add_argument("--cfg", type=BaseGood)
    args = parser.parse_args(["--cfg=SpecializedGood", "--cfg.number=5"])
    print(args)
    
if __name__ == "__main__":
    main()

Example that does not work:

class BaseBad(Protocol):
    def __call__(self, a: int) -> int: ...


@dataclass
class SpecializedBad(BaseBad):
    number: int = 5

    def __call__(self, a: int) -> int:
        return a + 5


def main() -> None:
    parser = ArgumentParser(description="CLI for training models", exit_on_error=False)
    parser.add_argument("--cfg", type=BaseBad)
    args = parser.parse_args(["--cfg=SpecializedBad", "--cfg.number=5"])
    print(args)
    
if __name__ == "__main__":
    main()

The second example raises the following error:

Import path SpecializedBad does not correspond to a subclass of BaseBad

Seems like the parser does not match the inherited protocol signature to the base if the protocol methods are private (?).

Expected behavior

I would expect the second example to work like the first.

Environment

  • jsonargparse version: 4.33.1
  • Python version: 3.10
  • How jsonargparse was installed: pip
  • OS: MacOS
@MiguelMonteiro MiguelMonteiro added the bug Something isn't working label Oct 14, 2024
@mauvilsa
Copy link
Member

Thank you for reporting!

Indeed the intention was to not consider private methods when checking whether a protocol is implemented. This is here

jsonargparse/jsonargparse/_typehints.py

Lines 1096 to 1097 in 2825a73

if name.startswith("_"):
continue

Though, it was not intended to exclude __call__, just it wasn't noticed. Would be an easy fix. Just change the condition to

if name.startswith("_") and name != "__call__":

Do you want to contribute the fix?

@MiguelMonteiro
Copy link
Contributor Author

Thanks, yes sure I will fix it

@MiguelMonteiro
Copy link
Contributor Author

what is the reason for excluding private methods though?

@mauvilsa
Copy link
Member

what is the reason for excluding private methods though?

The purpose of a Protocol is to define the public interface that a family of classes should adhere to. Even though it might be possible to add a private method to a protocol, that would be discouraged and considered a bad practice. I would not add support for things that are known to be bad practice.

But note that dunder methods, such as __call__ are not private methods. Which is why it does make sense to consider for Protocols. And it is not just __call__. There are other dunder methods that would also make sense for protocols. Though there are also dunder methods that don't make sense. For example, __len__ might make sense, whereas __del__ wouldn't make sense. A question could be, do we add now support for more than __call__? I would prefer to not simply allow any dunder method, since for the cases that doesn't make sense, better to not support it. Could also be that we only add support for __call__ now and only extend when someone requests it, which would mean that an actual use case for it has been found.

@MiguelMonteiro
Copy link
Contributor Author

I see your point, maybe we can have a list of allowed dunder methods which can be expanded when needed?

However, I do think it might be out of the scope of the parser to try to enforce/disallow discouraged practices. Unless there is consistent rule about which dunder methods are allowed, it will be hard to predict the expected behavior of the parser. Is there any other downside to removing the check:

jsonargparse/jsonargparse/_typehints.py

Lines 1096 to 1097 in 2825a73

if name.startswith("_"):
continue

@mauvilsa
Copy link
Member

The good practices is a design decision, similar to the one for lightning. Also in practical terms, (not specific to this case) allowing unexpected uses, can mean that people implement and rely on behaviors which were not really intended, and add to the maintainance cost of the library. For sure allowing private methods will not happen. Dunder methods is something to analyze.

@MiguelMonteiro
Copy link
Contributor Author

Ok, makes sense. I created a PR which adds a set of exceptions for dunder methods and added __call__ to said set. This can be expanded in the future if needed.

@mauvilsa mauvilsa linked a pull request Oct 16, 2024 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix/callable protocols 2 MiguelMonteiro/jsonargparse
2 participants
@mauvilsa @MiguelMonteiro