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.

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

First pass mypy typing #387

Merged
merged 18 commits into from
Jun 14, 2022
Merged

First pass mypy typing #387

merged 18 commits into from
Jun 14, 2022

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Jun 1, 2022

This PR adds begins adding Python types Cunumeric, starting with the following modules

cunumeric._sphinxext
cunumeric._ufunc
cunumeric.coverage
cunumeric.fft
cunumeric.install_info
cunumeric.linalg
cunumeric.patch
cunumeric.random
cunumeric.utils
cunumeric.window

Note the ugly mypy_path hack currently in pyproject.toml:

mypy_path = "typings/:../legate.core/typings/:..legate.core/install38/lib/python3.8/site-packages/"

This is because legate is not actually installed in the site-packages for the Python that is executing mypy. I think probably the only solution is a dedicated script to invoke mypy and also set the env var MYPY_PATH from computed values for the legate install. But if anyone has any other ideas, I would love to hear them.

@bryevdv bryevdv requested a review from magnatelee June 1, 2022 17:27
@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 1, 2022

Working on linalg now and running into issues with the code that uses legate.core directly. I think actually we will have to simply check both together, in order to check cunumeric:

mypy ../legate.core/install38/lib/python3.8/site-packages/legate /home/bryan/work/legate.core/typings/ cunumeric

I tried several approaches with MYPYPATH and it was just not picking up imports and types correctly.

Anyway, I will stop this PR for now as a first step.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 1, 2022

Once #387 is merged, then linalg also works, with either of these invocations:

~/work/cunumeric bryanv/mypy*
dev38 ❯ mypy ../legate.core/legate /home/bryan/work/legate.core/typings/ cunumeric
Success: no issues found in 79 source files

~/work/cunumeric bryanv/mypy*
dev38 ❯ mypy ../legate.core/install38/lib/python3.8/site-packages/legate /home/bryan/work/legate.core/typings/ cunumeric
Success: no issues found in 79 source files

I assume passing the installed version will be more CI-friendly? If so then I will add a script to wrap the mypy invocation to include this.

@manopapad
Copy link
Contributor

This is because legate is not actually installed in the site-packages for the Python that is executing mypy. I think probably the only solution is a dedicated script to invoke mypy and also set the env var MYPY_PATH from computed values for the legate install. But if anyone has any other ideas, I would love to hear them.

We could just bite the bullet and update our setup process to always install legate/cunumeric properly in the currently active virtual environment, instead of using a custom directory that needs to be passed everywhere. We will need to do this anyway eventually, as the setuptools deprecation warnings keep telling us.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 1, 2022

We could just bite the bullet and update our setup process to always install legate/cunumeric properly in the currently active virtual environment, instead of using a custom directory that needs to be passed everywhere. We will need to do this anyway eventually, as the setuptools deprecation warnings keep telling us.

I think that would be wonderful but also sounds like a bit of an endeavor. I guess I would not hold up this PR for that, since a wrapper script is a simple solution for the immediate term.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 2, 2022

OK I removed the mypy_path config. I don't want to add a script just yet until the cmake PRs are merged, since the mechanism to discover the legate install will be different. For now, run e.g.

mypy ../legate.core/install38/lib/python3.8/site-packages/legate /home/bryan/work/legate.core/typings/ cunumeric

I guess we will actually need to find a place to install those typings if we want to avoid referencing the source tree for legate (or maybe that's fine).

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 2, 2022

whoops, introduced two minor issues. Should go green now (all tests pass locally eager/gpu/cpu)

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 2, 2022

OK it was also easy to add cunumeric.logic and at that point also simplify the config in pyproject.toml The rest are better left to a follow-on PR.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jun 3, 2022

Merge conflict is resolved.

cunumeric/_ufunc/__init__.py Show resolved Hide resolved
cunumeric/_ufunc/ufunc.py Outdated Show resolved Hide resolved
cunumeric/_ufunc/ufunc.py Show resolved Hide resolved
cunumeric/_ufunc/ufunc.py Outdated Show resolved Hide resolved
cunumeric/fft/fft.py Show resolved Hide resolved
cunumeric/linalg/cholesky.py Outdated Show resolved Hide resolved
cunumeric/linalg/linalg.py Show resolved Hide resolved
cunumeric/linalg/linalg.py Outdated Show resolved Hide resolved
cunumeric/linalg/linalg.py Outdated Show resolved Hide resolved
cunumeric/random/random.py Show resolved Hide resolved
if runtime.args.test_mode:
num_tiles = runtime.num_procs * 2
return (num_tiles, num_tiles)
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diff here was simply removing the superfluous else.

@bryevdv bryevdv merged commit 2a00f02 into nv-legate:branch-22.07 Jun 14, 2022
@bryevdv bryevdv deleted the bryanv/mypy branch June 14, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants