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

[python-package] fix mypy error about dask._HostWorkers.__eq__() #5782

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3756.
Contributes to #3867.

Fixes the following mypy error.

python-package/lightgbm/dask.py:47: error: Argument 1 of "__eq__" is incompatible with supertype "object"; supertype defines the argument type as "object"  [override]
python-package/lightgbm/dask.py:47: note: This violates the Liskov substitution principle
python-package/lightgbm/dask.py:47: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
python-package/lightgbm/dask.py:47: note: It is recommended for "__eq__" to work with arbitrary objects, for example:
python-package/lightgbm/dask.py:47: note:     def __eq__(self, other: object) -> bool:
python-package/lightgbm/dask.py:47: note:         if not isinstance(other, _HostWorkers):
python-package/lightgbm/dask.py:47: note:             return NotImplemented
python-package/lightgbm/dask.py:47: note:         return <logic to compare two _HostWorkers instances>

@jmoralez
Copy link
Collaborator

jmoralez commented Mar 15, 2023

I just learned about typing.NamedTuple. We can have the _HostWorkers class inherit from that and we wouldn't need the __eq__, it works the same way as dataclass with some slight differences (stackoverflow reference).

@jameslamb
Copy link
Collaborator Author

I mentioned that in the description of #3756

I chose to use a regular Python class instead of dataclasses.dataclass to preserve support for Python 3.6 (#5765 (comment)), and chose to avoid typing.NamedTuple (https://stackoverflow.com/a/50038614/3986677) to reduce the risk of incompatibilities caused by future updates to typing.

I'm nervous about introducing a dependency on typing for runtime (not just type-checking) behavior.

@jameslamb
Copy link
Collaborator Author

sorry,I meant #5766

@jmoralez
Copy link
Collaborator

Oh, sorry

@jameslamb
Copy link
Collaborator Author

jameslamb commented Mar 15, 2023

No problem at all! I know having to also have this __eq__ feels like a lot just for slightly better type-checking on two small variables. In a future where we drop Python 3.6 support, this definitely should be converted to a dataclasses.dataclass (but to be clear, I still agree with you that something bigger needs to happen to cause us to drop Python 3.6 support).

@jameslamb jameslamb merged commit 125ca03 into master Mar 15, 2023
@jameslamb jameslamb deleted the ci/mypy-dask-hints branch March 15, 2023 03:32
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed.
To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues
including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants