Skip to content

Conversation

@phobson
Copy link
Contributor

@phobson phobson commented Mar 29, 2022

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

This mostly ignore remaining complex typing situations to get mypy passing so that we can enable it and start enforcing going forward.

Builds off of #8706

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@phobson phobson requested a review from crusaderky March 29, 2022 07:24
@github-actions github-actions bot added array dataframe documentation Improve or add to documentation labels Mar 29, 2022
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review - more later

@mrocklin
Copy link
Member

Before we start requiring mypy in CI I think that we should raise that as a discussion in https://github.com/dask/community (unless there was already a decision on this).

@phobson
Copy link
Contributor Author

phobson commented Mar 29, 2022

@crusaderky thanks so much for these detailed and knowledgeable comments. I'll be working through them today, trying to answer the questions you've asked ASAP following the standup.

@phobson phobson marked this pull request as draft March 29, 2022 16:07
@crusaderky
Copy link
Collaborator

Before we start requiring mypy in CI I think that we should raise that as a discussion in https://github.com/dask/community (unless there was already a decision on this).

I believe there was consensus at the maintainers weekly meeting

@crusaderky
Copy link
Collaborator

@crusaderky
Copy link
Collaborator

I'm a bit swarmed right now with PyConDE and Easter coming up, so I'm afraid I won't be able to continue the review before April 19th

@ian-r-rose ian-r-rose self-requested a review April 5, 2022 14:25
Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @phobson !

@github-actions github-actions bot added the io label Apr 14, 2022
@crusaderky
Copy link
Collaborator

Final code review at phobson#2

@phobson phobson marked this pull request as ready for review April 21, 2022 20:41
@phobson phobson requested a review from ian-r-rose April 21, 2022 21:44
@phobson
Copy link
Contributor Author

phobson commented Apr 21, 2022

Failed Mac build(s) appear to be related to GHA issues.

@crusaderky crusaderky merged commit 099909a into dask:main Apr 21, 2022
@crusaderky
Copy link
Collaborator

Thank you!

@ian-r-rose
Copy link
Collaborator

Woo! Thanks @phobson and @crusaderky!

@phobson phobson deleted the mypy branch April 22, 2022 00:54
@phobson
Copy link
Contributor Author

phobson commented Apr 22, 2022

Thanks @crusaderky and @ian-r-rose for the patience and guidance!

jsignell pushed a commit that referenced this pull request May 6, 2022
[PEP 544](https://www.python.org/dev/peps/pep-0544/) introduces the `Protocol` class to the `typing` module in Python 3.8 (the soon be the minimum supported version, dask/community#213). Writing new Dask collections for [dask-awkward](https://github.com/ContinuumIO/dask-awkward/) has had me thinking about working on a `DaskCollection` protocol. I imagine the benefits to be:

- usage with static type checkers
  - other activity in this area at
    - #8295 
    - #8706 
    -  #8854
  - Python supporting IDEs take advantage of typing
- self-documenting; some improvements to [the custom collections page](https://docs.dask.org/en/latest/custom-collections.html) of the docs. The protocol docs can be autogenerated and added to that page.
- purely opt-in feature

The `typing.runtime_checkable` decorator allows use of `isinstance(x, DaskCollection)` in any code base
that uses Dask collections; for example:

```python
>>> from dask.typing import DaskCollection
>>> import dask.array as da
>>> x = da.zeros((10, 3))
>>> isinstance(x, DaskCollection)
True
```
(though this is an order of magnitude slower than `dask.base.is_dask_collection` which only checks for `x.__dask_graph__() is not None`; static typing checking & built-in interface documentation are the core benefits IMO)

Something else that came up in the brief discussion on a call last week was having `{Scheduler,Worker,Nanny}Plugin` protocols in `distributed`; and perhaps those are better places to start introducing protocols to Dask since on the user side typically more folks would write plugins than new collections.
erayaslan pushed a commit to erayaslan/dask that referenced this pull request May 12, 2022
[PEP 544](https://www.python.org/dev/peps/pep-0544/) introduces the `Protocol` class to the `typing` module in Python 3.8 (the soon be the minimum supported version, dask/community#213). Writing new Dask collections for [dask-awkward](https://github.com/ContinuumIO/dask-awkward/) has had me thinking about working on a `DaskCollection` protocol. I imagine the benefits to be:

- usage with static type checkers
  - other activity in this area at
    - dask#8295 
    - dask#8706 
    -  dask#8854
  - Python supporting IDEs take advantage of typing
- self-documenting; some improvements to [the custom collections page](https://docs.dask.org/en/latest/custom-collections.html) of the docs. The protocol docs can be autogenerated and added to that page.
- purely opt-in feature

The `typing.runtime_checkable` decorator allows use of `isinstance(x, DaskCollection)` in any code base
that uses Dask collections; for example:

```python
>>> from dask.typing import DaskCollection
>>> import dask.array as da
>>> x = da.zeros((10, 3))
>>> isinstance(x, DaskCollection)
True
```
(though this is an order of magnitude slower than `dask.base.is_dask_collection` which only checks for `x.__dask_graph__() is not None`; static typing checking & built-in interface documentation are the core benefits IMO)

Something else that came up in the brief discussion on a call last week was having `{Scheduler,Worker,Nanny}Plugin` protocols in `distributed`; and perhaps those are better places to start introducing protocols to Dask since on the user side typically more folks would write plugins than new collections.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

array dataframe documentation Improve or add to documentation io

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants