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

DEPS: Bump numpy to 1.13.3 #25554

Merged
merged 25 commits into from
Mar 28, 2019
Merged

DEPS: Bump numpy to 1.13.3 #25554

merged 25 commits into from
Mar 28, 2019

Conversation

h-vetinari
Copy link
Contributor

@h-vetinari h-vetinari commented Mar 5, 2019

#25227 is trying to add pyproject.toml which is necessary for the whole pip-machinery to work smoothly post v.19.0. However, this seems to be running into problems with a numpy version bump, and mood there was to bump numpy requirement to 1.13.x.

The bare minimum for the bump is implemented in that PR, but a version bump requires getting rid of a bunch of compat code (and CI adjustments), otherwise the cruft just accumulates (it took me three PRs to clean out until 1.12).

Scipy is goint to bump to 1.13.3 with the next release (due to cython issues, I believe), so I thought I'll directly take it to 1.13.3 here.

I tried redistributing the CI jobs as best as I could. Here's the before/after:

EDIT: updated pins in #25554 (comment)

CI job: line number before this PR
ci/deps/azure-27-compat.yaml:10: numpy=1.12.0 numpy=1.13.3
ci/deps/azure-27-locale.yaml:10: numpy=1.12.0 numpy=1.14.*
ci/deps/azure-36-locale_slow.yaml:16: numpy numpy=1.15.*
ci/deps/azure-37-locale.yaml:15: numpy numpy
ci/deps/azure-37-numpydev.yaml:18: numpy numpy
ci/deps/azure-macos-35.yaml:14: numpy=1.12.0 numpy=1.13.3
ci/deps/azure-windows-27.yaml:15: numpy=1.12* numpy=1.13.3
ci/deps/azure-windows-36.yaml:12: numpy=1.14* numpy=1.15.*
ci/deps/travis-27.yaml:20: numpy=1.13* numpy=1.14.*
ci/deps/travis-36-doc.yaml:23: numpy=1.13* numpy
ci/deps/travis-36-locale.yaml:15: numpy numpy
ci/deps/travis-36-slow.yaml:12: numpy numpy
ci/deps/travis-36.yaml:17: numpy numpy=1.15.*
ci/deps/travis-37.yaml:10: numpy numpy

Matplotlib needs to be bumped as well (since current min 2.0.0 cannot be resolved by conda together with numpy 1.13.3), but it's only a tiny bump to 2.0.2 2.1.0 2.1.1 2.2.2. In any case, the version spread should probably start to reflect that there's a bunch of mpl 3.0.x about, and so I also changed the spread a bit here:

CI job: line number before this PR
ci/deps/azure-27-locale.yaml:9: matplotlib=2.0.0 matplotlib=2.2.2
ci/deps/azure-36-locale_slow.yaml:13: matplotlib matplotlib=3.0.*
ci/deps/azure-37-locale.yaml:12: matplotlib matplotlib
ci/deps/azure-macos-35.yaml:11: matplotlib=2.2.0 matplotlib=2.2.3
ci/deps/azure-windows-27.yaml:13: matplotlib=2.0.1 matplotlib=2.2.4
ci/deps/azure-windows-36.yaml:10: matplotlib matplotlib
ci/deps/travis-27.yaml:16: matplotlib=2.2.2 matplotlib=2.2.*
ci/deps/travis-36-doc.yaml:17: matplotlib matplotlib
ci/deps/travis-36-locale.yaml:12: matplotlib matplotlib=3.0.1
ci/deps/travis-36-slow.yaml:10: matplotlib matplotlib
ci/deps/travis-36.yaml:14: matplotlib matplotlib=3.0.0

It's probably wishful thinking that everything will pass straight away (especially when mpl versions change, see the code in pandas/tests/plotting/test_datetimelike.py), but oh well...

@rgommers
Copy link
Contributor

rgommers commented Mar 5, 2019

Scipy is goint to bump to 1.13.3 with the next release (due to cython issues, I believe), so I thought I'll directly take it to 1.13.3 here.

Not due to Cython issues, just to reduce the maintenance burden. See https://mail.python.org/pipermail/scipy-dev/2018-November/023181.html for rationale.

