[dask] Add type hints in Dask package #3866

jameslamb · 2021-01-26T23:10:37Z

As part of #3756 , this PR proposes type hints for the Dask package.

This PR attempts to document the current state of the package, and isn't a proposal about the final state for release 3.2.0. So, for example, right now X and y in .predict() have to be Dask collections, but I could see it being useful to allow other data types in the future. I'd like to defer that discussion to other issues / PRs.

Notes for Reviewers

I'm pushing this on the docs/jlamb branch because that branch already has readthedocs build enabled, and I want to see what effect, if any, this change has on the documentation
I want to add isinstance() checks on the types of arguments to fit() and predict() that should be Dask collections, as mentioned in Support DataTable in Dask #3830 (comment), but I'm proposing that that be a separate PR
It might be weird to see that the hint for **kwargs is not Dict[str, Any]. I've learned that the hint for * and ** stuff is expected to be the set of allowable types for the VALUES. https://www.python.org/dev/peps/pep-0484/#arbitrary-argument-lists-and-default-argument-values
The annotation Type[] in a hint says "this should be a class, not an instance". So df_class: Type[pd.DataFrame] means you pass pd.DataFrame or a sub-class, where df: pd.DataFrame means a constructed data frame is expected.

I ran mypy python-package/lightgbm/dask.py. There are still some mypy issues but I don't think any are a direct result of this PR and some come from LightGBM's dependencies. mypy can also get easily confused when you use mixin classes, which we do a lot in the Dask package and the sklearn classes it imports. So I'm not worrying too much about the mypy stuff below right now (other than the one I fixed in #3865 ).

mypy results for the curious

python-package/lightgbm/dask.py:15: error: Skipping analyzing 'numpy': found module but no type hints or library stubs
python-package/lightgbm/dask.py:15: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
python-package/lightgbm/dask.py:16: error: Skipping analyzing 'scipy.sparse': found module but no type hints or library stubs
python-package/lightgbm/dask.py:16: error: Skipping analyzing 'scipy': found module but no type hints or library stubs
python-package/lightgbm/dask.py:71: error: Incompatible return value type (got "Optional[int]", expected "int")
python-package/lightgbm/dask.py:94: error: Need type annotation for 'lightgbm_ports' (hint: "lightgbm_ports: Set[<type>] = ...")
python-package/lightgbm/dask.py:111: error: Value of type "Iterable[Union[Any, Any, Any, Any]]" is not indexable
python-package/lightgbm/dask.py:113: error: Value of type "Iterable[Union[Any, Any, Any, Any]]" is not indexable
python-package/lightgbm/dask.py:115: error: Value of type "Iterable[Union[Any, Any, Any, Any]]" is not indexable
python-package/lightgbm/dask.py:118: error: Value of type "Iterable[Union[Any, Any, Any, Any]]" is not indexable
python-package/lightgbm/dask.py:246: error: Name 'tree_learner' is not defined
python-package/lightgbm/dask.py:285: error: "Dict[str, Any]" has no attribute "status"
python-package/lightgbm/dask.py:286: error: Incompatible return value type (got "Dict[str, Any]", expected "LGBMModel")
python-package/lightgbm/dask.py:289: error: "Dict[str, Any]" has no attribute "key"; maybe "keys"?
python-package/lightgbm/dask.py:455: error: "_DaskLGBMModel" has no attribute "get_params"
python-package/lightgbm/dask.py:468: error: "_DaskLGBMModel" has no attribute "set_params"
python-package/lightgbm/dask.py:474: error: "_DaskLGBMModel" has no attribute "get_params"
python-package/lightgbm/dask.py:480: error: "_DaskLGBMModel" has no attribute "get_params"
python-package/lightgbm/dask.py:490: error: Signature of "fit" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:490: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:499: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMClassifier")
python-package/lightgbm/dask.py:509: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:515: error: Signature of "predict" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:515: error: Signature of "predict" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:526: error: Signature of "predict_proba" incompatible with supertype "LGBMClassifier"
python-package/lightgbm/dask.py:550: error: Signature of "fit" incompatible with supertype "LGBMRegressor"
python-package/lightgbm/dask.py:550: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:559: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMRegressor")
python-package/lightgbm/dask.py:569: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:575: error: Signature of "predict" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:598: error: Signature of "fit" incompatible with supertype "LGBMRanker"
python-package/lightgbm/dask.py:598: error: Signature of "fit" incompatible with supertype "LGBMModel"
python-package/lightgbm/dask.py:612: error: Incompatible return value type (got "_DaskLGBMModel", expected "DaskLGBMRanker")
python-package/lightgbm/dask.py:623: error: Item "None" of "Optional[str]" has no attribute "partition"
python-package/lightgbm/dask.py:629: error: Signature of "predict" incompatible with supertype "LGBMModel"

jameslamb · 2021-01-26T23:11:03Z

@ffineis if you have time, could you take a look at this diff too? Since you've recently worked with the internals of the Dask module

ffineis · 2021-01-27T00:08:30Z

@ffineis if you have time, could you take a look at this diff too? Since you've recently worked with the internals of the Dask module

Yep on it tonight

python-package/lightgbm/dask.py

ffineis · 2021-01-27T01:10:16Z

python-package/lightgbm/dask.py

+        y: _DaskCollection,
+        sample_weight: Optional[_DaskCollection] = None,
+        init_score: Optional[_DaskCollection] = None,
+        group: Optional[_1DArrayLike] = None,


Think this might be group: Optional[_DaskCollection], group array is still distributed at this point. Since group can be so much smaller than both X and y, I think supporting a locally-defined group input list or array is a noble cause, but this would need to be its own PR in which it was defined which parts of group get sent where to accompany the X and y parts.

If this comment stands, then _1DArrayLike can be removed at the top of this file.

oooooo ok! I misunderstood what was happening. Awesome, I'll look again and then change that hint.

I'll also write up a feature request for this. I think it's something that's non-breaking and additive, that could be done after 3.2.0. But like you said, since group is often fairly small, I think it could be a nice thing to be able to specify it as a list or lil numpy array.

ffineis · 2021-01-27T04:14:03Z

python-package/lightgbm/dask.py

+    params: Dict[str, Any],
+    model_factory: Type[LGBMModel],
+    sample_weight: Optional[_DaskCollection] = None,
+    group: Optional[_1DArrayLike] = None,


group: Optional[_DaskCollection] = None,

Would be great if the docs also reflected that sample_weight and group are, up to this point, still distributed vectors

I'll update the docs in this PR too, I think that's directly related to this scope. Thanks!

Thinking through this, I realized that with the way we do doc inheritance, it will actually be a bit tricky to override the docs for group.

I wrote up my thoughts in #3871, but I won't update the docs (except those on internal functions) in this PR

fixed the hint in https://docs.dask.org/en/latest/dataframe-api.html#dask.dataframe.to_csv

ffineis · 2021-01-27T04:21:19Z

python-package/lightgbm/dask.py

        model = model_factory(**self.get_params())
        self._copy_extra_params(self, model)
        return model

    @staticmethod
-    def _copy_extra_params(source, dest):
+    def _copy_extra_params(source: "_DaskLGBMModel", dest: "_DaskLGBMModel") -> None:


I think that dest could refer to both an LGBMModel or a _DaskLGBMModel, referring to line to 473 (in _to_local).

def _copy_extra_params(source: "_DaskLGBMModel", dest: Union["_DaskLGBMModel", LGBMModel]) -> None:

shoot, you're totally right. good eye

fixed in bfd9dc0

ffineis · 2021-01-27T04:34:11Z

python-package/lightgbm/dask.py

