dask icon indicating copy to clipboard operation
dask copied to clipboard

Add array annotations

Open jason-trinidad opened this issue 1 year ago • 5 comments

Adding annotations to array functions. Primary motivation for this PR is improving compatibility with projects that use PyRight (a popular type checker for Python).

This PR adds type annotations for:

  • from_array
  • rechunk
  • map_blocks
  • map_overlap

In particular, adding annotations to map_blocks yields a return type of class <method> for the method. @sam-goodwin if possible, please advise on whether this solves your problem with Pyright!

I have two main questions for this PR:

  1. Approach Much of the existing code was not written with types in mind. Adding types causes a lot of mypy errors. I've chosen to ignore those errors. Do admins think this is a bad idea?

Making the code type-safe would involve refactoring these functions. For example it's common to see:

  1. Variable is declared as a parameter with an expected type
  2. Variable is reassigned to an object of a different type for use throughout the function

This pattern causes mypy some distress!

For this PR, I decided to add type annotations, and ignore mypy warnings where necessary. This does lead to a lot of type: ignore statements. However I believe this is the best option because refactoring this code could be pulling on a long thread.

A refactor would delay deliver of a feature someone is asking for, only to fix something that does not seem to be a problem for most users. Plus, accurate type annotations will actually help users avoid type-unsafe operations in the first place. But I am curious to hear an admin's opinion on this.

  1. Type choices I did my best to choose types based on the docstrings and code context. I'd appreciate a once-over from someone who's familiar with these functions!
  • [ ] Closes #11047
  • [ ] Passes pre-commit run --all-files

jason-trinidad avatar Jul 06 '24 00:07 jason-trinidad

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

GPUtester avatar Jul 06 '24 00:07 GPUtester

Digging into this, it's possible that mypy does not like my choice of annotations. I'll keep looking into it.

jason-trinidad avatar Jul 06 '24 00:07 jason-trinidad

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

     15 files  ± 0       15 suites  ±0   3h 27m 45s :stopwatch: - 1m 11s  13 127 tests + 1   12 012 :white_check_mark: +15   1 068 :zzz: ±0  47 :x:  - 14  162 771 runs  +12  139 937 :white_check_mark: +26  22 787 :zzz: ±0  47 :x:  - 14 

For more details on these failures, see this check.

Results for commit 4d04c9f5. ± Comparison against base commit b4b33cae.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 06 '24 00:07 github-actions[bot]

Just seen this is still open 🥲 any chance one of the maintainers is able to give it a quick once over?

It's only type annotations so isn't that complex a merge! (plus would close an issue!)

(hope this bump isn't too rude- I know you guys are all pretty busy!!)

benrutter avatar Aug 25 '24 06:08 benrutter

Hello, just wanted to check in and see if this got any attention. I moved to fully typed code in my latest project and dask was the only library with significant lack of annotations. I really want to give this issue a bump. Installing this branch manually solved all my issues, so props to the author(s).

piotr-geca-npl avatar Oct 10 '24 08:10 piotr-geca-npl