+1 for going to 1.13.3 as well. Matplotlib is doing a similar thing (actually they want to be a bit more agressive), and wants to standardize a common Python/NumPy version support policy for core packages (see scipy/scipy#9610, we'll come back on that soon).

@gfyoung gfyoung added the Dependencies Required and optional dependencies label Mar 6, 2019
@jschendel
Copy link
Member

install.rst also needs a small update:

* `NumPy <http://www.numpy.org>`__: 1.12.0 or higher

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #25554 into master will decrease coverage by 0.48%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25554      +/-   ##
==========================================
- Coverage   91.75%   91.26%   -0.49%     
==========================================
  Files         173      173              
  Lines       52960    52961       +1     
==========================================
- Hits        48595    48337     -258     
- Misses       4365     4624     +259
Flag Coverage Δ
#multiple 89.83% <100%> (-0.51%) ⬇️
#single 41.73% <75%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/categorical.py 95.96% <ø> (ø) ⬆️
pandas/core/arrays/sparse.py 92.17% <ø> (ø) ⬆️
pandas/compat/numpy/__init__.py 93.1% <100%> (-0.23%) ⬇️
pandas/plotting/_compat.py 87.5% <100%> (ø) ⬆️
pandas/core/groupby/generic.py 86.98% <100%> (-0.02%) ⬇️
pandas/core/arrays/numpy_.py 94.49% <100%> (+0.83%) ⬆️
pandas/core/panel.py 38.56% <0%> (-33.19%) ⬇️
pandas/core/sparse/series.py 93.3% <0%> (-2.24%) ⬇️
pandas/core/indexing.py 90.88% <0%> (-1.41%) ⬇️
pandas/core/internals/managers.py 93.92% <0%> (-0.92%) ⬇️
... and 5 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 d10bbce...5b22257. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #25554 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25554      +/-   ##
==========================================
+ Coverage   91.53%   91.55%   +0.02%     
==========================================
  Files         175      175              
  Lines       52808    52775      -33     
==========================================
- Hits        48338    48320      -18     
+ Misses       4470     4455      -15
Flag Coverage Δ
#multiple 90.12% <100%> (+0.02%) ⬆️
#single 41.82% <71.42%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/missing.py 93.96% <ø> (+1.39%) ⬆️
pandas/util/_test_decorators.py 93.22% <ø> (+0.57%) ⬆️
pandas/core/arrays/datetimes.py 97.79% <ø> (-0.01%) ⬇️
pandas/core/arrays/categorical.py 95.96% <ø> (ø) ⬆️
pandas/core/arrays/sparse.py 92.17% <ø> (ø) ⬆️
pandas/core/computation/check.py 92.3% <100%> (ø) ⬆️
pandas/core/nanops.py 93.84% <100%> (ø) ⬆️
pandas/core/arrays/numpy_.py 94.49% <100%> (+0.83%) ⬆️
pandas/compat/numpy/__init__.py 93.1% <100%> (-0.23%) ⬇️
pandas/core/groupby/generic.py 87.03% <100%> (-0.02%) ⬇️
... and 4 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 882961d...20ad5a5. Read the comment docs.

README.md Outdated Show resolved Hide resolved
ci/deps/azure-36-locale_slow.yaml Show resolved Hide resolved
ci/deps/travis-27.yaml Outdated Show resolved Hide resolved
ci/deps/travis-36.yaml 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.

@jreback
Regarding pins, please have a look at the OP. Bumping numpy is always a good time to look at the distribution in the CI.

For numpy, there's currently 3x 1.12, 2x 1.13, 1x 1.14, ZERO 1.15 and 7 unpinned.
I'm proposing to make this 3x 1.13, 2x 1.14, 2x 1.15 and 6 unpinned.

For matplotlib, there's currently 2.0.0, 2.0.1, 2.2.0, 2.2.2 and 7 unpinned.
I'm proposing to make this 2.0.2 (new min), 2.1.0, 2.2.0, 2.2.* (LTS!), 3.0.0, 3.0.1, 3.0.* and 4 unpinned.

README.md Outdated Show resolved Hide resolved
ci/deps/azure-36-locale_slow.yaml Show resolved Hide resolved
ci/deps/travis-27.yaml Outdated Show resolved Hide resolved
ci/deps/travis-36.yaml Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

I like @h-vetinari's proposed pins in
#25554 (comment)

@h-vetinari
Copy link
Contributor Author

@jreback @TomAugspurger
This should be done now. :)

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

LGTM.

@h-vetinari if you're interested, I believe that this will let us delete the following on SparseArray / SparseSeries

  • __array_wrap__
  • __array_prepare__
  • __array_priority__

Those are disabled when the class defines __array_ufunc__, which was started in NumPy 1.13

@h-vetinari
Copy link
Contributor Author

@TomAugspurger
That sounds like a separate PR?

@jreback
PTAL

@h-vetinari
Copy link
Contributor Author

@TomAugspurger: @h-vetinari if you're interested [...]

@h-vetinari: @TomAugspurger That sounds like a separate PR?

