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

Add Python 3.7 and Python 3.8 to CI test jobs #2450

Merged
merged 17 commits into from
Apr 1, 2020

Conversation

munkm
Copy link
Member

@munkm munkm commented Feb 10, 2020

PR Summary

This PR is a progression from the work happening in #2448. In that PR some of the CI test jobs on travis were changed as a result of updates our code related to numpy compatibility. The highest level of Python that we're testing against on both appveyor and travis right now is 3.6. This PR adds jobs to both appveyor and travis for Python 3.7 and 3.8. It also removes the python 2.7 testing from appveyor.

(this should wait to be merged until after 2448 is merged.)

@munkm
Copy link
Member Author

munkm commented Mar 10, 2020

@yt-fido test this please

@munkm
Copy link
Member Author

munkm commented Mar 10, 2020

The environment issue for appveyor on python 3.8 is solved now, so this should be good to merge.

@munkm
Copy link
Member Author

munkm commented Mar 10, 2020

Nevermind. The build resolves but the tests aren't running.

@neutrinoceros
Copy link
Member

I remember you mentioning matplotlib 3.2 failures on appveyor, but couldn't find your statement back. Since the present PR is challenging in itself, I'd suggest we freeze mpl to 3.1 here, for the time being.

@munkm
Copy link
Member Author

munkm commented Mar 16, 2020

So the test failures in this particular PR are not stemming from matplotlib 3.2 (yet. I'm sure they will appear if I push an update.). For some reason the Python 3.8 environment isn't running the tests at all and I need to figure out why.

@munkm
Copy link
Member Author

munkm commented Mar 19, 2020

@yt-fido test this please

@munkm
Copy link
Member Author

munkm commented Mar 20, 2020

Ok I did some digging on the failures on Jenkins. I think the issue is that for SciPy v 1.4 there are import errors with Python 3.5 (see here: scipy/scipy#11356) . When SciPy 1.5 is released we can use that (or if we update to Python 3.6 we can use Scipy 1.4). For now I'm bumping the SciPy version back down to 1.3.3.

For cartopy the failures are also resulting from SciPy. Because SciPy isn't importing, cartopy is trying to use pykdtree for the transform/projections and then an error is being thrown because that package isn't installed. Bumping the scipy version fixes this.

@Xarthisius
Copy link
Member

I run into an issue with MPL-3.0.3 and py3.8. TriLinearInterpolation seems to take ages. I recall we had a similar problem when cython/c module responsible for it was compiled in debug mode upstream. I checked that mpl-3.1.3 works just fine. Can we use that version as min requirement for tests?

@Xarthisius
Copy link
Member

@yt-fido test this please

@munkm
Copy link
Member Author

munkm commented Mar 21, 2020

Ok, done! I bumped the mpl version from 3.0.3 to 3.1.1 in thetest_requirements file, not the test_minimum_requirements file. Let me know if I should bump another place!

@munkm munkm added the yt-3.6 feature targeted for the yt-3.6 release label Mar 27, 2020
.travis.yml Outdated
- stage: tests
name: "Python: 3.7 Unit Tests"
python: 3.7
script: coverage run $(which nosetests) -c nose_unit.cfg
Copy link
Member

Choose a reason for hiding this comment

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

I think it's sufficient to test only 3.6 and 3.8. Hopefully it's going to be enough and will save us a lot of time.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok i've removed py 3.5 and 3.7 from appveyor and travis

@munkm munkm mentioned this pull request Mar 31, 2020
@matthewturk
Copy link
Member

The answer test difference results are here:

http://use.yt/upload/1f30ffb2

Looking these over, they all look quite reasonable, and likely are the result of matplotlib changes.

@munkm
Copy link
Member Author

munkm commented Apr 1, 2020

Ok all of the tests pass now, so this should be ready for review!

Copy link
Member

@cphyc cphyc left a comment

Choose a reason for hiding this comment

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

I left some comments, mostly stylistic ones but overall this LGTM.

@@ -16,12 +16,14 @@ pandas==0.23.4
pytest~=5.2; python_version >= '3.0'
pytest~=4.6; python_version < '3.0'
requests==2.20.0
scipy==1.1.0
scipy==1.1.0; python_version < '3.0'
Copy link
Member

Choose a reason for hiding this comment

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

Since Python 2 is not supported, is there a reason to keep this?

Copy link
Member

Choose a reason for hiding this comment

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

We're still running tests for py2.7, just not on Travis. It hasn't been dropped yet.

sympy==1.5
pyqt5==5.11.3; python_version >= '3.0'
thingking==1.0.2; python_version < '3.0'
pint==0.8.1
netCDF4==1.4.2
netCDF4==1.4.2; python_version < '3.0'
netCDF4==1.5.3; python_version >= '3.0'
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, I would advocate to remote the python_version < '3.0' line.

Comment on lines +53 to +56
if [[ ${TRAVIS_BUILD_STAGE_NAME} == "lint" ]]; then
export TRAVIS_BUILD_STAGE_NAME="Lint"
fi
if [[ ${TRAVIS_BUILD_STAGE_NAME} != "Lint" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Here and below, what is the reason to use Lint instead of lint?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it could be either, but basically what seemed to be happening is that while previously Travis had been capitalizing the stage names (i.e., lint became Lint) for some reason it no longer was in our jobs. And, because OS X runs bash 3.2, we couldn't use the ${VAR^^} or ${VAR,,} construction to change the case as a whole.

So, this is in there in case it switches back. It could use either one, but we figured that the least invasive one would be to assume the old behavior would return some day.

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Great job getting the test working again! I second all of the comments from @cphyc, but don't have any more of my own, so I'll approve.

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Well done ! Same as @brittonsmith , I second @cphyc's review and have no further comment.
Thanks a lot for fixing this !

@Xarthisius Xarthisius merged commit fa959a6 into yt-project:master Apr 1, 2020
matthewturk added a commit to matthewturk/yt that referenced this pull request Apr 1, 2020
Xarthisius added a commit that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yt-3.6 feature targeted for the yt-3.6 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants