[python-package] fix some warnings from mypy #3891
Conversation
tara-jawahar
requested review from
chivee,
henry0312,
jameslamb,
StrikerRUS and
wxchan
as code owners
|
|
There was a problem hiding this comment.
Thanks very much for taking the time to contribute!
I left a few suggestions, most of which can be accepted directly in the browser.
After your next commit, can you please add a comment on this pull request with the output of running mypy --ignore-missing-imports python-package/?.
Also I hope you don't mind, but I've changed the title of this pull request to something that will be easy to understand in a changelog. We automatically generate changelogs from PR titles in this project: https://github.com/microsoft/LightGBM/releases/tag/untagged-e810435e8b97e053e761
python-package/lightgbm/dask.py
Outdated
| @@ -27,7 +27,7 @@ | |||
| _PredictionDtype = Union[Type[np.float32], Type[np.float64], Type[np.int32], Type[np.int64]] | |||
|
|
|||
|
|
|||
| def _find_open_port(worker_ip: str, local_listen_port: int, ports_to_skip: Iterable[int]) -> int: | |||
| def _find_open_port(worker_ip: str, local_listen_port: int, ports_to_skip: Iterable[int]) -> Optional[int]: | |||
There was a problem hiding this comment.
Is this to fix this warning?
python-package/lightgbm/dask.py:71: error: Incompatible return value type (got "Optional[int]", expected "int")
I don't think this is the right fix. I think that mypy is (rightly) telling us that I wrote _find_open_port() in a way that makes it a little confusing to reason about the return type of out_port. Can you please change this back to int, and remove out_port = None on line 54? I think that should fix that warning.
There was a problem hiding this comment.
I saw this conversation was marked resolved, but you did not implement what recommended.
Can you please change this back to int, and remove out_port = None on line 54, and see if it resolves the warning? If you disagree with that, can you please explain why?
jameslamb
changed the title
tara-jawahar
force-pushed
the
mypy-errors
branch
from
d1db63e
to
8232887
Compare
|
Thank you for the comments! Output of mypy --ignore-missing-imports python-package/ : python-package/lightgbm/compat.py:14: error: Name 'pd_Series' already defined (possibly by an import) |
python-package/lightgbm/dask.py
Outdated
| @@ -564,7 +564,7 @@ def fit( | |||
| ) | |||
|
|
|||
| _base_doc = LGBMRegressor.fit.__doc__ | |||
| _before_init_score, _init_score, _after_init_score = _base_doc.partition('init_score :') | |||
| _before_init_score, _init_score, _after_init_score = _base_doc.partition('init_score :') # type: ignore | |||
There was a problem hiding this comment.
ah, do these calls fix this?
python-package/lightgbm/dask.py:567: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py
Outdated
| @@ -27,7 +27,7 @@ | |||
| _PredictionDtype = Union[Type[np.float32], Type[np.float64], Type[np.int32], Type[np.int64]] | |||
|
|
|||
|
|
|||
| def _find_open_port(worker_ip: str, local_listen_port: int, ports_to_skip: Iterable[int]) -> int: | |||
| def _find_open_port(worker_ip: str, local_listen_port: int, ports_to_skip: Iterable[int]) -> Optional[int]: | |||
There was a problem hiding this comment.
I saw this conversation was marked resolved, but you did not implement what recommended.
Can you please change this back to int, and remove out_port = None on line 54, and see if it resolves the warning? If you disagree with that, can you please explain why?
|
@tara-jawahar sorry for the inconvenience, but can you please merge the latest changes on the |
|
@jameslamb I selectively ignored some mypy errors based on your previous comments - let me know how they look! However, I was unable to fix the "Signature of [function in dask.py] incompatible with supertype LGBMClassifier/LGBMModel" errors. |
|
Thanks very much! Can you please merge the latest changes from the |
tara-jawahar
requested review from
btrotta,
guolinke,
huanzhang12 and
Laurae2
as code owners
|
Hi James, sorry I missed your comment about that before! Thanks for pointing it out. Does it look like I did it correctly? |
no problem! The merging does not look quite correct. We shouldn't see all these other commits from previous PRs. I'd be happy to fix it if you'd like. Can I push to your branch? |
jameslamb
removed request for
henry0312,
chivee,
huanzhang12,
Laurae2,
wxchan,
guolinke and
btrotta
|
Yes, go ahead! |
|
Thanks! I just fixed this. In case you're curious, these are the commands I ran git clone git@github.com:tara-jawahar/LightGBM.git tara-lgb
cd tara-lgb
git remote add upstream git@github.com:microsoft/LightGBM.git
git pull upstream master
git fetch origin mypy-errors
git checkout mypy-errors
git merge master
git push origin mypy-errors |
python-package/lightgbm/sklearn.py
Outdated
| _base_doc = (_base_doc[:_base_doc.find('group :')] | ||
| + _base_doc[_base_doc.find('eval_set :'):]) | ||
| _base_doc = (_base_doc[:_base_doc.find('group :')] # type: ignore | ||
| + _base_doc[_base_doc.find('eval_set :'):]) # type: ignore |
There was a problem hiding this comment.
what issue did these # type: ignore comments on _base_doc fix? I ran mypy --ignore-missing-imports python-package/ on current master, and I don't see any notes related to these lines
python-package/lightgbm/dask.py:97: error: Incompatible return value type (got "Optional[int]", expected "int")
python-package/lightgbm/dask.py:120: error: Need type annotation for 'lightgbm_ports' (hint: "lightgbm_ports: Set[<type>] = ...")
python-package/lightgbm/dask.py:309: error: "Dict[str, Any]" has no attribute "status"
python-package/lightgbm/dask.py:310: error: Incompatible return value type (got "Dict[str, Any]", expected "LGBMModel")
python-package/lightgbm/dask.py:313: error: "Dict[str, Any]" has no attribute "key"; maybe "keys"?
python-package/lightgbm/dask.py:478: error: "_DaskLGBMModel" has no attribute "_other_params"
python-package/lightgbm/dask.py:496: error: "_DaskLGBMModel" has no attribute "get_params"
python-package/lightgbm/dask.py:510: error: "_DaskLGBMModel" has no attribute "set_params"
python-package/lightgbm/dask.py:516: error: "_DaskLGBMModel" has no attribute "get_params"
python-package/lightgbm/dask.py:525: error: Item "_DaskLGBMModel" of "Union[_DaskLGBMModel, LGBMModel]" has no attribute "get_params"
python-package/lightgbm/dask.py:587: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:602: error: Signature of "fit" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:602: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:610: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMClassifier")
python-package/lightgbm/dask.py:636: error: Signature of "predict" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:636: error: Signature of "predict" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:654: error: Signature of "predict_proba" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:738: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:753: error: Signature of "fit" incompatible with supertype "LGBMRegressor"
python-package/lightgbm/dask.py:753: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:761: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMRegressor")
python-package/lightgbm/dask.py:787: error: Signature of "predict" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:870: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:885: error: Signature of "fit" incompatible with supertype "LGBMRanker"
python-package/lightgbm/dask.py:885: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:898: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMRanker")
python-package/lightgbm/dask.py:928: error: Signature of "predict" incompatible with supertype "LGBMModel"
There was a problem hiding this comment.
these are the issues I was ignoring:
for this block of code:
Let me know if that's incorrect and I can take it out!
There was a problem hiding this comment.
haha oh! I was looking the Dask module but clearly these changes and the warning come from sklearn.py. My mistake, sorry. Thanks for the explanation!
Co-authored-by: James Lamb <jaylamb20@gmail.com>
|
Done! Thank you so much for the detailed comments - I'll keep all of these in mind. |
|
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. |
Fixed some mypy type warnings in setup.py, dask.py, and sklearn.py