I've got an initial draft prepared, but will wait until this one is merged.

@jreback
Copy link
Contributor

jreback commented Mar 10, 2019

this needs to wait until 0.24.2 is done and needs review

@@ -11,10 +11,10 @@ dependencies:
- gcsfs
- geopandas
- html5lib
- matplotlib
- matplotlib=3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason this is pinned exactly to 3.0.0, which was superseded immedately by 3.0.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were almost three months between 3.0.0 and 3.0.1, see https://github.com/matplotlib/matplotlib/releases.

I just tried to spread some pins throughout the 3.x Series, see OP.

@@ -53,8 +53,10 @@ def setup_method(self, method):
import matplotlib as mpl
mpl.rcdefaults()

self.mpl_ge_2_0_1 = plotting._compat._mpl_ge_2_0_1()
self.mpl_eq_2_0_2 = plotting._compat._mpl_eq_2_0_2()
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 just bump mpl to 2.1 min to avoid all of these extra things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do, but the special casing for 2.2.0 will still be necessary, unless we bump to 2.2.2

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @tacaswell any thoughts here?

@jreback jreback added this to the 0.25.0 milestone Mar 10, 2019
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 For pins, please check OP. Happy to change however you want. Will bump matplotlib pending further input from you (already to 2.2.2?)

@@ -11,10 +11,10 @@ dependencies:
- gcsfs
- geopandas
- html5lib
- matplotlib
- matplotlib=3.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were almost three months between 3.0.0 and 3.0.1, see https://github.com/matplotlib/matplotlib/releases.

I just tried to spread some pins throughout the 3.x Series, see OP.

@@ -53,8 +53,10 @@ def setup_method(self, method):
import matplotlib as mpl
mpl.rcdefaults()

self.mpl_ge_2_0_1 = plotting._compat._mpl_ge_2_0_1()
self.mpl_eq_2_0_2 = plotting._compat._mpl_eq_2_0_2()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do, but the special casing for 2.2.0 will still be necessary, unless we bump to 2.2.2

@jreback jreback mentioned this pull request Mar 10, 2019
@h-vetinari
Copy link
Contributor Author

@jreback
Checked the MPL releases, and 2.2.2 is barely a year old. Therefore, I bumped only to 2.1.0.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 11, 2019

@jreback
OK, nevermind, I went to matplotlib 2.2.2 after all. By the time pandas 0.24.2 hits, this will be over a year old, and there's just too much variability in the MPL point releases for 2.1.x so that any starting point in 2.0.1 | 2.0.2 | 2.1.0 | 2.1.1 | 2.1.2 (and to a degree 2.2.0) would lead to lots of compat code (along the lines of what you didn't like above). And since 2.2.2 was only released 10 days after 2.2.0, I went to 2.2.2 right away.

@tacaswell
Copy link
Contributor

Sorry for the Matplotlib API thrashing.

2.2.x is the LTS version so it is a good choice!

@h-vetinari
Copy link
Contributor Author

@jreback
I keep running into a weird cython error on the azure-windows-27 job:

running build_ext
pandas._libs.algos: -> [['pandas\\_libs/algos.c']]
Traceback (most recent call last):
  File "setup.py", line 751, in <module>
    **setuptools_kwargs)
  File "C:\Miniconda\lib\site-packages\setuptools\__init__.py", line 143, in setup
    return distutils.core.setup(**attrs)
  File "C:\Miniconda\lib\distutils\core.py", line 148, in setup
    dist.run_commands()
  File "C:\Miniconda\lib\distutils\dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "C:\Miniconda\lib\distutils\dist.py", line 985, in run_command
    cmd_obj.run()
  File "C:\Miniconda\lib\distutils\command\build_ext.py", line 339, in run
    self.build_extensions()
  File "setup.py", line 372, in build_extensions
    self.check_cython_extensions(self.extensions)
  File "setup.py", line 369, in check_cython_extensions
    """.format(src=src))
Exception: Cython-generated file 'pandas\_libs/algos.c' not found.
                Cython is required to compile pandas from a development branch.
                Please install Cython or download a release package of pandas.

Bumping cython didn't help (resp. then the 3.5 jobs can't satisfy min). How about we drop the PY2 CI (#24942) first before bumping numpy?

@jreback jreback mentioned this pull request Mar 14, 2019
@WillAyd
Copy link
Member

WillAyd commented Mar 19, 2019

@h-vetinari can you merge master on this now that Py2 CI jobs are out?

@h-vetinari
Copy link
Contributor Author

@WillAyd: @h-vetinari can you merge master on this now that Py2 CI jobs are out?

Since #24942 was split up, this PR here still depends on #25752. It would be helpful if you could give the latter one a look. :)

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 23, 2019

Currently rebasing on top of #25752. This is the current set of pins for python/numpy/matplotlib. Comments welcome.

CI job: line number OS python numpy mpl
azure-35-compat.yaml:10: posix python=3.5.* numpy=1.13.3 -
azure-36-locale.yaml:10: posix python=3.6.* numpy=1.14.* matplotlib=2.2.2
azure-36-locale_slow.yaml:16: posix python=3.6.* numpy=1.15.* matplotlib=3.0.*
azure-37-locale.yaml:15: posix python=3.7.* numpy matplotlib
azure-37-numpydev.yaml:18: posix python=3.7.* numpy -
azure-macos-35.yaml:14: macos python=3.5.* numpy=1.13.3 matplotlib=2.2.3
azure-windows-36.yaml:15: win python=3.6.* numpy=1.15.* matplotlib=3.0.2
azure-windows-37.yaml:12: win python=3.7.* numpy=1.14.* matplotlib=2.2.*
travis-36-cov.yaml:17: posix python=3.6.* numpy=1.15.* matplotlib
travis-36-doc.yaml:23: posix python=3.6.* numpy matplotlib
travis-36-locale.yaml:15: posix python=3.6.* numpy matplotlib=3.0.0
travis-36-slow.yaml:12: posix python=3.6.* numpy matplotlib
travis-37.yaml:10: posix python=3.7.* numpy -

PS. Note that matplotlib 3.0.1 was superseded by 3.0.2 within a day.

@jreback
Copy link
Contributor

jreback commented Mar 23, 2019

see if we can remove 1 or more jobs keeping the same coverage (reasonably) of deps (can be a follow up)
i don’t think it’s a big deal if we only test oldest numpy and then newest
1.14/1.15 or pretty much compat with 1.16

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 23, 2019

Well, I didn't design the CI jobs - there's OSes, numpies, locales, DB tests. Not sure we should reduce much more, but from the table above, I'd get rid of travis-3.6-locale.

Maybe @TomAugspurger @jorisvandenbossche want to opine?

Also, we still need to actually merge #25752.

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.

ok looks good. don't make any more changes, except for chopping the whatsnew as indicated. ping on green.

ci/deps/azure-35-compat.yaml Show resolved Hide resolved
ci/deps/travis-36-locale.yaml Show resolved Hide resolved
Below is an overview of the minimal versions, as required resp. recommended. Optional libraries below the lowest tested version may still work, but are not considered supported.

+-----------------+-----------------+-----------------------+
| Package | Minimum Version | Comment |
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 too verbose. pls separate into 2 tables, the basic deps (numpy, pytz, bottleneck, numexpr); make the table like we had it exactly

you can make a table for other things, but i am going to limit that list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept pytest due to #23519.

I kept the structure of the table because the distinction between the kind of requirement (hard requirement vs. requirement if installed) is clearer like this, and fits with the other table.

+-----------------+-----------------+-----------------------+
| numexpr | 2.6.2 | Required if installed |
+-----------------+-----------------+-----------------------+
| beautifulsoup4 | 4.4.1 | Lowest tested version |
Copy link
Contributor

Choose a reason for hiding this comment

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

remove beautifulsoup, blosc, gcfs, junja2, lxml, psycopg2, pymsql, s3fs, cython and pytest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pytest was explicitly added to this table by #23519.

I added all the libraries from https://pandas-docs.github.io/pandas-docs-travis/install.html#dependencies. I think this is a worthwhile overview to have somewhere (maybe not the whatsnew). Thoughts, @datapythonista?

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.

Will follow up later today, no time right now.

ci/deps/azure-35-compat.yaml Show resolved Hide resolved
ci/deps/travis-36-locale.yaml Show resolved Hide resolved
+-----------------+-----------------+-----------------------+
| numexpr | 2.6.2 | Required if installed |
+-----------------+-----------------+-----------------------+
| beautifulsoup4 | 4.4.1 | Lowest tested version |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pytest was explicitly added to this table by #23519.

I added all the libraries from https://pandas-docs.github.io/pandas-docs-travis/install.html#dependencies. I think this is a worthwhile overview to have somewhere (maybe not the whatsnew). Thoughts, @datapythonista?

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.

PTAL

Below is an overview of the minimal versions, as required resp. recommended. Optional libraries below the lowest tested version may still work, but are not considered supported.

+-----------------+-----------------+-----------------------+
| Package | Minimum Version | Comment |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept pytest due to #23519.

I kept the structure of the table because the distinction between the kind of requirement (hard requirement vs. requirement if installed) is clearer like this, and fits with the other table.

@h-vetinari
Copy link
Contributor Author

@jreback
Argh, now we're right back to the errors I had fixed with the regex stuff that you reverted against my request. #25874

r"')|"
r"unorderable types: int\(\) > datetime\.datetime\(\)")
msg = ("unorderable types: ({0} [<>] {1}|{1} [<>] {0})".format(
r"int\(\)", r"datetime\.datetime\(\)") # noqa: E126
Copy link
Contributor

Choose a reason for hiding this comment

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

this fails the wheel building CI. pls don't revert this, I don't want to debug this problem. Just make this MUCH simpler so it will pretty much always pass.

Copy link
Contributor Author

@h-vetinari h-vetinari Mar 27, 2019

Choose a reason for hiding this comment

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

It will not fail the wheel building CI. I actually checked the wheel builder failures from #25867 before giving my review in #25874.

In case you don't have time to read the review, the original code in #25752 has an if _np_version_under1p14 else-switch that turned out to be wrong, while the regex here only uses a |-conjunction.

[...] I don't want to debug this problem.

Neither did I, but I've had to for a passing CI. Please have a look at the actual regexes. What I've written is a proper superset of the current regexes (again, now that the switch has been turned into "|").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just make this MUCH simpler so it will pretty much always pass.

Done.

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.

pls just make the changes I indicated. otherwise this iteration cycle will continue to extend.

+-----------------+-----------------+-----------------------+
| numexpr | 2.6.2 | Required if installed |
+-----------------+-----------------+-----------------------+
| pytest | 4.0.2 | Development-only req. |
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 a misleading comment for pytest. its required if you run tests

Require if installed is very misleading, remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it. That being said, pytest is not necessary to run pandas, so it is "as required" as, say openpyxl to write excel files.

The following table lists the lowest version per library that is currently being tested throughout the development of pandas.
Optional libraries below the lowest tested version may still work, but are not considered supported.

+-----------------+-----------------+-----------------------+
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the comment column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Updated. Please take the time to read my comment about the regex.

+-----------------+-----------------+-----------------------+
| numexpr | 2.6.2 | Required if installed |
+-----------------+-----------------+-----------------------+
| pytest | 4.0.2 | Development-only req. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it. That being said, pytest is not necessary to run pandas, so it is "as required" as, say openpyxl to write excel files.

The following table lists the lowest version per library that is currently being tested throughout the development of pandas.
Optional libraries below the lowest tested version may still work, but are not considered supported.

+-----------------+-----------------+-----------------------+
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

r"')|"
r"unorderable types: int\(\) > datetime\.datetime\(\)")
msg = ("unorderable types: ({0} [<>] {1}|{1} [<>] {0})".format(
r"int\(\)", r"datetime\.datetime\(\)") # noqa: E126
Copy link
Contributor Author

@h-vetinari h-vetinari Mar 27, 2019

Choose a reason for hiding this comment

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

It will not fail the wheel building CI. I actually checked the wheel builder failures from #25867 before giving my review in #25874.

In case you don't have time to read the review, the original code in #25752 has an if _np_version_under1p14 else-switch that turned out to be wrong, while the regex here only uses a |-conjunction.

[...] I don't want to debug this problem.

Neither did I, but I've had to for a passing CI. Please have a look at the actual regexes. What I've written is a proper superset of the current regexes (again, now that the switch has been turned into "|").

@jreback
Copy link
Contributor

jreback commented Mar 28, 2019

It will not fail the wheel building CI. I actually checked the wheel builder failures from #25867 before giving my review in #25874.

not sure what you are talking about. it DID fail. The reverted does not. If you fixed it great. I want to avoid having broken CI's endlessly.

@jreback jreback merged commit 68dd979 into pandas-dev:master Mar 28, 2019
@jreback
Copy link
Contributor

jreback commented Mar 28, 2019

thanks

@jreback
Copy link
Contributor

jreback commented Mar 28, 2019

@h-vetinari looks like this broke the gbq tests; these are pretty funky and I don't think are tested on actual branches (thoguh they used to be AFAIR): see https://travis-ci.org/pandas-dev/pandas/jobs/512494733

so pls do a PR to disable for now.

@h-vetinari
Copy link
Contributor Author

@jreback: not sure what you are talking about. it DID fail. The reverted does not. If you fixed it great. I want to avoid having broken CI's endlessly.

I did fail, and I had fixed it (not by reverting exactly, but then I've tried to explain that already twice). Now it's more simplified anyway, which is IMO anyway a better approach for matching these error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Required and optional dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants