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

Remove py27 CI jobs #24942

Merged
merged 59 commits into from
Mar 17, 2019
Merged

Remove py27 CI jobs #24942

merged 59 commits into from
Mar 17, 2019

Conversation

h-vetinari
Copy link
Contributor

The start of a looooooooong process to get rid of the py2 compat code.

I kept most of the CI jobs that used to be 2.7 at the lower end of the various supported versions, with 2x 2.7->3.5, 1x 2.7->3.6 and only one real upgrade from 2.7->3.7 (first CI job for 3.7 & windows).

@jreback @TomAugspurger

@jreback
Copy link
Contributor

jreback commented Jan 26, 2019

keep in mind that we likely won't merge anything PY2 removal for a month or so.

@h-vetinari
Copy link
Contributor Author

keep in mind that we likely won't merge anything PY2 removal for a month or so.

Which makes a lot of sense from a backports POV, but I didn't think about that. Guess I just couldn't/can't wait. ;)

@gfyoung gfyoung added Build Library building on various platforms 2/3 Compat labels Jan 26, 2019
@gfyoung gfyoung added this to the 0.25.0 milestone Jan 26, 2019
@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

Merging #24942 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24942      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52420    52388      -32     
==========================================
- Hits        48423    48396      -27     
+ Misses       3997     3992       -5
Flag Coverage Δ
#multiple 90.8% <ø> (ø) ⬆️
#single 42.9% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/io/clipboard/clipboards.py 28.23% <0%> (-2.36%) ⬇️
pandas/core/indexes/base.py 96.32% <0%> (-0.23%) ⬇️
pandas/core/reshape/merge.py 94.4% <0%> (-0.08%) ⬇️
pandas/core/indexes/multi.py 95.59% <0%> (-0.02%) ⬇️
pandas/core/indexes/range.py 97.36% <0%> (-0.01%) ⬇️
pandas/core/indexes/datetimes.py 96.27% <0%> (-0.01%) ⬇️
pandas/core/resample.py 97.23% <0%> (-0.01%) ⬇️
pandas/core/generic.py 96.63% <0%> (-0.01%) ⬇️
pandas/core/tools/timedeltas.py 98% <0%> (ø) ⬆️
pandas/core/tools/numeric.py 100% <0%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb43726...b88b87d. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

Merging #24942 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24942      +/-   ##
==========================================
- Coverage   91.25%   91.25%   -0.01%     
==========================================
  Files         172      172              
  Lines       52976    52976              
==========================================
- Hits        48342    48341       -1     
- Misses       4634     4635       +1
Flag Coverage Δ
#multiple 89.82% <ø> (ø) ⬆️
#single 41.74% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/util/testing.py 88.98% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a61d823...fe83f86. Read the comment docs.

@h-vetinari
Copy link
Contributor Author

