Ensure stubs are valid for Python 2 and fix running of tests #19

FuegoFro · 2018-05-08T01:03:34Z

The stubs contained an unconditional reference to SupportsBytes, which only exists in Python 3. To make these valid on Python 2, conditionally import that Protocol in Python 3 and otherwise use a dummy class in Python 2. Also have ndarray extend Contains, while we're here.

This also extends the test suites to run all tests against both Python 2 and Python 3, with the ability to specify that certain tests should only be run against Python 3 (eg to test Python 3 exclusive operators). This should help prevent errors like this moving forward.

One downside of this is that flake8 doesn't understand the # type: comments, so it thinks that imports from typing are unused. A workaround for this is to add # noqa: F401 at the end of the relevant imports, though this is a bit tedious.

Finally, change how test requirements are installed and how the numpy-stubs package is exposed to mypy, and update the README/Travis file to reflect this. See python/mypy#5007 for more details about the rational behind this change.

The stubs contained an unconditional reference to SupportsBytes, which only exists in Python 3. To make these valid on Python 2, conditionally import that Protocol in Python 3 and otherwise use a dummy class in Python 2. Also have `ndarray` extend `Contains`, while we're here. This also extends the test suites to run all tests against both Python 2 and Python 3, with the ability to specify that certain tests should only be run against Python 3 (eg to test Python 3 exclusive operators). This should help prevent errors like this moving forward. One downside of this is that flake8 doesn't understand the `# type:` comments, so it thinks that imports from `typing` are unused. A workaround for this is to add `# noqa: F401` at the end of the relevant imports, though this is a bit tedious. Finally, change how test requirements are installed and how the `numpy-stubs` package is exposed to mypy, and update the README/Travis file to reflect this. See python/mypy#5007 for more details about the rational behind this change.

shoyer

This looks great, thank you!

@alanhdu @eric-wieser any comments?

shoyer · 2018-05-08T02:15:40Z

test-requirements.txt

@@ -2,3 +2,6 @@ flake8==3.3.0
 flake8-pyi==17.3.0
 pytest==3.4.2
 mypy==0.590
+# This makes sure that the repo's stubs are accessible. Using MYPYPATH won't
+# work. See https://github.com/python/mypy/issues/5007 for more details.
+.


Interesting, I didn't realize that this worked in a requirements.txt file.

I prefer to keep the self install separate, as requirements are usually for "dependencies", but I suppose that is more of a stylistic nit.

@shoyer Yeah as far as I can tell, anything you can put after pip install you can put as a line in a requirements file :)

@ethanhs That's fair. I mainly wanted to simplify the process of getting the tests to work. But we can definitely remove this and change the README to tell people to run both commands

pip install -r test-requirements.txt pip install .

if that's what folks prefer :)

Let's do the two command version, just so it's more obvious what is going on. It's pretty non-standard to put . in a requirements.txt file.

Sounds good, I separated it out and updated the Travis file an README.

eric-wieser · 2018-05-08T02:22:39Z

numpy-stubs/__init__.pyi

+if sys.version_info[0] < 3:
+    class SupportsBytes: ...
+else:
+    from typing import SupportsBytes


I'd prefer to see this change be in a separate commit to the reformatting (but I do agree with the reformatting in general)

Personally, I am OK with minor reformatting inline with other changes. There are relatively few cases where I've wanted to look at changes with more granularity than that provided by pull request.

I'm happy to pull this out to a separate commit if you'd like :) In my day job we only do squash-and-merge, so I'm not used to thinking about having granular commits within a PR haha.

I've been doing squash-and-merge for most of the PRs to numpy-stubs, so I think it's fine to keep this as is.

emmatyping · 2018-05-08T02:58:27Z

tests/pass/simple_py3.py

+array = np.array([1, 2])
+
+# The @ operator is not in python 2
+array @ array


Some people on 3.4 may want to run the test suite, so perhaps this shouldn't be used? Mypy defaults to using the python version of the executable used to run it.

I don't think enough people are still running Python 3.4 that we need to worry about this.

We could try to come up with a more generalized way of specifying what versions of python a given file can run on, or extend the mechanism so we have something like _py<min_version>.py files, where min_version could be 3, but could also be something like 3.5. If you think that's worth it I'm happy to do that :)

emmatyping · 2018-05-08T03:00:52Z

test-requirements.txt

@@ -2,3 +2,6 @@ flake8==3.3.0
 flake8-pyi==17.3.0
 pytest==3.4.2
 mypy==0.590
+# This makes sure that the repo's stubs are accessible. Using MYPYPATH won't
+# work. See https://github.com/python/mypy/issues/5007 for more details.
+.


I prefer to keep the self install separate, as requirements are usually for "dependencies", but I suppose that is more of a stylistic nit.

FuegoFro

Thank you so much for the quick reviews! It was a bit unclear to me if I should actually change some of these things, so let me know what you'd like to see changed before merging :)

FuegoFro · 2018-05-08T15:00:29Z

numpy-stubs/__init__.pyi

+if sys.version_info[0] < 3:
+    class SupportsBytes: ...
+else:
+    from typing import SupportsBytes


I'm happy to pull this out to a separate commit if you'd like :) In my day job we only do squash-and-merge, so I'm not used to thinking about having granular commits within a PR haha.

FuegoFro · 2018-05-08T15:00:35Z

test-requirements.txt

@@ -2,3 +2,6 @@ flake8==3.3.0
 flake8-pyi==17.3.0
 pytest==3.4.2
 mypy==0.590
+# This makes sure that the repo's stubs are accessible. Using MYPYPATH won't
+# work. See https://github.com/python/mypy/issues/5007 for more details.
+.


@shoyer Yeah as far as I can tell, anything you can put after pip install you can put as a line in a requirements file :)

@ethanhs That's fair. I mainly wanted to simplify the process of getting the tests to work. But we can definitely remove this and change the README to tell people to run both commands

pip install -r test-requirements.txt pip install .

if that's what folks prefer :)

FuegoFro · 2018-05-08T15:00:43Z

tests/pass/simple_py3.py

+array = np.array([1, 2])
+
+# The @ operator is not in python 2
+array @ array


We could try to come up with a more generalized way of specifying what versions of python a given file can run on, or extend the mechanism so we have something like _py<min_version>.py files, where min_version could be 3, but could also be something like 3.5. If you think that's worth it I'm happy to do that :)

shoyer · 2018-05-10T17:58:03Z

I should have mentioned this before, but could you kindly add Python 2.7 (and maybe Python 3.5) to our test matrix configuration for Travis-CI?

… Travis and README files accordingly.

FuegoFro

Regarding

but could you kindly add Python 2.7 (and maybe Python 3.5) to our test matrix configuration for Travis-CI

I'm not entirely sure what this means in this context. In particular, mypy itself should always be run on Python 3, and I'm not sure if we need to test that the mypy tool works on Python 3.5 (that's what mypy's tests are for, right?).

In terms of telling mypy which Python version it should interpret the code/stubs for (eg via the --python-version flag), this diff already handles Python 2.7:

Note that the -2 and --py2 flags are aliases for --python-version 2.7

We can certainly extend the parameterization to also pass --python-version 3.5 to mypy for each test file :)

FuegoFro · 2018-05-10T20:52:04Z

test-requirements.txt

@@ -2,3 +2,6 @@ flake8==3.3.0
 flake8-pyi==17.3.0
 pytest==3.4.2
 mypy==0.590
+# This makes sure that the repo's stubs are accessible. Using MYPYPATH won't
+# work. See https://github.com/python/mypy/issues/5007 for more details.
+.


Sounds good, I separated it out and updated the Travis file an README.

shoyer · 2018-05-10T21:33:51Z

I'm not entirely sure what this means in this context. In particular, mypy itself should always be run on Python 3, and I'm not sure if we need to test that the mypy tool works on Python 3.5 (that's what mypy's tests are for, right?).

OK, I think I missed this about mypy. So nevermind then :).

At some point it would be interesting to run unit tests with pytype, but that's a whole separate project.

shoyer · 2018-05-10T21:35:05Z

Thanks @FuegoFro !

FuegoFro · 2018-05-10T22:27:50Z

Thank you!

shoyer reviewed May 8, 2018

View reviewed changes

eric-wieser reviewed May 8, 2018

View reviewed changes

emmatyping reviewed May 8, 2018

View reviewed changes

FuegoFro commented May 8, 2018

View reviewed changes

Split pip install . out of the test-requirements.txt file, update…

514f8e3

… Travis and README files accordingly.

FuegoFro commented May 10, 2018

View reviewed changes

shoyer merged commit a4857d4 into numpy:master May 10, 2018

Uh oh!

Ensure stubs are valid for Python 2 and fix running of tests #19

Ensure stubs are valid for Python 2 and fix running of tests #19

Uh oh!

Conversation

FuegoFro commented May 8, 2018

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FuegoFro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented May 10, 2018

Uh oh!

FuegoFro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented May 10, 2018

Uh oh!

shoyer commented May 10, 2018

Uh oh!

FuegoFro commented May 10, 2018

Uh oh!

Uh oh!

FuegoFro left a comment •

edited

Loading