FIX: Numpy pre-release accommodations #700

effigies · 2018-12-10T23:06:50Z

This has turned into something of an omnibus PR, which does the following things:

Updates the setup script to use setuptools and take advantage of conditional dependencies
Require bz2file for Python 2.7 only
Add Opener.readinto method (via RF: Circumvents a deprecation warning from np.fromstring #702)
Use bytearray and filelike.readinto in numerous places to preserve zero-copy construction of numpy arrays
Remove deprecated numpy.fromstring (via RF: Circumvents a deprecation warning from np.fromstring #702)

Closes #702.

Original

It looks like we've been taking advantage of numpy letting us declare a memory block writeable even when the underlying object is ostensibly immutable. This has allowed us to avoid copies for objects that we knew wouldn't be accessed through their original interfaces, but they decided this was a bug and closed this loophole in numpy/numpy#11739.

Don't know what else to do but make the copies they think we should be making, but perhaps others have more numpy arcana to share.

Fixes #697.

matthew-brett · 2018-12-11T13:29:06Z

For volumeutils, I think this only represents a change in behavior for bz2 files on Python 2.7, because:

we first check if the file-like object has a readinto method, and use it if so, to read into a bytearray. This is mutable. GzipFile, io.BytesIO and standard file objects have readinto in Python 2.7 and >=3.5. BZ2File also had this method for Python >= 3.5, but not for Python 2.7. So bz2 files fall through to the next check, on Python 2.7.
Then we check if the file is in the SAFE_STRINGERS tuple; this only includes GzipFile and BZ2File;
If so, then we attempt to avoid the copy. Here's where the BZ2File objects will cause a new double copy, because we cannot set the read contents as writeable.

The neater fix, for volumeutils, would be to drop the SAFE_STRINGERS check. This will make the code simpler at the expense of making BZFile objects copy for numpies, where we can set the writeable flag.

matthew-brett · 2018-12-11T20:25:30Z

For the trackvis reader, we could probably avoid nearly all the memory inflation by using the readinto trick that volumeutils is already using.

matthew-brett · 2018-12-11T20:36:20Z

This suggestion would help - we could depend on bz2file on Python 2.7, and then drop the SAFE_STRINGERS logic, with no loss of memory performance on any Numpy version.

This reverts commit 0a1ea75.

matthew-brett · 2018-12-12T18:40:17Z

nibabel/info.py

@@ -209,4 +209,5 @@ def cmp_pkg_version(version_str, pkg_version_str=__version__):
 ISRELEASE = _version_extra == ''
 VERSION = __version__
 PROVIDES = ["nibabel", 'nisext']
-REQUIRES = ["numpy (>=%s)" % NUMPY_MIN_VERSION]
+REQUIRES = ["numpy (>=%s)" % NUMPY_MIN_VERSION,


We still need to use the bz2file.BZ2File object instead of the the bz2.BZ2File object, in openers, for Python 2. Maybe:

import sys if sys.version_info[0] < 3: from bz2file import BZ2File else: from bz2 import BZ2File

and so on, maybe importing from here for other uses of BZ2File. I can't think immediately of a test for saving memory here, can you?

As in to avoid an early import? pkgutil.find_loader() is a Python 2/3-safe way to see if a module can be imported without actually importing.

No, sorry, the bz2file package provides an alternative BZ2File implementation, it does not replace the implementation in the bz2 package. If you check, I think you'll find that bz2.BZ2File still doesn't have readinto after you've installed bz2file, but bz2file.BZ2File does have readinto (on Python 2.7).

Ah, reparsed. Yeah, I still need to make that change. On the whole, this PR just isn't ready. To get tests working again, I still need to figure out why we can't use version selectors in install_requires. I suspect it's a distutils or setuptools problem.

``` /Users/arokem/.virtualenvs/afq/lib/python3.7/site-packages/nibabel/streamlines/trk.py:562: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead header_rec = np.fromstring(string=header_str, dtype=header_2_dtype) ```

This is because newer numpy doesn't allow to change the writeable flag.

coveralls · 2018-12-21T17:39:35Z

Coverage decreased (-0.004%) to 91.846% when pulling ea1b0cc on effigies:fix/writable_bug into d9c864b on nipy:master.

codecov-io · 2018-12-21T19:21:49Z

Codecov Report

Merging #700 into master will decrease coverage by 0.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   88.88%   88.86%   -0.02%     
==========================================
  Files          93       93              
  Lines       11449    11455       +6     
  Branches     1892     1894       +2     
==========================================
+ Hits        10176    10180       +4     
- Misses        933      934       +1     
- Partials      340      341       +1

Impacted Files	Coverage Δ
nibabel/volumeutils.py	`92.75% <100%> (-0.03%)`	⬇️
nibabel/streamlines/trk.py	`94.31% <100%> (+0.01%)`	⬆️
nibabel/gifti/parse_gifti_fast.py	`84.51% <100%> (ø)`	⬆️
nibabel/streamlines/tck.py	`98.9% <100%> (+0.01%)`	⬆️
nibabel/info.py	`100% <100%> (ø)`	⬆️
nibabel/nifti1.py	`91.22% <100%> (ø)`	⬆️
nibabel/openers.py	`75.8% <71.42%> (-0.67%)`	⬇️

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 d9c864b...ea1b0cc. Read the comment docs.

…/writable_bug

effigies · 2018-12-21T20:15:03Z

@arokem I ended up needing your Opener.readinto to resolve the streamlines complaints, so I just merged your PR into this one.

(Moved here from the edited top post, since I'm not sure if adding a handle in an edit notifies people.)

effigies · 2018-12-21T21:18:28Z

@matthew-brett This one is ready for review.

matthew-brett

Great - thanks - just a couple of comments.

matthew-brett · 2018-12-22T10:18:37Z

nibabel/streamlines/trk.py

-            header_str = f.read(header_2_dtype.itemsize)
-            header_rec = np.fromstring(string=header_str, dtype=header_2_dtype)
-
+            # Read the header into a bytearray.


Might be worth a comment explaining use of readinto.

matthew-brett · 2018-12-22T10:20:12Z

nisext/sexts.py

@@ -191,7 +191,6 @@ def version_getter(pkg_name):
                        optional,
                        dependency)
        return
-    _add_append_key(setuptools_args, 'install_requires', dependency)


I guess it's just possible someone else is using nisext. Any way to avoid the API change?

effigies · 2018-12-27T00:25:17Z

@matthew-brett Any more comments?

effigies · 2018-12-31T17:38:26Z

@matthew-brett Going to merge this and start the 2.3.2 process. If you have further concerns, can you open a PR with a suggested fix?

matthew-brett · 2019-01-02T02:11:56Z

Good call - looks fine to me.

FIX: Immutable buffers should not be set writeable

0a1ea75

effigies added 4 commits December 11, 2018 19:02

Revert "FIX: Immutable buffers should not be set writeable"

81fe543

This reverts commit 0a1ea75.

MAINT: Add bz2file dependency for Python 2

c0440e0

RF: Drop SAFE_STRINGERS, depend on bz2file to provide .readinto()

2452a0f

CI: Upgrade setuptools

81e775f

matthew-brett reviewed Dec 12, 2018

View reviewed changes

arokem added 2 commits December 17, 2018 13:20

RF: fromstring => frombuffer in externals.netcdf.

53d9413

effigies mentioned this pull request Dec 17, 2018

RF: Circumvents a deprecation warning from np.fromstring #702

Closed

arokem and others added 7 commits December 17, 2018 13:50

RF: fromstring => frombuffer in gifti/cifti2

5d11c9e

RF: fromstring => frombuffer.

b7e4850

BF: Use a writeable bytearray for the header data.

8c0b21d

This is because newer numpy doesn't allow to change the writeable flag.

NF: Implement a readinto method for Opener.

a696d0d

MAINT: Move to setuptools exclusively

a97b067

FIX: Use bz2file for Python 2

c79a291

FIX: Rely on openers.BZ2File to provide correct class

62b8cc9

effigies force-pushed the fix/writable_bug branch from 633eef8 to 62b8cc9 Compare December 21, 2018 17:04

CI: Upgrade pip, setuptools, wheel on AppVeyor

28412d9

effigies added 3 commits December 21, 2018 12:40

TEST: fobj string assumptions need only apply to bytearrays

d7a619f

FIX: Load TckFile streamlines using bytearray

f5eafcf

TEST: Return mutable bytearray from trk_with_bytes

8209664

effigies added 4 commits December 21, 2018 14:36

Merge remote-tracking branch 'arokem/fromstring_deprecation' into fix…

e6f58cb

…/writable_bug

FIX: Truncate buffer on EOF

ed42db3

CI: Try alternative pip install

24cba1b

FIX: Use io.BytesIO to test TckFile

03086a4

STY: Drop unused variable

d4f4477

effigies changed the title ~~FIX: Immutable buffers should not be set writeable~~ FIX: Numpy pre-release accommodations Dec 21, 2018

matthew-brett reviewed Dec 22, 2018

View reviewed changes

effigies added 2 commits December 22, 2018 09:13

RF: Leave nisext alone, pop install_requires

7cf14a3

DOC: Add a comment explaining readinto/frombuffer idiom

ea1b0cc

effigies mentioned this pull request Dec 25, 2018

BUG: Fix deprecation warning on use of np.fromstring. #706

Closed

effigies mentioned this pull request Dec 31, 2018

Numpy pre-release breaks setting some arrays writable #697

Closed

effigies merged commit a13de6e into nipy:master Dec 31, 2018

effigies deleted the fix/writable_bug branch December 31, 2018 17:38

This was referenced Dec 31, 2018

ENH,WIP: New transforms module #656

Closed

DEBUG: Revert to pre-error trees and enable AppVeyor debugging #680

Closed

effigies mentioned this pull request Apr 1, 2019

Cannot set values in GIFTI data array loaded from file #746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Numpy pre-release accommodations #700

FIX: Numpy pre-release accommodations #700

effigies commented Dec 10, 2018 •

edited

Loading

matthew-brett commented Dec 11, 2018

matthew-brett commented Dec 11, 2018

matthew-brett commented Dec 11, 2018

matthew-brett Dec 12, 2018

effigies Dec 12, 2018

matthew-brett Dec 12, 2018

effigies Dec 12, 2018

coveralls commented Dec 21, 2018 •

edited

Loading

codecov-io commented Dec 21, 2018 •

edited

Loading

effigies commented Dec 21, 2018

effigies commented Dec 21, 2018

matthew-brett left a comment

matthew-brett Dec 22, 2018

matthew-brett Dec 22, 2018

effigies commented Dec 27, 2018

effigies commented Dec 31, 2018

matthew-brett commented Jan 2, 2019

FIX: Numpy pre-release accommodations #700

FIX: Numpy pre-release accommodations #700

Conversation

effigies commented Dec 10, 2018 • edited Loading

Original

matthew-brett commented Dec 11, 2018

matthew-brett commented Dec 11, 2018

matthew-brett commented Dec 11, 2018

matthew-brett Dec 12, 2018

Choose a reason for hiding this comment

effigies Dec 12, 2018

Choose a reason for hiding this comment

matthew-brett Dec 12, 2018

Choose a reason for hiding this comment

effigies Dec 12, 2018

Choose a reason for hiding this comment

coveralls commented Dec 21, 2018 • edited Loading

codecov-io commented Dec 21, 2018 • edited Loading

Codecov Report

effigies commented Dec 21, 2018

effigies commented Dec 21, 2018

matthew-brett left a comment

Choose a reason for hiding this comment

matthew-brett Dec 22, 2018

Choose a reason for hiding this comment

matthew-brett Dec 22, 2018

Choose a reason for hiding this comment

effigies commented Dec 27, 2018

effigies commented Dec 31, 2018

matthew-brett commented Jan 2, 2019

effigies commented Dec 10, 2018 •

edited

Loading

coveralls commented Dec 21, 2018 •

edited

Loading

codecov-io commented Dec 21, 2018 •

edited

Loading