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

ENH: io/netcdf: make mmap=False the default on PyPy #8483

Merged
merged 1 commit into from Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion scipy/io/netcdf.py
Expand Up @@ -36,6 +36,7 @@
__all__ = ['netcdf_file']


import sys
import warnings
import weakref
from operator import mul
Expand All @@ -51,6 +52,8 @@

from scipy._lib.six import integer_types, text_type, binary_type

IS_PYPY = ('__pypy__' in sys.modules)

ABSENT = b'\x00\x00\x00\x00\x00\x00\x00\x00'
ZERO = b'\x00\x00\x00\x00'
NC_BYTE = b'\x00\x00\x00\x01'
Expand Down Expand Up @@ -248,7 +251,10 @@ def __init__(self, filename, mode='r', mmap=None, version=1,
omode = 'r+' if mode == 'a' else mode
self.fp = open(self.filename, '%sb' % omode)
if mmap is None:
mmap = True
# Mmapped files on PyPy cannot be usually closed
# before the GC runs, so it's better to use mmap=False
# as the default.
mmap = (not IS_PYPY)

if mode != 'r':
# Cannot read write-only files
Expand Down
31 changes: 18 additions & 13 deletions scipy/io/tests/test_netcdf.py
Expand Up @@ -14,7 +14,7 @@
from numpy.testing import assert_, assert_allclose, assert_equal
from pytest import raises as assert_raises

from scipy.io.netcdf import netcdf_file
from scipy.io.netcdf import netcdf_file, IS_PYPY

from scipy._lib._numpy_compat import suppress_warnings
from scipy._lib._tmpdirs import in_tempdir
Expand Down Expand Up @@ -82,8 +82,8 @@ def test_read_write_files():

# To read the NetCDF file we just created::
with netcdf_file('simple.nc') as f:
# Using mmap is the default
assert_(f.use_mmap)
# Using mmap is the default (but not on pypy)
assert_equal(f.use_mmap, not IS_PYPY)
check_simple(f)
assert_equal(f._attributes['appendRan'], 1)

Expand Down Expand Up @@ -111,10 +111,14 @@ def test_read_write_files():
check_simple(f)

# Read file from fileobj, with mmap
with open('simple.nc', 'rb') as fobj:
with netcdf_file(fobj, mmap=True) as f:
assert_(f.use_mmap)
check_simple(f)
with suppress_warnings() as sup:
if IS_PYPY:
sup.filter(RuntimeWarning,
"Cannot close a netcdf_file opened with mmap=True.*")
with open('simple.nc', 'rb') as fobj:
with netcdf_file(fobj, mmap=True) as f:
assert_(f.use_mmap)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe switch to plain assert now that we are using pytest, since pytest replaces plain assert with a bunch of useful stuff for error handling & prevents the removal of plain asserts by the python optimization flag. Conversely, not sure that we want a campaign of converting all the assert_ to assert anytime soon.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for switching to assert statements in new code

check_simple(f)

# Again read it in append mode (adding another att)
with open('simple.nc', 'r+b') as fobj:
Expand Down Expand Up @@ -314,12 +318,13 @@ def test_ticket_1720():
def test_mmaps_segfault():
filename = pjoin(TEST_DATA_PATH, 'example_1.nc')

with warnings.catch_warnings():
warnings.simplefilter("error")
with netcdf_file(filename, mmap=True) as f:
x = f.variables['lat'][:]
# should not raise warnings
del x
if not IS_PYPY:
with warnings.catch_warnings():
warnings.simplefilter("error")
with netcdf_file(filename, mmap=True) as f:
x = f.variables['lat'][:]
# should not raise warnings
del x

def doit():
with netcdf_file(filename, mmap=True) as f:
Expand Down