Restore CI after PY2 drop #25752
Restore CI after PY2 drop #25752
Conversation
|
pls explain why you think we need all of these new jobs? exactly which is each testing that we don't have on the existing ones? I of course want comprehensive testing, but duplication is also not great. |
This would have been so much easier to explain as part of #24942. For example, now there's no compat jobs anymore, theres two less locale jobs, there's less version coverage for numpy/mpl/other libs, etc. I mean, if there's good reason to remove CI jobs, great. But the default should be updating the old ones, not throwing them out. Now you make me argue for why these should be in - TBH I don't care about the old compat jobs (so I'm not gonna waste my energy fighting you for them), but lots of other people seem to do. @jorisvandenbossche @TomAugspurger @toobaz @gfyoung @WillAyd |
|
@h-vetinari you are missing the point. You have changed a bunch of stuff and now some things fail. Its not really clear why this would be the case, so you must explain it. Since the CI jobs are so important to backward compatibility, it is now not clear what guarantees we have. |
Codecov Report
@@ Coverage Diff @@
## master #25752 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 172 172
Lines 52977 52977
=======================================
Hits 48342 48342
Misses 4635 4635
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25752 +/- ##
=======================================
Coverage 91.47% 91.47%
=======================================
Files 173 173
Lines 52872 52872
=======================================
Hits 48364 48364
Misses 4508 4508
Continue to review full report at Codecov.
|
The changes to the jobs have been minimal (basically just raising pins until dependencies can be resolved by conda), and 5
You forced me to remove those compat jobs. It's my goodwill despite your uncompromising reviews to try to re-add them. But yes, due to that removal, the diff is now much less clear. However, it's not a hill I'm gonna die on. |
Let's try to keep two discussions separated: 1) do we need to add new jobs (to replace certain aspects that the py27 jobs were testing) and 2) how do we fix the new failures. I am not too up to date with all the different CI builds at the moment (especially not with the more recent Azure builds), so it is hard for me to help assessing if those extra builds are needed. @h-vetinari you said above "For example, now there's no compat jobs anymore, theres two less locale jobs, there's less version coverage for numpy/mpl/other libs, etc." With "no compat", I assume you mean there is no CI build anymore that consistently uses the lowest supported versions? So we should probably list the removed py27 builds with the specific things they were testing. So that we can then assess for each of them whether this is already covered or not by the existing ones. BTW, @h-vetinari thanks lot for working on this! (and it's not the most attractive part I would say ..) |
|
OK, here's the diff to the commit in This should allow easier discussion of what this PR does compared to the CI up until very recently. diff --git a/ci/azure/posix.yml b/ci/azure/posix.yml
index b9e0cd0b9..7119054cf 100644
--- a/ci/azure/posix.yml
+++ b/ci/azure/posix.yml
@@ -9,20 +9,20 @@ jobs:
strategy:
matrix:
${{ if eq(parameters.name, 'macOS') }}:
- py35_np_120:
+ py35_macos:
ENV_FILE: ci/deps/azure-macos-35.yaml
CONDA_PY: "35"
PATTERN: "not slow and not network"
${{ if eq(parameters.name, 'Linux') }}:
- py27_np_120:
- ENV_FILE: ci/deps/azure-27-compat.yaml
- CONDA_PY: "27"
+ py35_compat:
+ ENV_FILE: ci/deps/azure-35-compat.yaml
+ CONDA_PY: "35"
PATTERN: "not slow and not network"
- py27_locale_slow_old_np:
- ENV_FILE: ci/deps/azure-27-locale.yaml
- CONDA_PY: "27"
+ py36_locale_slow_old_np:
+ ENV_FILE: ci/deps/azure-36-locale.yaml
+ CONDA_PY: "36"
PATTERN: "slow"
LOCALE_OVERRIDE: "zh_CN.UTF-8"
EXTRA_APT: "language-pack-zh-hans"
diff --git a/ci/azure/windows.yml b/ci/azure/windows.yml
index cece00202..cd5879bf5 100644
--- a/ci/azure/windows.yml
+++ b/ci/azure/windows.yml
@@ -12,9 +12,9 @@ jobs:
ENV_FILE: ci/deps/azure-windows-36.yaml
CONDA_PY: "36"
- py27_np121:
- ENV_FILE: ci/deps/azure-windows-27.yaml
- CONDA_PY: "27"
+ py37_np141:
+ ENV_FILE: ci/deps/azure-windows-37.yaml
+ CONDA_PY: "37"
steps:
- task: CondaEnvironment@1
@@ -22,13 +22,6 @@ jobs:
updateConda: no
packageSpecs: ''
- - powershell: |
- $wc = New-Object net.webclient
- $wc.Downloadfile("https://download.microsoft.com/download/7/9/6/796EF2E4-801B-4FC4-AB28-B59FBF6D907B/VCForPython27.msi", "VCForPython27.msi")
- Start-Process "VCForPython27.msi" /qn -Wait
- displayName: 'Install VC 9.0 only for Python 2.7'
- condition: eq(variables.CONDA_PY, '27')
-
- script: |
ci\\incremental\\setup_conda_environment.cmd
displayName: 'Before Install'
diff --git a/ci/deps/azure-27-compat.yaml b/ci/deps/azure-35-compat.yaml
similarity index 59%
rename from ci/deps/azure-27-compat.yaml
rename to ci/deps/azure-35-compat.yaml
index a7784f17d..05436e799 100644
--- a/ci/deps/azure-27-compat.yaml
+++ b/ci/deps/azure-35-compat.yaml
@@ -3,26 +3,27 @@ channels:
- defaults
- conda-forge
dependencies:
+ - beautifulsoup4==4.4.1
- bottleneck=1.2.0
- cython=0.28.2
+ - hypothesis>=3.58.0
- jinja2=2.8
- numexpr=2.6.1
- numpy=1.12.0
- - openpyxl=2.5.5
+ - openpyxl=2.2.6
- pytables=3.4.2
- python-dateutil=2.5.0
- - python=2.7*
- - pytz=2013b
+ - python=3.5*
+ - pytz=2015.4
- scipy=0.18.1
- xlrd=1.0.0
- - xlsxwriter=0.5.2
- - xlwt=0.7.5
+ - xlsxwriter=0.7.7
+ - xlwt=1.0.0
# universal
- - pytest>=4.0.2
- pytest-xdist
- pytest-mock
- isort
- pip:
+ # for python 3.5, pytest>=4.0.2 is not available in conda
+ - pytest>=4.0.2
- html5lib==1.0b2
- - beautifulsoup4==4.2.1
- - hypothesis>=3.58.0
diff --git a/ci/deps/azure-27-locale.yaml b/ci/deps/azure-36-locale.yaml
similarity index 74%
rename from ci/deps/azure-27-locale.yaml
rename to ci/deps/azure-36-locale.yaml
index 8636a63d0..c74d56443 100644
--- a/ci/deps/azure-27-locale.yaml
+++ b/ci/deps/azure-36-locale.yaml
@@ -3,6 +3,7 @@ channels:
- defaults
- conda-forge
dependencies:
+ - beautifulsoup4==4.5.1
- bottleneck=1.2.0
- cython=0.28.2
- lxml
@@ -11,14 +12,13 @@ dependencies:
- openpyxl=2.4.0
- python-dateutil
- python-blosc
- - python=2.7
- - pytz
- - pytz=2013b
+ - python=3.6
+ - pytz=2016.10
- scipy
- - sqlalchemy=0.8.1
+ - sqlalchemy=1.1.4
- xlrd=1.0.0
- - xlsxwriter=0.5.2
- - xlwt=0.7.5
+ - xlsxwriter=0.9.4
+ - xlwt=1.2.0
# universal
- pytest>=4.0.2
- pytest-xdist
@@ -27,4 +27,3 @@ dependencies:
- isort
- pip:
- html5lib==1.0b2
- - beautifulsoup4==4.2.1
diff --git a/ci/deps/azure-windows-36.yaml b/ci/deps/azure-windows-36.yaml
index 8517d340f..5ce55a4cb 100644
--- a/ci/deps/azure-windows-36.yaml
+++ b/ci/deps/azure-windows-36.yaml
@@ -15,7 +15,7 @@ dependencies:
- pyarrow
- pytables
- python-dateutil
- - python=3.6.6
+ - python=3.6.*
- pytz
- scipy
- xlrd
diff --git a/ci/deps/azure-windows-27.yaml b/ci/deps/azure-windows-37.yaml
similarity index 81%
rename from ci/deps/azure-windows-27.yaml
rename to ci/deps/azure-windows-37.yaml
index f40efdfca..96ddc1d62 100644
--- a/ci/deps/azure-windows-27.yaml
+++ b/ci/deps/azure-windows-37.yaml
@@ -5,17 +5,17 @@ channels:
dependencies:
- beautifulsoup4
- bottleneck
- - dateutil
- gcsfs
- html5lib
- - jinja2=2.8
+ - jinja2
- lxml
- - matplotlib=2.0.1
+ - matplotlib=3.0.1
- numexpr
- - numpy=1.12*
+ - numpy=1.14.*
- openpyxl
- pytables
- - python=2.7.*
+ - python=3.7.*
+ - python-dateutil
- pytz
- s3fs
- scipy
diff --git a/ci/deps/travis-27.yaml b/ci/deps/travis-35.yaml
similarity index 71%
rename from ci/deps/travis-27.yaml
rename to ci/deps/travis-35.yaml
index a910af36a..8ae0bf6a0 100644
--- a/ci/deps/travis-27.yaml
+++ b/ci/deps/travis-35.yaml
@@ -6,11 +6,9 @@ dependencies:
- beautifulsoup4
- bottleneck
- cython=0.28.2
- - fastparquet>=0.2.1
- gcsfs
- html5lib
- ipython
- - jemalloc=4.5.0.post
- jinja2=2.8
- lxml
- matplotlib=2.2.2
@@ -24,28 +22,27 @@ dependencies:
- py
- pyarrow=0.9.0
- PyCrypto
- - pymysql=0.6.3
+ - pymysql=0.6.6
- pytables
- blosc=1.14.3
- python-blosc
- python-dateutil=2.5.0
- - python=2.7*
- - pytz=2013b
- - s3fs
+ - python=3.5*
+ - pytz=2015.4
- scipy
- - sqlalchemy=0.9.6
+ - sqlalchemy=1.0.8
+ - s3fs
- xarray=0.9.6
- xlrd=1.0.0
- - xlsxwriter=0.5.2
- - xlwt=0.7.5
+ - xlsxwriter=0.7.7
+ - xlwt=1.0.0
# universal
- - pytest>=4.0.2
- pytest-xdist
- pytest-mock
- - moto==1.3.4
+ - moto
- hypothesis>=3.58.0
- isort
- pip:
- - backports.lzma
+ # not available for python 3.5 through conda
- pandas-gbq
- - pathlib
+ - pytest>=4.0.2
diff --git a/ci/deps/travis-36-doc.yaml b/ci/deps/travis-36-doc.yaml
index 6f33bc58a..8015f7bdc 100644
--- a/ci/deps/travis-36-doc.yaml
+++ b/ci/deps/travis-36-doc.yaml
@@ -15,12 +15,12 @@ dependencies:
- ipywidgets
- lxml
- matplotlib
- - nbconvert
+ - nbconvert>=5.4.1
- nbformat
- nbsphinx
- - notebook
+ - notebook>=5.7.5
- numexpr
- - numpy=1.13*
+ - numpy
- numpydoc
- openpyxl
- pandoc
diff --git a/ci/run_with_env.cmd b/ci/run_with_env.cmd
index 848f4608c..0661039a2 100644
--- a/ci/run_with_env.cmd
+++ b/ci/run_with_env.cmd
@@ -1,5 +1,5 @@
:: EXPECTED ENV VARS: PYTHON_ARCH (either x86 or x64)
-:: CONDA_PY (either 27, 33, 35 etc. - only major version is extracted)
+:: CONDA_PY (either 35, 36 etc. - only major version is extracted)
::
::
:: To build extensions for 64 bit Python 3, we need to configure environment
@@ -45,7 +45,7 @@ SET WIN_SDK_ROOT=C:\Program Files\Microsoft SDKs\Windows
SET MAJOR_PYTHON_VERSION=%CONDA_PY:~0,1%
IF "%CONDA_PY:~2,1%" == "" (
- :: CONDA_PY style, such as 27, 34 etc.
+ :: CONDA_PY style, such as 36, 37 etc.
SET MINOR_PYTHON_VERSION=%CONDA_PY:~1,1%
) ELSE (
IF "%CONDA_PY:~3,1%" == "." ( |
pandas/tests/io/test_excel.py
Outdated
| @@ -2182,6 +2182,8 @@ def test_write_cells_merge_styled(self, merge_cells, ext, engine): | |||
| assert xcell_b1.font == openpyxl_sty_merged | |||
| assert xcell_a2.font == openpyxl_sty_merged | |||
|
|
|||
| @pytest.mark.xfail(not PY36, reason='only fails on Linux?', | |||
There was a problem hiding this comment.
What failure are you getting here exactly?
There was a problem hiding this comment.
@WillAyd
Those come from #24942, see e.g. the CI failures for one of the commits where I specifically reverted those xfails:
https://travis-ci.org/pandas-dev/pandas/builds/506535598?utm_source=github_status&utm_medium=notification
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=9457
.travis.yml
Outdated
| @@ -35,6 +35,10 @@ matrix: | |||
| env: | |||
| - JOB="3.7" ENV_FILE="ci/deps/travis-37.yaml" PATTERN="(not slow and not network)" | |||
|
|
|||
| - dist: trusty | |||
There was a problem hiding this comment.
can you remove this job. I think everything else is ok.
|
Let's have a look at the python versions in the CI:
Since you're asking me to remove Maybe the overview helps with other decisions as well? More windows jobs? |
|
@jreback Now that the |
|
@jreback |
pandas/tests/test_sorting.py
Outdated
| @@ -7,7 +7,7 @@ | |||
| from numpy import nan | |||
| import pytest | |||
|
|
|||
| from pandas.compat import PY2 | |||
| from pandas.compat.numpy import _np_version_under1p14 | |||
There was a problem hiding this comment.
import from the pandas namespace
pandas/tests/test_algos.py
Outdated
| from pandas.compat import PY2, lrange, range | ||
| from pandas.compat.numpy import np_array_datetime64_compat | ||
| from pandas.compat import lrange, range | ||
| from pandas.compat.numpy import ( |
There was a problem hiding this comment.
import the the pandas namespace (for consistency with the rest of the codebase)
I don't remember if np_array_datetime64_compat is from there as well (and I think we are removing this after the numpy bump anyhow)
|
also merge master |
|
@jreback |
pandas/tests/io/test_excel.py
Outdated
| @@ -2182,6 +2182,8 @@ def test_write_cells_merge_styled(self, merge_cells, ext, engine): | |||
| assert xcell_b1.font == openpyxl_sty_merged | |||
| assert xcell_a2.font == openpyxl_sty_merged | |||
|
|
|||
| @pytest.mark.xfail(not PY36, reason='only fails on Linux', | |||
There was a problem hiding this comment.
why exactly causes this not to work? is it a openpyxl version issue?
|
@jreback I guess now that there's only one xfail left, I could try bumping openpyxl again - before it had no impact on the xfails, but that may have also been for the job that's how been removed. |
|
yeah this is an openpyxl issue. so no problem bumping that up 2.4.0 should work i think (and that is the pinned on another version). ping on fixed. |
For that particular job, it was already pinned to 2.5.5. I've unpinned it now, but if it still does not pass, can we agree that a single PY35 skip is OK? |
|
openpyxl==2.2.6 on the 35_compat build pls bump in that one |
That's already the case. The failure was on the other 35 build, and already had Also, I seem to remember that the DB-tests only run on travis (cc @TomAugspurger), so by removing the travis-compat job, there's now no lower bound being tested for pymysql anymore. |
|
ok fair can u out a comprehensive comment on that skip then - explaining why u think it’s failing and on what build (so hence u skip) |
|
@jreback But so you're fine with dropping compat testing for |
|
yeah lowest resolvable for py36 is fine |
|
@jreback I "solved" the xfail by bumping the minimum for openpyxl to 2.4.0, but that's from 2016, so shouldn't be an issue. pymysql is now the lowest resolvable for py36. |
| +-----------------+-----------------+----------+ | ||
| | pymysql | 0.6.6 | | | ||
| | pymysql | 0.7.9 | | |
There was a problem hiding this comment.
We don't actually require these versions for sqlalchemy / pymsql, they are just the recommended ones. So may want to re-word this (can do in a followup). We are don't actually reject an earlier version.
|
thanks @h-vetinari comment, but can address in a followup. just want to distinguish real required version that we test vs 'recommended' things that we just have compat on our CI for. (maybe just needs a small expl / wording change). |
|
pls rebase the numpy bump and can get that in. |
|
@jreback |
|
pytz i agree |
|
@jreback
Haven't found other enforced minimum versions. |
#24942 wanted to update the PY2-CI to newer python versions, but @jreback ultimately wanted to split this off. This takes the open changes from #24942.
Quoting from the OP there: