Open MiguelMonteiro opened 1 week ago
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?
Thanks, yes sure I will fix it
what is the reason for excluding private methods though?
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.
🐛 Bug report
Callable protocols (implement
__call__
) seems to be parser incorrectly in comparison to "normal" protocols.To reproduce
Example that works
Example that does not work:
The second example raises the following error:
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