omni-us
Callable protocols inheritance broken
🐛 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
Thank you for reporting!
Indeed the intention was to not consider private methods when checking whether a protocol is implemented. This is here https://github.com/omni-us/jsonargparse/blob/2825a7357908e324b48e9512b721158c028310a3/jsonargparse/_typehints.py#L1096-L1097
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?
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.
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: https://github.com/omni-us/jsonargparse/blob/2825a7357908e324b48e9512b721158c028310a3/jsonargparse/_typehints.py#L1096-L1097
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.
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.