@jreback @TomAugspurger
The last couple of builds have had weird (but consistent) travis errors in the travis_35 job, where the worker nodes crash after about ~10 sec (so I'm thinking it's not related to actual tests segfaulting, but somehow the interplay between different versions of "infrastructure packages).

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

@h-vetinari this is again changing too much. I want only the actual env files to be changed. you are changing all kinds of CI things and something is still breaking. I would start anew and simply remove the PY2 builds and leave the actual updates of versions to another PR.

@@ -3,14 +3,13 @@ channels:
- defaults
- conda-forge
dependencies:
- backports.lzma
Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@@ -19,33 +18,40 @@ dependencies:
- numexpr
- numpy=1.13*
- openpyxl=2.4.0
- pathlib
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed as its builtin

- hypothesis>=3.58.0
- isort
# satisfy fastparquet deps with conda
Copy link
Contributor

Choose a reason for hiding this comment

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

i would rather remove fastparquet from this build then, we have it installed for 1 or more of 3.6/3.7 already I think (pls check)

@@ -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(PY35 and not PY36, reason='only fails on Linux?',
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wasn't able to make this test work on PY35 regardless of the versions I tried.

@@ -28,7 +28,7 @@
import pytest

import pandas.compat as compat
from pandas.compat import PY2, PY36, lrange, range, string_types
from pandas.compat import PY2, PY35, PY36, lrange, range, string_types
Copy link
Contributor

Choose a reason for hiding this comment

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

really really don't change things, if you are forcing chanegs then roll back the env changes which cause this.

@@ -11,8 +11,9 @@

from pandas._libs import (
algos as libalgos, groupby as libgroupby, hashtable as ht)
from pandas.compat import PY2, lrange, range
from pandas.compat.numpy import np_array_datetime64_compat
from pandas.compat import lrange, range
Copy link
Contributor

Choose a reason for hiding this comment

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

see my comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, if I don't fix the regex here, the tests will fail.

Copy link
Contributor Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

@jreback
Pushing some changes based on your review. This PR contains some xfails that I wasn't able to solve so far, as well as two changes to regexes that would otherwise fail. If the goal is a passing CI, those are unavoidable.

@@ -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(PY35 and not PY36, reason='only fails on Linux?',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wasn't able to make this test work on PY35 regardless of the versions I tried.

@@ -11,8 +11,9 @@

from pandas._libs import (
algos as libalgos, groupby as libgroupby, hashtable as ht)
from pandas.compat import PY2, lrange, range
from pandas.compat.numpy import np_array_datetime64_compat
from pandas.compat import lrange, range
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, if I don't fix the regex here, the tests will fail.

- hypothesis>=3.58.0
- isort
# satisfy fastparquet deps with conda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Remaining jobs with fastparquet:

ci/deps\azure-windows-36.yaml:9:  - fastparquet>=0.2.1
ci/deps\travis-36-doc.yaml:9:  - fastparquet>=0.2.1
ci/deps\travis-36.yaml:10:  - fastparquet>=0.2.1

@jorisvandenbossche
Copy link
Member

@h-vetinari can you point to the test errors for which you needed to add the additional xfails? (or do a new build without any of them) Ideally, we should investigate each of those in more detail why they are failing instead of just xfailing them (unless you already know the reason for each of them)

@h-vetinari
Copy link
Contributor Author

@jorisvandenbossche
The fails are distributed over the CI runs throughout the thread, but I've reverted them in the last commit (including the regex fixes @jreback wanted me to revert), so then there's a single place to inspect the failures. Also - who knows - maybe some of them resolved themselves...

@h-vetinari
Copy link
Contributor Author

@jorisvandenbossche @TomAugspurger @jreback
Please check out https://travis-ci.org/pandas-dev/pandas/jobs/506535600, it fails already in the single stage with the tests I would have xfailed. When I first started this PR I played around with bumping the versions of xlrd etc (even when unpinning pymysql/xlsxwriter), but that didn't solve things either.

Considering the fact that removing support for PY35 is already being talked about in #25725, I think a small handful of xfailed tests is not a big burden. Certainly those are less trivial to fix than for example the skips that have been added for regex fails (of which there about 30).

@jreback
Copy link
Contributor

jreback commented Mar 15, 2019

as i said above pls start over and JUST remove the 2.7 jobs; since things are working now this will just work. after that you can do independent PR to add new / change things.

something is breaking and certainly needs fixing
but not here

@jreback
Copy link
Contributor

jreback commented Mar 15, 2019

PY35 is not being removed at this time

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 15, 2019

@jreback: as i said above pls start over and JUST remove the 2.7 jobs; since things are working now this will just work. after that you can do independent PR to add new / change things.

Of course it's easier to just rip things out, but I actually went to the effort here of thinking of a good distribution for the CI, updating the jobs, and making them pass again.

That's also how the last numpy bump was made. Tbh, I feel these conditions are quite arbitrary, but well... I'll delete the former PY2 jobs later today, and post the fixes in a separate PR.

PY35 is not being removed at this time

I know that. But adding 6-7 PY35-specific xfails (for non-core functionality) aren't that severe IMO, considering the point in the lifecycle.

@jreback
Copy link
Contributor

jreback commented Mar 15, 2019

@h-vetinari as I have explained many many times. PRs which do lots of things take way more time to review and are much harder to merge. It is the same case here.

@h-vetinari
Copy link
Contributor Author

@jreback
This is green now. If only it always were as easy as deleting CI jobs, haha.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

just the 1 comment

doc/source/whatsnew/v0.25.0.rst Show resolved Hide resolved
Copy link
Contributor Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Sorry, somehow didn't get a notification for the first comment.

doc/source/whatsnew/v0.25.0.rst Show resolved Hide resolved
@jreback jreback merged commit 707c720 into pandas-dev:master Mar 17, 2019
@jreback
Copy link
Contributor

jreback commented Mar 17, 2019

thanks, would also appreciate a PR against : https://github.com/Macpython/pandas-wheels (the daily branch, not master).

@h-vetinari
Copy link
Contributor Author

thanks, would also appreciate a PR against : https://github.com/Macpython/pandas-wheels (the daily branch, not master).

This same PR or the PY35 one?

@jreback
Copy link
Contributor

jreback commented Mar 17, 2019

neither. one to remove the PY2 jobs only (this is where we build wheels), and is our daily testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants