Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add type annotation in _creation.basic #7739

Merged
merged 9 commits into from
Aug 25, 2023
Merged

Add type annotation in _creation.basic #7739

merged 9 commits into from
Aug 25, 2023

Conversation

asi1024
Copy link
Member

@asi1024 asi1024 commented Jul 20, 2023 •
edited

Part of #4831.

Blocked by #7738 (merged).

@emcastillo emcastillo self-assigned this Jul 21, 2023
@emcastillo emcastillo added cat:code-fix Code refactoring that do not change behavior prio:medium labels Jul 21, 2023
@asi1024 asi1024 marked this pull request as ready for review July 21, 2023 15:42
@kmaehashi
Copy link
Member

kmaehashi commented Jul 22, 2023 •
edited

@asi1024 Thanks for starting to work on this! Just reviewed the surface 😅

@asi1024 asi1024 force-pushed the type-creation-basic branch 4 times, most recently from fd7ea15 to bf3edd9 Compare July 24, 2023 21:16
@asi1024 asi1024 changed the title Add type annotation in _creation.basic [WIP] Add type annotation in _creation.basic Jul 24, 2023
@asi1024 asi1024 changed the title [WIP] Add type annotation in _creation.basic Add type annotation in _creation.basic Jul 26, 2023
@asi1024 asi1024 force-pushed the type-creation-basic branch 2 times, most recently from fd0697b to ccaf0e9 Compare July 26, 2023 12:38
@kmaehashi kmaehashi self-assigned this Aug 2, 2023
kmaehashi
kmaehashi reviewed Aug 3, 2023
View reviewed changes
cupy/_creation/basic.py Outdated


_OrderKACF = Literal[None, "K", "A", "C", "F"]
_OrderCF = Literal[None, "C", "F"]
Copy link
Member

Choose a reason for hiding this comment

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

How about putting them under cupy.typing._types so that they can be reused in other code?

NumPy: https://github.com/numpy/numpy/blob/e0c8bc50b262b4b8ce80b332b5349e165319cf42/numpy/__init__.pyi#L981

.github/workflows/pretest.yml Show resolved Hide resolved
.pfnci/generate.py Outdated
@@ -175,6 +175,7 @@ def generate_dockerfile(self) -> str:
f'RUN pyenv install {py_spec} && \\',
f' pyenv global {py_spec} && \\',
' pip install -U setuptools pip wheel',
' pip install mypy==1.4.1',
Copy link
Member

Choose a reason for hiding this comment

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

Let's test typing things in GitHub Actions instead of FlexCI with GPU, which is expensive.

Suggested change
' pip install mypy==1.4.1',

.pfnci/windows/test.ps1 Outdated
@@ -78,7 +78,7 @@ function Main {

echo "Building..."
$build_retval = 0
RunOrDie python -m pip install -U "numpy<1.24" "scipy<1.10.0"
RunOrDie python -m pip install -U "numpy<1.24" "scipy<1.10.0" "mypy==1.4.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RunOrDie python -m pip install -U "numpy<1.24" "scipy<1.10.0" "mypy==1.4.1"
RunOrDie python -m pip install -U "numpy<1.24" "scipy<1.10.0"

tests/typing_tests/.gitignore Outdated
@@ -0,0 +1 @@
*_numpy.pyi
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is it possible to generate files under the temporary directory? Or will that invalidate mypy cache?

tests/typing_tests/cupy_tests/creation_tests/test_basic.pyi Outdated
Comment on lines 1 to 3
import numpy as np
import cupy as cp
import cupy as xp
Copy link
Member

Choose a reason for hiding this comment

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

I felt difficult distinguishing xp, cp, np in test cases 😅
How about this?

Suggested change
import numpy as np
import cupy as cp
import cupy as xp
import cupy as xp
import numpy as _np
import cupy as _cp

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree about not using np and cp, but I prefer import numpy and import cupy without aliasing.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds nice!

tests/typing_tests/test_typing.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we need tests/typing_tests/mypy.ini to define the rule we should obey?

@kmaehashi
Copy link
Member

Hmm Sphinx docs seems still noisy 😢
https://cupy--7739.org.readthedocs.build/en/7739/reference/generated/cupy.empty_like.html#cupy-empty-like

@asi1024
Copy link
Member Author

asi1024 commented Aug 9, 2023

@kmaehashi @emcastillo Ready for review!

emcastillo
emcastillo previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM! Only the pyi nuance :)

@asi1024
Copy link
Member Author

asi1024 commented Aug 10, 2023

/test mini

@asi1024
Copy link
Member Author

asi1024 commented Aug 10, 2023

/test mini

@asi1024
Copy link
Member Author

asi1024 commented Aug 15, 2023

CI failure seems unrelated.

@kmaehashi
Copy link
Member

LGTM!

@kmaehashi kmaehashi merged commit 501dc69 into cupy:main Aug 25, 2023
47 of 48 checks passed
@kmaehashi kmaehashi added this to the v13.0.0rc1 milestone Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmaehashi kmaehashi kmaehashi approved these changes

@emcastillo emcastillo emcastillo left review comments

Assignees

@kmaehashi kmaehashi

@emcastillo emcastillo

Labels
cat:code-fix Code refactoring that do not change behavior prio:medium
Projects
None yet
Milestone
v13.0.0rc1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@asi1024 @kmaehashi @emcastillo