8000 MAINT: Move typing tests by charris · Pull Request #16800 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Move typing tests #16800

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
Merged
merged 1 commit into from
Sep 7, 2020
Merged

MAINT: Move typing tests #16800

merged 1 commit into from
Sep 7, 2020

Conversation

charris
Copy link
Member
@charris charris commented Jul 10, 2020 •
edited
Loading

Move typing tests into a new numpy/typing/tests directory where they are easier to define and
closer to their data.

Note that this makes the typing module part of the runtime. Is that a problem?

@charris charris added 05 - Testing 03 - Maintenance component: numpy.typing labels Jul 10, 2020
@charris charris force-pushed the move-typing-tests branch from f0570f1 to 8e4d796 Compare July 11, 2020 00:58
@person142
Copy link
Member
person142 commented Jul 11, 2020 •
edited
Loading

Note that this makes the typing module part of the runtime. Is that a problem?

The conclusion last time this came up was that it was something like a 3% increase in import time, and the odds are (increasingly) good that something else will have imported typing already in which case it will be less.

That should be the only consequence.

@charris
Copy link
Member Author
charris commented Jul 11, 2020 •
edited
Loading

@person123 This PR is motivated by the desire to move the debug tests to python-3.8. Those tests are built and tested inplace and the typing tests error. Currently we use python-3.6 for the tests and the typing tests aren't run at all for that version. Because we will be dropping 3.6 soon and the choice of python-dbg is driven by the distribution, Ubuntu focal in this case, we need to have it working or find another way to skip them. I think there is a path problem that this PR will fix, although another possibility is that mypy isn't properly upgraded for the debug run.

I suppose I should check that this PR actually fixes the problem before arguing further.

I don't see why the import times should go up much unless mypy is a slow import, the module isn't imported at the top level. Maybe lazy imports would fix it?

@charris charris changed the title MAINT: Move typing tests WIP, MAINT: Move typing tests Jul 11, 2020
@charris
Copy link
Member Author
charris commented Jul 11, 2020

Looks like this doesn't fix the debug testing problem. I'm suspecting the mypy version is not properly updated.

@charris charris force-pushed the move-typing-tests branch from 6cd4aa2 to c121550 Compare July 11, 2020 12:20
@charris
Copy link
Member Author
charris commented Jul 11, 2020

Solved the debug test problem by adding export MYPYPATH=$PWD to tools/travis-test.sh.

@charris
Copy link
Member Author
charris commented Jul 11, 2020

My numpy import time went from .068 sec. to .068 sec. But such things are hard to time when disk and SSD access may go through fast cache memories.

@charris charris force-pushed the move-typing-tests branch from c121550 to 8e4d796 Compare July 11, 2020 13:35
@eric-wieser
8000
Copy link
Member

Note that this makes the typing module part of the runtime. Is that a problem?

How exactly is this the case?

@charris
Copy link
Member Author
charris commented Jul 11, 2020

How exactly is this the case?

Changed my mind, I don't think it does.

@person142
Copy link
Member

I don't see why the import times should go up much unless mypy is a slow import, the module isn't imported at the top level.

Oops yes, ignore me; I was misunderstanding.

Solved the debug test problem by adding export MYPYPATH=$PWD to tools/travis-test.sh.

The intent was for that to be set here:

https://github.com/numpy/numpy/blob/8e4d796b679520a90ad61d370afc03e8e2ba559f/numpy/typing/tests/data/mypy.ini#L2

but it does look like the path is missing a necessary .. to get all the way to the root. Does adding that help?

@charris
Copy link
Member Author
charris commented Jul 11, 2020 •
edited
Loading

The intent was for that to be set here:

The debug build is special, there is also an export PYTHONPATH=$PWD and it doesn't work without that either.

EDIT: Only the debug build had problems, the mypy tests were skipped when Python-3.6 was used for it, so didn't previously show up.

EDIT: It didn't work with the current typing tests location either without setting the path.

@person142
Copy link
Member
person142 commented Jul 11, 2020 •
edited
Loading

Oh, I think that what you're seeing is that PEP 561 is Python 3.7+ only. So for Python 3.6 it does make sense that you would have to set the MYPYPATH explicitly.

@charris
Copy link
Member Author
charris commented Jul 11, 2020

Python 3.6 it does make sense that you would have to set the MYPYPATH explicitly.

Other way around :) Python-3.6 skipped the tests, Python-3.7-9 run the tests, but python-3.8-dbg, which is built inplace, needs the path set.

@person142
Copy link
Member

Yargh I am really all scrambled up yesterday/today. I am going to just keep quiet now...

@mattip
Copy link
Member
mattip commented Aug 2, 2020

@charris this is still marked WIP and now has merge conflicts

@mattip
Copy link
Member
mattip commented Aug 24, 2020

@charris ping

@charris
Copy link
Member Author
charris commented Aug 24, 2020

The question is if we want to do this.

@charris charris force-pushed the move-typing-tests branch from 8e4d796 to 9d2f944 Compare August 24, 2020 15:07
@charris
Copy link
Member Author
charris commented Aug 24, 2020

The merge conflict came from the addition of numpy/tests/typing/fail/dtype.py in master.

@mattip
Copy link
Member
mattip commented Sep 2, 2020

I think it makes sense, but there are ~10 open PRs for typing that will have a conflict if we merge this. Maybe we should merge those first (hint, hint) and take a break from new PRs so we can fix this one up.

@BvB93
Copy link
Member
BvB93 commented Sep 2, 2020

Considering we have a set of dedicated typing modules I feel it would also make sense to store the typing tests there.
+1

@charris
Copy link
Member Author
charris commented Sep 2, 2020

Rebased here, just needed to add the new files. It would be good to get the rest of the testing stuff in and make the move.

@charris
Copy link
Member Author
charris commented Sep 3, 2020

I have no idea why the test is failing. The error message is

E       AssertionError: numpy/typing/tests/data/pass/flatiter.py:3: error: Module has no attribute "rand"
E         Found 1 error in 1 file (checked 1 source file)
E         
E       assert 1 == 0

Yet if I put that failed line in a try... except clause, it is running just fine. The mypy testing is doing something funny and I suspect there is a bug in api.run, some sort of path thing. The test runs fine in master, it is only the moved test and the call to np.random.rand that fails. Anyone have a idea what run.api is actually checking? And why are we using a np.random.rand(5).flat which flattens a 1-D array, wouldn't np.empty((2, 2)).flat work as well.

@BvB93
Copy link
Member
BvB93 commented Sep 3, 2020

Supposedly adding follow_imports = silent (docs) to the ini file should do the trick, but after some trial and error I can't seem to get it to work.

And why are we using a np.random.rand(5).flat which flattens a 1-D array, wouldn't np.empty((2, 2)).flat work as well.

Mostly a force of habit; let's just change it.

@charris
Copy link
Member Author
charris commented Sep 3, 2020

@BvB93 There is also a report of the rand problem on s390x, see #16134. My guess is that np.random is being confused with Python random in some check.

@BvB93
Copy link
Member
BvB93 commented Sep 3, 2020

My guess is that np.random is being confused with Python random in some check.

This would explain a lot actually.
I was wondering why np.random was already annotated as a types.ModuleType despite it lacking any sort of stub file.

@charris
MAINT: Move typing tests
e97d913 
Move them into a new `numpy/typing/tests directory`
@charris charris changed the title WIP, MAINT: Move typing tests MAINT: Move typing tests Sep 7, 2020
@charris charris removed the 25 - WIP label Sep 7, 2020
@charris
Copy link
Member Author
charris commented Sep 7, 2020

Not waiting on shippable.

@charris charris merged commit 8033864 into numpy:master Sep 7, 2020
@charris charris deleted the move-typing-tests branch September 7, 2020 20:47
@charris
Copy link
Member Author
charris commented Sep 7, 2020

Tests are moved. @BvB93 Could you check that the path in mypi.ini is correct? I'm not sure where it needs to point.

@BvB93 BvB93 mentioned this pull request Sep 7, 2020
Merged
@BvB93
Copy link
Member
BvB93 commented Sep 7, 2020

The mypy_path parameter should actually be redundant as of the addition of #17123.
The latter explicitly sets the MYPYPATH environment variable (which takes priority).

As a side note, I just noticed that we forgot to update runtest.py (fix: #17267).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance 05 - Testing 41 - Static typing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@charris @person142 @eric-wieser @mattip @BvB93 @anirudh2290
0