@@ -102,7 +107,7 @@ def _find_ports_for_workers(client: Client, worker_addresses: Iterable[str], loc
    return worker_ip_to_port


-def _concat(seq):
+def _concat(seq: Iterable[_DaskPart]) -> _DaskPart:


I get that technically any Iterable (tuple/list/np.array/...) could work for seq here, but what is the benefit to using Iterable and not List? I guess you would never use the List type hint for an input parameter (i.e. always prefer Iterable over List) unless the function called some list-specific method like .sort or mutability...?

I only raise this because I came across this S/O comment: https://stackoverflow.com/a/52827511/14480058, not sure what to think.

I didn't have any special reason for choosing Iterable instead of List. I was just looking at the code and didn't see any list-specific stuff so I thought it made sense to use the more general thing.

But since this is a totally-internally function, where we control the input, and since Iterable does weird stuff as that S/O post points out, I'll change this to List[]

fixed in bfd9dc0

ffineis · 2021-01-27T04:49:54Z

python-package/lightgbm/dask.py

-             dtype=np.float32, **kwargs):
+def _predict(
+    model: LGBMModel,
+    data: _DaskCollection,


The corresponding docstr for the data parameter reads dask array of shape = [n_samples, n_features] - consider changing it to data : dask array or dask DataFrame of shape = [n_samples, n_features]?

yup will do in this PR, thanks!

fixed in bfd9dc0

python-package/lightgbm/dask.py

StrikerRUS · 2021-01-27T22:48:53Z

@jameslamb
Could you please replace all occurrences of dask array with dask Array for the consistency with dask DataFrame, or dask Series?

jameslamb · 2021-01-27T22:58:27Z

@jameslamb
Could you please replace all occurrences of dask array with dask Array for the consistency with dask DataFrame, or dask Series?

oh sure, no problem. done in b20ac37

jameslamb · 2021-01-27T23:04:13Z

The type hints are currently shown in method signatures on the docs site:

from https://lightgbm.readthedocs.io/en/docs-jlamb/pythonapi/lightgbm.DaskLGBMClassifier.html#lightgbm.DaskLGBMClassifier.fit

I think this makes the method signature really hard to read. I'm going to try disabling this by setting autodoc_typehints = "none" in conf.py (https://stackoverflow.com/a/63560720/3986677).

StrikerRUS · 2021-01-27T23:34:14Z

I'm going to try disabling this by setting autodoc_typehints = "none" in conf.py

Don't forget to bump minimum required Sphinx version then

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_typehints

LightGBM/docs/conf.py

Line 77 in b4b1b75

needs_sphinx = '1.3' # Due to sphinx.ext.napoleon

jameslamb · 2021-01-28T04:06:13Z

I'm going to try disabling this by setting autodoc_typehints = "none" in conf.py

Don't forget to bump minimum required Sphinx version then

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_typehints

LightGBM/docs/conf.py

Line 77 in b4b1b75

needs_sphinx = '1.3' # Due to sphinx.ext.napoleon

Oh thanks! Didn't know about that. I though the version was only managed in

LightGBM/docs/requirements_rtd.txt

Line 2 in b4b1b75

sphinx >= 3.2.2

I'll update that. But happy to say the builds on RTD did pass, and the type hints were successfully hidden: https://lightgbm.readthedocs.io/en/docs-jlamb/pythonapi/lightgbm.DaskLGBMClassifier.html#lightgbm.DaskLGBMClassifier

StrikerRUS

Not sure my comments will be useful, but anyway: 🙂

python-package/lightgbm/dask.py

…jlamb

Co-authored-by: Nikita Titov <[email protected]>

StrikerRUS

LGTM except two comment below and one unresolved question from previous review: #3866 (comment).

Thanks a lot!

python-package/lightgbm/dask.py

Co-authored-by: Nikita Titov <[email protected]>

StrikerRUS · 2021-01-29T11:52:30Z

@jameslamb
Well, I see this branch is in sync with master and there is a Dask teardown error in regular job.
https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=8861&view=logs&j=c28dceab-947a-5848-c21f-eef3695e5f11&t=fa158246-17e2-53d4-5936-86070edbaacf

I'm not re-running the failed job to let you collect all details from log you may want to get familiar.

According to #3829 (comment) I'm going to increase timeout value for Dask tests.

jameslamb · 2021-01-29T15:55:36Z

@jameslamb
Well, I see this branch is in sync with master and there is a Dask teardown error in regular job.
https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=8861&view=logs&j=c28dceab-947a-5848-c21f-eef3695e5f11&t=fa158246-17e2-53d4-5936-86070edbaacf

I'm not re-running the failed job to let you collect all details from log you may want to get familiar.

According to #3829 (comment) I'm going to increase timeout value for Dask tests.

Thanks. I moved the discussion over to #3829 and re-opened it.

I pushed an empty commit here to re-run CI, since I don't have rights to re-run jobs in Azure.

jameslamb · 2021-01-29T17:58:41Z

Ok since #3866 (comment) has been moved to #3881 , I think this PR can be merged. Thanks for the reviews!

github-actions · 2023-08-24T01:27:58Z

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.

jameslamb added 6 commits January 25, 2021 18:17

add type hints in dask module

18d3bea

starting on asserts

e84f64e

merge master

444bdc8

remove unused code

67626a4

add hints for dtypes

eb1df4e

replace accidentally-removed docstrings

33963cd

jameslamb added the doc label Jan 26, 2021

jameslamb requested a review from StrikerRUS January 26, 2021 23:10

revert unrelated change

232e09b

jameslamb commented Jan 27, 2021

View reviewed changes

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved

Update python-package/lightgbm/dask.py

b8a3c45

ffineis suggested changes Jan 27, 2021

View reviewed changes

jameslamb added 3 commits January 27, 2021 14:56

empty commit

e889c0b

fix hints on group

bfd9dc0

Merge branch 'master' into docs/jlamb

3d3b75f

jameslamb mentioned this pull request Jan 27, 2021

[RFC] [dask] [docs] Override details in API docs that differ between dask and sklearn interfaces #3871

Closed

jameslamb commented Jan 27, 2021

View reviewed changes

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved

capitalize array

b20ac37

hide hints in signatures

7e41a97

jameslamb requested a review from Laurae2 as a code owner January 27, 2021 23:04

empty commit

b559bd0

sphinx version

a1512ee

jameslamb mentioned this pull request Jan 28, 2021

v3.2.0 release #3872

Merged

jameslamb mentioned this pull request Jan 28, 2021

[dask] Random failures in Dask tests during teardown #3829

Closed

StrikerRUS mentioned this pull request Jan 28, 2021

R-lintr failures at CI #3873

Closed

StrikerRUS reviewed Jan 28, 2021

View reviewed changes

jameslamb and others added 5 commits January 28, 2021 08:09

Merge branch 'docs/jlamb' of github.com:microsoft/LightGBM into docs/…

9107746

…jlamb

Merge branch 'master' into docs/jlamb

ef6f131

Apply suggestions from code review

b3c2d58

Co-authored-by: Nikita Titov <[email protected]>

fix hint for MatrixLike

8b19e04

merge master

32b2626

jameslamb requested review from chivee, henry0312 and wxchan as code owners January 28, 2021 14:58

Update python-package/lightgbm/dask.py

c1afc1e

Co-authored-by: Nikita Titov <[email protected]>

StrikerRUS approved these changes Jan 28, 2021

View reviewed changes

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved

python-package/lightgbm/dask.py Outdated Show resolved Hide resolved

jameslamb and others added 3 commits January 28, 2021 18:08

Apply suggestions from code review

66cee64

Co-authored-by: Nikita Titov <[email protected]>

Merge branch 'master' into docs/jlamb

58c09da

update docstring

7b3a0e5

StrikerRUS mentioned this pull request Jan 29, 2021

[dask][tests] increase client close timeout for Dask tests #3879

Merged

jameslamb mentioned this pull request Jan 29, 2021

[dask] Result shape from DaskLGBMClassifier.predict(pred_contrib=True) for CSC matrices is inconsistent with LGBMClassifier #3881

Closed

empty commit

a3393a5

jameslamb merged commit ea8e47e into master Jan 29, 2021

jameslamb mentioned this pull request Feb 16, 2021

[python-package] type hints in python package #3756

Open

12 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dask] Add type hints in Dask package #3866

[dask] Add type hints in Dask package #3866

jameslamb commented Jan 26, 2021

jameslamb commented Jan 26, 2021

ffineis commented Jan 27, 2021

ffineis Jan 27, 2021 •

edited

Loading

jameslamb Jan 27, 2021

ffineis Jan 27, 2021

jameslamb Jan 27, 2021

jameslamb Jan 27, 2021

jameslamb Jan 27, 2021

ffineis Jan 27, 2021

jameslamb Jan 27, 2021

jameslamb Jan 27, 2021

ffineis Jan 27, 2021

jameslamb Jan 27, 2021

jameslamb Jan 27, 2021

ffineis Jan 27, 2021 •

edited

Loading

jameslamb Jan 27, 2021

jameslamb Jan 27, 2021

StrikerRUS commented Jan 27, 2021 •

edited

Loading

jameslamb commented Jan 27, 2021

jameslamb commented Jan 27, 2021

StrikerRUS commented Jan 27, 2021

jameslamb commented Jan 28, 2021

StrikerRUS left a comment

StrikerRUS left a comment

StrikerRUS commented Jan 29, 2021

jameslamb commented Jan 29, 2021

jameslamb commented Jan 29, 2021

github-actions bot commented Aug 24, 2023

[dask] Add type hints in Dask package #3866

[dask] Add type hints in Dask package #3866

Conversation

jameslamb commented Jan 26, 2021

Notes for Reviewers

jameslamb commented Jan 26, 2021

ffineis commented Jan 27, 2021

ffineis Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffineis Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS commented Jan 27, 2021 • edited Loading

jameslamb commented Jan 27, 2021

jameslamb commented Jan 27, 2021

StrikerRUS commented Jan 27, 2021

jameslamb commented Jan 28, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

StrikerRUS commented Jan 29, 2021

jameslamb commented Jan 29, 2021

jameslamb commented Jan 29, 2021

github-actions bot commented Aug 24, 2023

ffineis Jan 27, 2021 •

edited

Loading

ffineis Jan 27, 2021 •

edited

Loading

StrikerRUS commented Jan 27, 2021 •

edited

Loading