-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make any callable compatible with (*args: Any, **kwargs: Any) #11203
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
def f(self, *args: Any, **kwargs: Any) -> int: ... | ||
|
||
class B(A): | ||
def f(self, x: int) -> int: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is very handy in some cases, #5876 has a handful of them. But, it is important to remember that this is not type-safe.
from typing import Any
class A(object):
def f(self, *args: Any, **kwargs: Any) -> int: ...
class B(A):
def f(self, x: int) -> int: ...
def test(arg: A):
arg.f(x=0, kw1=1, kw2=2)
test(A()) # ok
test(B()) # mypy is ok with that, but raises TypeError in real-life
It breaks Liskov's law.
Maybe we can somehow limit the scope of this new rule? For example:
- It is fine for decorator example from Make any callable compatible with (*args: Any, **kwargs: Any)? #5876
- It is fine for
Protocol
check (when protocol defines less-or-equally broad signature):Procol.method(self, arg: int)
-> is ok withImpl.method(self, *args, **kwargs)
- It is fine for callbacks
- Maybe something else
- It is not fine for subclassing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the ask is explicitly for something that is not type safe. The check when "protocol defines less-or-equally broad signature" should already work. The decorator example from #5876 is a little interesting; this PR doesn't actually change inference there.
#5876 is probably a top five most popular mypy issue, so ideally something would be done. Maybe the answer is another flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think we should just go ahead with this.
This comment has been minimized.
This comment has been minimized.
It looks like |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: ignite (https://github.com/pytorch/ignite)
+ ignite/distributed/comp_models/xla.py:110: error: Unused "type: ignore" comment
+ ignite/distributed/comp_models/native.py:350: error: Unused "type: ignore" comment
+ ignite/distributed/comp_models/horovod.py:129: error: Unused "type: ignore" comment
spark (https://github.com/apache/spark)
+ python/pyspark/sql/types.py:142: error: Unused "type: ignore" comment
ibis (https://github.com/ibis-project/ibis)
- ibis/backends/pandas/__init__.py:37: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/pandas/__init__.py:37: note: Superclass:
- ibis/backends/pandas/__init__.py:37: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/pandas/__init__.py:37: note: Subclass:
- ibis/backends/pandas/__init__.py:37: note: def do_connect(self, dictionary: MutableMapping[str, Any]) -> None
- ibis/backends/datafusion/__init__.py:44: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/datafusion/__init__.py:44: note: Superclass:
- ibis/backends/datafusion/__init__.py:44: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/datafusion/__init__.py:44: note: Subclass:
- ibis/backends/datafusion/__init__.py:44: note: def do_connect(self, config: Union[Mapping[str, Union[str, Path]], Any]) -> None
- ibis/backends/dask/__init__.py:34: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/dask/__init__.py:34: note: Superclass:
- ibis/backends/dask/__init__.py:34: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/dask/__init__.py:34: note: Subclass:
- ibis/backends/dask/__init__.py:34: note: def do_connect(self, dictionary: MutableMapping[str, Any]) -> None
- ibis/backends/clickhouse/__init__.py:41: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/clickhouse/__init__.py:41: note: Superclass:
- ibis/backends/clickhouse/__init__.py:41: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/clickhouse/__init__.py:41: note: Subclass:
- ibis/backends/clickhouse/__init__.py:41: note: def do_connect(self, host: str = ..., port: int = ..., database: str = ..., user: str = ..., password: str = ..., client_name: str = ..., compression: Union[bool, Literal['lz4', 'lz4hc', 'quicklz', 'zstd']] = ..., **kwargs: Any) -> Any
- ibis/backends/base/sql/alchemy/__init__.py:93: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/base/sql/alchemy/__init__.py:93: note: Superclass:
- ibis/backends/base/sql/alchemy/__init__.py:93: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/base/sql/alchemy/__init__.py:93: note: Subclass:
- ibis/backends/base/sql/alchemy/__init__.py:93: note: def do_connect(self, con: Any) -> None
- ibis/backends/pyspark/__init__.py:91: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/pyspark/__init__.py:91: note: Superclass:
- ibis/backends/pyspark/__init__.py:91: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/pyspark/__init__.py:91: note: Subclass:
- ibis/backends/pyspark/__init__.py:91: note: def do_connect(self, session: Any) -> None
- ibis/backends/impala/__init__.py:195: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/impala/__init__.py:195: note: Superclass:
- ibis/backends/impala/__init__.py:195: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/impala/__init__.py:195: note: Subclass:
- ibis/backends/impala/__init__.py:195: note: def do_connect(self, host: str = ..., port: int = ..., database: str = ..., timeout: int = ..., use_ssl: bool = ..., ca_cert: Union[str, Path, None] = ..., user: Optional[str] = ..., password: Optional[str] = ..., auth_mechanism: Literal['NOSASL', 'PLAIN', 'GSSAPI', 'LDAP'] = ..., kerberos_service_name: str = ..., pool_size: int = ..., hdfs_client: Optional[Any] = ...) -> Any
- ibis/backends/sqlite/__init__.py:52: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/sqlite/__init__.py:52: note: Superclass:
- ibis/backends/sqlite/__init__.py:52: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/sqlite/__init__.py:52: note: Subclass:
- ibis/backends/sqlite/__init__.py:52: note: def do_connect(self, path: Union[str, Path, None] = ...) -> None
- ibis/backends/mysql/__init__.py:24: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/mysql/__init__.py:24: note: Superclass:
- ibis/backends/mysql/__init__.py:24: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/mysql/__init__.py:24: note: Subclass:
- ibis/backends/mysql/__init__.py:24: note: def do_connect(self, host: str = ..., user: Optional[str] = ..., password: Optional[str] = ..., port: int = ..., database: Optional[str] = ..., url: Optional[str] = ..., driver: Literal['pymysql'] = ...) -> None
- ibis/backends/duckdb/__init__.py:100: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/duckdb/__init__.py:100: note: Superclass:
- ibis/backends/duckdb/__init__.py:100: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/duckdb/__init__.py:100: note: Subclass:
- ibis/backends/duckdb/__init__.py:100: note: def do_connect(self, path: Union[str, Path] = ..., read_only: bool = ...) -> None
- ibis/backends/postgres/__init__.py:22: error: Signature of "do_connect" incompatible with supertype "BaseBackend"
- ibis/backends/postgres/__init__.py:22: note: Superclass:
- ibis/backends/postgres/__init__.py:22: note: def do_connect(self, *args: Any, **kwargs: Any) -> None
- ibis/backends/postgres/__init__.py:22: note: Subclass:
- ibis/backends/postgres/__init__.py:22: note: def do_connect(self, host: str = ..., user: Optional[str] = ..., password: Optional[str] = ..., port: int = ..., database: Optional[str] = ..., url: Optional[str] = ..., driver: Literal['psycopg2'] = ...) -> None
discord.py (https://github.com/Rapptz/discord.py)
- discord/enums.py:154: error: Signature of "__call__" incompatible with supertype "type"
- discord/enums.py:154: note: Superclass:
- discord/enums.py:154: note: def __call__(self, *args: Any, **kwds: Any) -> Any
- discord/enums.py:154: note: Subclass:
- discord/enums.py:154: note: def __call__(cls, value: str) -> Any
|
Obviously I am happy that this issue was resolved, since it's such a popular request but it bothers me that such an obvious violation of LSP is now apparently ok. The primary use cases for using something like this in subclassing comes from the pytorch world where the base class module might define a function like this class Module(object):
def forward(self, *args: Any, **kwargs: Any) -> Union[Tensor, Dict[str, Any]]:
raise NotImplementedError What the above means is that it's just a contract, subclasses should technically be able to call forward with any arguments. class SummarizationModule(Module)
def forward(self, src: torch.Tensor, tgt: torch.Tensor) -> Tensor:
return torch.zeros(2, 3) It does not violate LSP technically even if it does because you will never call the base class forward anyway since it is not implemented. In compiled languages this won't be a problem since the base forward acts like a pure virtual function and the language does not even allow you to call it. But in the general case if the forward is not performing the role of a pure virtual function then allowing subclasses to call specific arguments is an obvious violation of LSP. Take this obviously contrived example class Base(object):
def count(self, *args, **kwargs):
return len(args) + len(kwargs)
class Derived(Base):
def count(self, a: int): # This should never be allowed since it's not polymorphic, just use a different name
return 1 Right now python afaik gives us no mechanism to distinguish these two cases, and I can understand why this was accepted, but it is stil problematic. |
I think cases like yours could be annotated as |
Yeah, I agree with @hauntsaninja just use |
Resolves #5876