Preparing to release bottleneck 1.3.0

kwgoodman commented 6 years ago

I am getting ready to release bottleneck 1.2.2. The only thing left to do is testing.

The following people gave test reports on the pre-release of bottleneck 1.2.1, so I'm pinging you again in case you have time to test this release (the master branch): @cgohlke @itdaniher @toobaz @shoyer and anyone else.

shoyer commented 6 years ago

Xarray's CI is passing with the development version of bottleneck, so I don't anticipate any troubles here: https://travis-ci.org/pydata/xarray/jobs/385018063

cgohlke commented 6 years ago

All builds and test are passing on Windows with numpy 1.14.3

vkhodygo commented 5 years ago

I've tried to build an Arch package and I get this:

bottleneck/src/move.c: In function ‘move_rank_int64’: bottleneck/src/move.c:2009:24: warning: self-comparison always evaluates to true [-Wtautological-compare] if (aj == aj) { ^~ bottleneck/src/move.c: In function ‘move_rank_int32’: bottleneck/src/move.c:2070:24: warning: self-comparison always evaluates to true [-Wtautological-compare] if (aj == aj) {

It also seems that my makepkg options conflict with your default compiler flags because I see such cryptic messages: gcc -pthread -DNDEBUG -g -fwrapv -O3 -Wall -march=native -O2 -pipe -fstack-protector-strong -fno-plt -D_FORTIFY_SOURCE=2 -fPIC -I/usr/lib/python3.7/site-packages/numpy/core/include -I/usr/include/python3.7m -c bottleneck/src/reduce.c -o build/temp.linux-x86_64-3.7/bottleneck/src/reduce.o -O2 gcc -pthread -shared -Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -march=native -O2 -pipe -fstack-protector-strong -fno-plt -D_FORTIFY_SOURCE=2 build/temp.linux-x86_64-3.7/bottleneck/src/reduce.o -L/usr/lib -lpython3.7m -o build/lib.linux-x86_64-3.7/bottleneck/reduce.cpython-37m-x86_64-linux-gnu.so

kwgoodman commented 5 years ago

The self comparison aj == aj is a check for NaN. I do not see that warning when I compile.

Does bottleneck run and do the unit tests pass?

vkhodygo commented 5 years ago

I use gcc 8.2.1 + CPPFLAGS="-D_FORTIFY_SOURCE=2" CFLAGS="-march=native -O2 -pipe -fstack-protector-strong -fno-plt" LDFLAGS="-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now"

OK, i tried a few combinations, here what I get: numpy + MKL + git version of the package built with gcc -- OK (tests and benchmarks work) numpy + MKL + latest stable release from the repository -- Test works, but throws a warning, benchmark fails. Now I get this:

/usr/lib/python3.7/site-packages/bottleneck/slow/move.py:149: FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; usearr[tuple(seq)]instead ofarr[seq]. In the future this will be interpreted as an array index,arr[np.array(seq)], which will result either in an error or a different result. nidx1 = n[idx1] /usr/lib/python3.7/site-packages/bottleneck/slow/move.py:150: FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; usearr[tuple(seq)]instead ofarr[seq]. In the future this will be interpreted as an array index,arr[np.array(seq)], which will result either in an error or a different result. nidx1 = nidx1 - n[idx2] /usr/lib/python3.7/site-packages/bottleneck/slow/move.py:152: FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; usearr[tuple(seq)]instead ofarr[seq]. In the future this will be interpreted as an array index,arr[np.array(seq)], which will result either in an error or a different result. idx[idx1] = nidx1 < min_count /usr/lib/python3.7/site-packages/bottleneck/slow/move.py:153: FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; usearr[tuple(seq)]instead ofarr[seq]. In the future this will be interpreted as an array index,arr[np.array(seq)], which will result either in an error or a different result. idx[idx3] = n[idx3] < min_count

nanrankdata 55.7 1.2 1.1 2.6 2.5 Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python3.7/site-packages/bottleneck/benchmark/bench.py", line 77, in bench speed = timer(test['statements'], test['setups']) File "/usr/lib/python3.7/site-packages/bottleneck/benchmark/bench.py", line 87, in timer t0 = autotimeit(statements[0], setup) File "/usr/lib/python3.7/site-packages/bottleneck/benchmark/autotimeit.py", line 7, in autotimeit number, time1 = autoscaler(timer, mintime) File "/usr/lib/python3.7/site-packages/bottleneck/benchmark/autotimeit.py", line 15, in autoscaler time = timer.timeit(number) File "/usr/lib/python3.7/timeit.py", line 176, in timeit timing = self.inner(it, self.timer) File "<timeit-src>", line 16, in inner TypeError:nmust be an integer

Regular numpy & bottleneck from the repos -- same problem with the test and benchmark. Regular numpy + git bottleneck -- OK.

kwgoodman commented 5 years ago

OK, good. Those issues have been fixed in the master branch (and confirmed by your results).

kwgoodman commented 5 years ago

I will make a release soon in case anyone wants to test.

itdaniher commented 5 years ago

Running unit tests for bottleneck
NumPy version 1.13.3
NumPy relaxed strides checking option: True
NumPy is installed in /usr/lib/python3/dist-packages/numpy
Python version 3.6.7 (default, Oct 22 2018, 11:32:17) [GCC 8.2.0]
nose version 1.3.7
...........................................................................................................                                                  ..............................................................
-
Ran 169 tests in 472.178s

@ 825c1169d090805ffb6b9024bff99815a0f7c723 on ARM64 Ubuntu 18.04

stas00 commented 5 years ago

Please test pandas with the bottleneck and numpy (all pip wheels!), see the problem I have just reported here: https://github.com/kwgoodman/bottleneck/issues/204

vkhodygo commented 5 years ago

@stas00 no problems with the dev release (please, note, that I use Arch, therefore, my versions of packages may be slightly different).

stas00 commented 5 years ago

Thanks, @V-Kh,

Do note that if I build 1.2.1 from scratch with pip the problem is not there. The problem appears only if installed via the wheel on pypi. So I think in order to test this properly a wheel needs to built in exactly the same way and on the same system that the final release is made on.

and then tested against:

conda create --name bottleneck python=3.7
conda activate bottleneck
pip install pandas 
pip install bottleneck-new.whl
python -c "import pandas"

of course, it'd also help if you can reproduce the problem first as described in https://github.com/kwgoodman/bottleneck/issues/204, since if you can't, then you aren't testing the same thing.

Thank you.

kwgoodman commented 5 years ago

@itdaniher ARM? Nice! Raspberry pi?

@stas00 bottleneck should be compiled against the same version of numpy as it is used with. (Or is it just that the numpy C-API shouldn't change between the two numpy version? I don't know.) The fix made by @astrofrog (#203) makes it more likely that the same version of numpy is used.

stas00 commented 5 years ago

@stas00 bottleneck should be compiled against the same version of numpy as it is used with. (Or is it just that the numpy C-API shouldn't change between the two numpy version? I don't know.) The fix made by @astrofrog (#203) makes it more likely that the same version of numpy is used.

There is no problem when building from source, the issue https://github.com/kwgoodman/bottleneck/issues/204 is about the binary wheel that the bottleneck release manager put on pypi (https://pypi.org/project/Bottleneck/). As @astrofrog correctly indicated there is a potential compatibility issue https://github.com/kwgoodman/bottleneck/pull/203#issuecomment-452635407 and most likely a need to pin to the compatible numpy version range, ideally as wide as possible, otherwise there will be whole other set of troubles as other packages will have a conflict of versions in dependency requirements - if they don't match that range of numpy that bottleneck will pin to.

I see that 1.2.1 was released 2017-05-15, which is quite a long time ago, so perhaps any recent numpy will do? If that's the case then it's about finding the lowest numpy version that it should work with and then pin it in the requirements with numpy >= 1.14.0 (this version number is just a guess).

On the other hand I don't know anything about bottleneck to tell whether its binary will work with a range of numpy versions or whether it needs to be built against a specific version of numpy, in which case releasing a binary wheel becomes a problem no matter how you slice it. If it requires a specific numpy version, then it just won't work at all for when an environment includes packages that pinned to a different range of numpy versions. i.e. dependencies conflict.

kwgoodman commented 5 years ago

There is no problem when building from source, the issue #204 is about the binary wheel that the bottleneck release manager put on pypi (https://pypi.org/project/Bottleneck/).

I don't maintain a binary. Pypi only contains the source distribution: https://pypi.org/project/Bottleneck/#files

stas00 commented 5 years ago

I don't maintain a binary. Pypi only contains the source distribution

Hmm, you're correct, then why do we have this problem then? Have you tried reproducing it?

If I install from pypi, the problem is there. If I install from github (same release) there is no problem. That's why I incorrectly assumed it was a binary wheel.

The details to reproduce it are in the issue: https://github.com/kwgoodman/bottleneck/issues/204

Should be trivial to copy-n-paste to reproduce.

kwgoodman commented 5 years ago

I think it has been fixed by #203

stas00 commented 5 years ago

Except https://github.com/kwgoodman/bottleneck/pull/203 has its own issues as the last comment indicates. If I understand his comment correctly this will break bottleneck if it uses C-API that has changed between the two numpy minor versions.

pip install numpy==1.15
pip install bottleneck  # will build against 1.15 C-API
pip install numpy==1.16 # say once it's released and some module triggers an upgrade

So I will let @astrofrog give his recommendations on how to proceed.

My feeling is that bottleneck needs to handle this gracefully should such situation arrive. So when it loads numpy it should check which version it was built against and if it's not matching give users an error indicating what happened and how to remedy it.

vkhodygo commented 5 years ago

@stas00 OK, luckily, I had a VM installed, so I managed to follow your steps. In my case it was the most recent version of Fedora (29), however, it's not enough to just install a few python packages. You also need python headers and a compiler and I strongly suspect that you just have some outdated software because I don't have any errors.

stas00 commented 5 years ago

Not at all, everything is quite up-to-date here, @V-Kh.

As you can derive from https://github.com/kwgoodman/bottleneck/pull/203 it doesn't seem to be related to either python sources or compiler tools, but which specific numpy version it was built against.

I know why you couldn't reproduce it. Please see my follow up: https://github.com/kwgoodman/bottleneck/issues/204#issuecomment-453783059

So it was about the binary wheel all along, except about one built on a user's system and cached. I will try to come up with a better reproducible scenario.

astrofrog commented 5 years ago

The issue with #204 is the one I was trying to address with https://github.com/kwgoodman/bottleneck/pull/203 - namely that currently if you pip install bottleneck, setup_requires is honored by easy_install which doesn't understand that it shouldn't use numpy pre-releases and therefore compiles bottleneck against Numpy 1.16rc2 and then pip subsequently installs Numpy 1.15, causing issues. This is actually a problem with all packages that have setup_requires=['numpy'], not just bottleneck.

With the change in #203, pip always builds the package inside an isolated environment, and will therefore always build against the latest stable version of numpy (not the RC). So this is somewhat of an improvement, but does cause other issues - namely if an older version of Numpy is already present in the environment, it's ignored during the build, and you might end up with bottleneck compiled against Numpy 1.15 in an environment with Numpy 1.13.

The correct solution is to actually pin the Numpy version inside the pyproject.toml file. However, the oldest version of Numpy depends on Python version, so you need to actually do something like:

[build-system]
requires = [
    "wheel",
    "setuptools",
    "numpy==1.8.2; python_version<='3.4'",
    "numpy==1.9.3; python_version=='3.5'",
    "numpy==1.12.1; python_version=='3.6'",
    "numpy==1.13.1; python_version>='3.7'",
]

But unfortunately environment markers as above were only supported in very recent versions of pip, so this won't work for many users. The scipy developers actually added this file too soon before this was properly supported in pip so they removed the pyproject.toml file in https://github.com/scipy/scipy/pull/8735 (back in October 2017) and only now are they considering adding it back - see https://github.com/scipy/scipy/issues/8734.

So the situation is that using a file with pinning and environment markers as above is the correct way to go in the medium term, but it's just a question of whether you are comfortable requiring pip 18 and later. If not, then an alternative is to do what I've settled on in the end in fast-histogram, which is to do the following in setup.py:

try:
    import numpy
except ImportError:
    # We include an upper limit to the version because setup_requires is
    # honored by easy_install not pip, and the former doesn't ignore pre-
    # releases. It's not an issue if the package is built against 1.15 and
    # then 1.16 gets installed after, but it still makes sense to update the
    # upper limit whenever a new version of Numpy is released.
    setup_requires = ['numpy<1.16']
else:
    setup_requires = []

Essentially, we first check if Numpy is present, and if not, we only build against versions older than 1.16 to make sure we don't pick up the 1.16rc (that pinning can be updated to 1.17 once 1.16 is out).

Hopefully this should clarify the two possible paths forward - let me know if anything is unclear!

astrofrog commented 5 years ago

One small clarification - the following will probably work with the pyproject.toml approach:

pip install numpy==1.15
pip install bottleneck  # will build against 1.15 C-API
pip install numpy==1.16 # say once it's released and some module triggers an upgrade

because normally the Numpy C API has remained compatible going forward. Also note that the first line (pip install numpy==1.15) will have no effect anyway since bottleneck will be built in an isolated environment against the latest version of numpy (1.15). Issues would happen however if you did:

pip install numpy==1.14
pip install bottleneck  # will build against 1.15 C-API

with the pyproject.toml approach because then bottleneck will be compiled with a more recent version of numpy than is present in the environment (unless you pin to the earliest compatible version in pyproject.toml as mentioned in my previous comment).

itdaniher commented 5 years ago

@kwgoodman ARM64 yes - pi, more or less! Rock64!

stas00 commented 5 years ago

Thank you for this very detailed explanation of the situation, @astrofrog!

My personal view is that users should always have the latest install tools (pip/conda), so to me pinning any packages to recent versions of install tools shouldn't give a second thought, unless we know that there is some kind of potential problems with them. So my vote is for pinning to pip>=18.1.

Wrt users installing a lower numpy minor version after compiling bottleneck with a higher one, why can't this runtime conflict be checked right after numpy import? Check which C-API got loaded when you load numpy and if it's not compatible, then give a user a clear message:

"Package incompatibility error: bottleneck was compiled with numpy-1.15, but numpy-1.14 was loaded, please either upgrade your numpy to 1.15 or higher, or reinstall bottleneck, which will then get rebuilt against the currently installed numpy version."

In which case there is no requirement to pin for 1.18, with the advanced configuration. It will be less DWIM, but at least easy to resolve for users.

vkhodygo commented 5 years ago

OK, numpy 1.16 is out.

kwgoodman commented 5 years ago

Ha, now Travis and AppVeyor are failing because #203 is compiling bottleneck with numpy 1.16.0 but the unit tests are run with numpy 1.15.4. Waiting a few days (while minconda upgrades to 1.16.0) should hopefully fix that.

https://anaconda.org/anaconda/numpy

vkhodygo commented 5 years ago

@kwgoodman It seems to work fine w/ numpy 1.16.0, all benchmarks and tests work flawlessly. By the way, have you ever thought about making it even faster using heavy gcc/icc optimizations?

kwgoodman commented 5 years ago

All tests pass for me too. But until anaconda upgrades to 1.16 many users will run into #203. Plus I don't want to release if travis and appveyor or failing.

I figure the performance of various compiler flags are system dependent.

vkhodygo commented 5 years ago

I figure the performance of various compiler flags are system dependent.

Yes, however, for those who are really interested in performance, such option would be great.

kwgoodman commented 5 years ago

Give it a try. Add any flags you want in setup.py to extra_compile_args:

    ext = [Extension("bottleneck.reduce",
                     sources=["bottleneck/src/reduce.c"],
                     extra_compile_args=['-O2'])]...

stas00 commented 5 years ago

I figure the performance of various compiler flags are system dependent.

Yes, however, for those who are really interested in performance, such option would be great.

I second that. Yes, please!

If it can't be automated please give us a build-time option to be activated perhaps via env var.

kwgoodman commented 5 years ago

Conda is now at numpy 1.16.2: https://anaconda.org/anaconda/numpy Yet travis and appveyor are still pulling the 1.15 series. I'll try again in a few days.

kwgoodman commented 5 years ago

conda on python 2.7 now pulls numpy 1.16 so those unit tests pass, but on py3 it still pulls numpy 1.15 so unit test fail due to #203

qwhelan commented 4 years ago

Hi everyone - in case you didn't see https://github.com/pydata/bottleneck/issues/209, I'm the new maintainer of bottleneck.

I believe I'm getting close to cutting a release candidate for 1.3.0 but wanted to check in with everyone on a few outstanding issues:

Regarding pyproject.toml - both scipy and pandas seem to be using an identical approach. I'm proposing we do the exact same (including an AIX patch) here https://github.com/pydata/bottleneck/pull/247
- @astrofrog Given 8 months have passed, hopefully support for python_version is significantly more widespread. Please let me know if this is still a concern.
Related to the above, unimportable bottleneck extensions will result in instructions on how to resolve: https://github.com/pydata/bottleneck/pull/246
Documentation hosting: I'm leaning towards readthedocs.org or GitHub Pages, unless if PyData would like to host them
There are still some easy performance gains I'd like to ship (namely nanmin/nanmax)
Support for applying -O3 at a function level was added in https://github.com/pydata/bottleneck/pull/223 . It's not always a win, but a step towards getting more vectorized code in.
I'd like to get the asv coverage a bit more complete to ensure we haven't regressed
General style and documentation cleanup

Additionally, there are a few requests that I'm deferring to 1.4, such as improvements to numerical stability and new functions.

Please let me know if there's anything I've missed or if you're seeing any issues on master!

qwhelan commented 4 years ago

The 1.3.0rc1 release candidate has been on PyPI for about a week now. Still waiting on access to the conda-forge/bottleneck-recipe repo, but should have the release candidate available there soon as well.

cgohlke commented 4 years ago

The 1.3.0rc1 release candidate has been on PyPI for about a week now

Installing on Windows using the source distribution on PyPI fails. See https://github.com/pydata/bottleneck/pull/271

qwhelan commented 4 years ago

The 1.3.0rc2 release candidate is now on PyPI and fixes a packaging bug and adds explicit Python 3.8 support.

Thanks again to @cgohlke for catching and fixing!

qwhelan commented 4 years ago

The conda-forge version of 1.3.0rc2 is now available and can be installed with:

conda install -c conda-forge/label/rc_bottleneck bottleneck

qwhelan commented 4 years ago

The 1.3.0 release is available on PyPI and will be on conda-forge shortly. Thanks to everyone that submitted bugs and fixes!

astrofrog commented 4 years ago

@qwhelan - just to check, are there plans to build wheels for 1.3.0? Just in case you are interested in trying to automate this, you might want to check out the Azure Pipeline template that we've written here: https://github.com/OpenAstronomy/azure-pipelines-templates/ (I'm happy to help set this up if you like)

qwhelan commented 4 years ago

@astrofrog Potentially in a future release, as we're now using -O3 for significant vectorization speedups. Publishing a wheel would require automatic detection of SSE/AVX support or targeting a really old architecture (which degrades performance for anyone with AVX512). I'm open to it but I would also need access to some older hardware to test on.

Thanks for the pointer to your pipelines - I may use them to automate rc channel updates.

pydata / bottleneck

Preparing to release bottleneck 1.3.0 #191