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

Install optional dependencies for Python 3.8 tests #5029

Merged
merged 3 commits into from Jan 7, 2020

Conversation

mbargull
Copy link
Contributor

@mbargull mbargull commented Jan 4, 2020

At conda-forge/numba-feedstock#41 we noticed that the test
numba.tests.test_annotations.TestAnnotation.test_exercise_code_path_with_lifted_loop
failed for Python 3.8.
The test is not run if jinja2 is not installed.

Additionally, I'm not sure whether the CI will run this test for Python 3.8 now since Git won't report numba/tests/test_annotations.py as changed. I might add a dummy change to that file in a second commit if that's the case.

@mbargull mbargull changed the base branch from release0.47 to master January 4, 2020 21:55
@mbargull
Copy link
Contributor Author

mbargull commented Jan 4, 2020

I changed the base branch to master since CI doesn't run on release0.47. I'll rebase on master later, after we made sure the test is run.

@mbargull
Copy link
Contributor Author

mbargull commented Jan 6, 2020

Linux py38_np117 fails as anticipated.


Overall it would make sense to make the -g in

NUMBA_ENABLE_CUDASIM=1 $SEGVCATCH python -m numba.runtests -b -v -g -m $TEST_NPROCS -- numba.tests
optional and remove it in case of a build system change, e.g., when a new Python version is added to the test matrix.


(Not directly related to this issue)
Additionally, I noticed a slight flaw in the sliced testing (unless I overlooked something): In case of environment-dependent tests, a subset of the tests won't be run. E.g., only about every 20th of the tbb will be run since only one of the test configurations over which the slices are split uses tbb.

@stuartarchibald
Copy link
Contributor

Linux py38_np117 fails as anticipated.

Thanks for the report. I can reproduce locally, seems like a loop that was being lifted now cannot be since the 3.8 bytecode change/entire interpreter rewrite.

Overall it would make sense to make the -g in

NUMBA_ENABLE_CUDASIM=1 $SEGVCATCH python -m numba.runtests -b -v -g -m $TEST_NPROCS -- numba.tests

optional and remove it in case of a build system change, e.g., when a new Python version is added to the test matrix.

The purpose of the -g is to run test files which have changed by virtue of git diff. Is there some problem with this?

(Not directly related to this issue)
Additionally, I noticed a slight flaw in the sliced testing (unless I overlooked something): In case of environment-dependent tests, a subset of the tests won't be run. E.g., only about every 20th of the tbb will be run since only one of the test configurations over which the slices are split uses tbb.

Thanks, yes, we are aware of this. The reason we do test slicing is that Public CI cannot cope with the size of the Numba test suite coupled with the amount of testing traffic. As a result Public CI is used essentially as a smoke test. Some time soon we're hoping we can resolve the performance issues in Pytest and move to that #4753. With the pytest labelling we separate the testing into 3 sets:

  • "tests that are from git diff"
  • "tests that can be run everywhere as they don't depend on anything specific (i.e. hardware or some package)"
  • "tests that depend on some specific thing (hardware or package)"

slicing will then only be done on the "tests that should be run everywhere" set.

@mbargull
Copy link
Contributor Author

mbargull commented Jan 6, 2020

I can reproduce locally

👍

The purpose of the -g is to run test files which have changed by virtue of git diff. Is there some problem with this?

Yes, a slight problem is when the build setup is changed, e.g., .travis.yml or buildscripts/incremental/ is modified, for instance by adding a new Python version to test against: In that case it would make sense to run all of the tests since that new test configuration might've never run the tests that are not reported by git diff.

Thanks, yes, we are aware of this. [...]
With the pytest labelling we separate the testing into 3 sets: [...]
slicing will then only be done on the "tests that should be run everywhere" set.

Ah, perfect!

@stuartarchibald
Copy link
Contributor

I can reproduce locally

+1

The purpose of the -g is to run test files which have changed by virtue of git diff. Is there some problem with this?

Yes, a slight problem is when the build setup is changed, e.g., .travis.yml or buildscripts/incremental/ is modified, for instance by adding a new Python version to test against: In that case it would make sense to run all of the tests since that new test configuration might've never run the tests that are not reported by git diff.

hmmm, I see, thanks, perhaps the -g logic needs to select all if the diff is coming from somewhere in buildscripts or a CI config file.

<snip>

@stuartarchibald
Copy link
Contributor

@mbargull am hoping bd8f048 will fix this branch.

Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

Looks ok for now. I'll give a quick look at why py38 does that

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Jan 7, 2020
@stuartarchibald stuartarchibald merged commit 510eefd into numba:master Jan 7, 2020
@mbargull mbargull mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants