Skip to content

Typing#848

Closed
marvinvanaalst wants to merge 3 commits into
numba:mainfrom
marvinvanaalst:typing
Closed

Typing#848
marvinvanaalst wants to merge 3 commits into
numba:mainfrom
marvinvanaalst:typing

Conversation

@marvinvanaalst

Copy link
Copy Markdown

Provides type annotations, see issue #828

The changes are mostly additions of type signatures, but since some aspects of Python are too dynamic for mypy, some of the code was rewritten a bit, while staying functionally equal (stuff like turning some decorators into explicit function calls).

I also did some housekeeping, like changing old-style string formatting, black, isort, flake8 and so on - if you are really opposed to that I could re-work it without doing that. I found it helped a lot not having to manually format the code when adding type annotations, as that requires shifting stuff around quite a lot.

Thanks for your great work & hope you'll like the contribution

@gmarkall

Copy link
Copy Markdown
Member

Many thanks for the PR! I've put it as "in progress" for now as I see the docs build failed, but I imagine some feedback review could be valuable at some point even if not all CI passes yet?

@gmarkall

Copy link
Copy Markdown
Member

I also did some housekeeping, like changing old-style string formatting, black, isort, flake8 and so on - if you are really opposed to that I could re-work it without doing that. I found it helped a lot not having to manually format the code when adding type annotations, as that requires shifting stuff around quite a lot.

All the reformatting, whilst it does improve the aesthetic of the code, does seem to add a lot to this PR - is it straightforward for you to break out a new formatting-only PR separate from this one?

@gmarkall

Copy link
Copy Markdown
Member

I am keen to look at the type annotations, both for the benefit of llvmlite and for my own learning, so I've just taken another look at this. It seems the formatting changes are quite extensive so it will be difficult to move this forward for review whilst the formatting changes are all combined into a single PR. I think the possible ways forward are:

  • Re-submit the PR without the formatting changes, or
  • Create a new PR just for the formatting changes, and a separate PR that adds the typing on top.

On the formatting changes:

  • In Numba there has been a conscious decision not to re-format the codebase with black - I don't think we've specifically discussed using it separately for llvmlite, but I do think that it would require weighing up the benefits and detriments of doing so to decide how to proceed.
  • There are now flake8 errors, because the new formatting doesn't match the existing flake8 configuration. If the configuration is to be changed, that would probably require some discussion too.

Given the above, I think the quickest way to move forward would be to have the addition of typing without all the re-formatting - it will be much easier to focus on the core points of reviewing the type annotations, without first having to proceed through a potentially lengthy process of building consensus around substantial format and style changes.

What do you think about the above?

Black formatted

isort

bullshit

conf

sum

ffi/build

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp

tmp
added py.typed file to signal typing information to tools

isort: added future annotations import

Added vscode config

Added mypy config

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

Black formatted

WIP: add type annotations

WIP: add type annotations

Black other repos

Removed (object) from all classes

Removed all old-style super calls

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

Moved tests outside llvmlite directory to exclude type checking for them

First version in which type check succeeds

Replaced old style string formatting

Moved tests back and just excluded them in mypy config file

Replaced some type: ignores with explicit casts

Removed unused imports and other flake8 suggestions

Transformed builder decorators into functions

Transformed values decorators into functions

Cleaned up

Fixed issues

Added devcontainer for easier builds

Removed auto crlf conversion

Changed opt ints to literals

Housekeeping

added newlines to files missing them

removed convenience files

added newline to .gitignore

Black formatted

Added types

isort: added future annotations import

Added vscode config

Added mypy config

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

Black formatted

WIP: add type annotations

WIP: add type annotations

Removed (object) from all classes

Removed all old-style super calls

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

WIP: add type annotations

Moved tests outside llvmlite directory to exclude type checking for them

First version in which type check succeeds

Replaced old style string formatting

Moved tests back and just excluded them in mypy config file

Replaced some type: ignores with explicit casts

Removed unused imports and other flake8 suggestions

Transformed builder decorators into functions

Transformed values decorators into functions

Cleaned up

Fixed issues

Added devcontainer for easier builds

Removed auto crlf conversion

Changed opt ints to literals

Housekeeping

added newlines to files missing them

removed convenience files

added newline to .gitignore

tmp

black & isort

tmp

tmp

tmp

tmp
@marvinvanaalst

marvinvanaalst commented May 31, 2022

Copy link
Copy Markdown
Author

Given the above, I think the quickest way to move forward would be to have the addition of typing without all the re-formatting - it will be much easier to focus on the core points of reviewing the type annotations, without first having to proceed through a potentially lengthy process of building consensus around substantial format and style changes

Yes, absolutely. I think I got a bit carried away with all the changes. I'm currently working on separating the formatting / other changes from just the typing additions. Since I could have been a little more strategic with my commits it's more work than it should be, but it seems to be moving in a good direction. You can take a look here to check.
Most files should then just contain additions, however there will be a few like ir/builder in which I had to re-work e.g. decorators into normal function calls, as otherwise mypy was always angry with me :)

I'd agree to close this and then re-open a new one once I isolated the type annotations.

@marvinvanaalst
marvinvanaalst deleted the typing branch May 31, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants