From 739dba2ba1df676691219be794158c73001d5def Mon Sep 17 00:00:00 2001 From: zacharyburnett Date: Fri, 25 Oct 2024 12:04:45 -0400 Subject: [PATCH 1/9] default `memmap=False` --- asdf/_asdf.py | 9 +++++---- changes/1801.general.rst | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) create mode 100644 changes/1801.general.rst diff --git a/asdf/_asdf.py b/asdf/_asdf.py index 3a5d4a3fb..8eb220b3e 100644 --- a/asdf/_asdf.py +++ b/asdf/_asdf.py @@ -57,7 +57,8 @@ def __init__( extensions=None, version=None, ignore_unrecognized_tag=False, - memmap=True, + ignore_implicit_conversion=NotSet, + memmap=False, lazy_load=True, custom_schema=None, ): @@ -88,7 +89,7 @@ def __init__( memmap : bool, optional When `True`, when reading files, attempt to memmap underlying data - arrays when possible. Defaults to ``True``. + arrays when possible. Defaults to ``False``. lazy_load : bool, optional When `True` and the underlying file handle is seekable, data @@ -1510,7 +1511,7 @@ def open_asdf( extensions=None, ignore_unrecognized_tag=False, _force_raw_types=False, - memmap=True, + memmap=False, lazy_tree=NotSet, lazy_load=True, custom_schema=None, @@ -1549,7 +1550,7 @@ def open_asdf( memmap : bool, optional When `True`, when reading files, attempt to memmap underlying data - arrays when possible. Defaults to ``True``. + arrays when possible. Defaults to ``False``. lazy_load : bool, optional When `True` and the underlying file handle is seekable, data diff --git a/changes/1801.general.rst b/changes/1801.general.rst new file mode 100644 index 000000000..ca218ddc5 --- /dev/null +++ b/changes/1801.general.rst @@ -0,0 +1 @@ +set ``memmap=False`` by default From e69a70572809ad66d74446df1d6bb06785162a41 Mon Sep 17 00:00:00 2001 From: zacharyburnett Date: Thu, 31 Oct 2024 14:34:43 -0400 Subject: [PATCH 2/9] fix tests to use new default --- asdf/_tests/tags/core/tests/test_ndarray.py | 6 +++--- asdf/_tests/test_api.py | 2 +- asdf/_tests/test_array_blocks.py | 10 ++++++---- asdf/_tests/test_generic_io.py | 6 +++--- asdf/_tests/test_reference.py | 4 +++- 5 files changed, 16 insertions(+), 12 deletions(-) diff --git a/asdf/_tests/tags/core/tests/test_ndarray.py b/asdf/_tests/tags/core/tests/test_ndarray.py index 08a2202fe..d0f4b80ab 100644 --- a/asdf/_tests/tags/core/tests/test_ndarray.py +++ b/asdf/_tests/tags/core/tests/test_ndarray.py @@ -976,7 +976,7 @@ def test_memmap_write(tmp_path): tmpfile = str(tmp_path / "data.asdf") tree = {"data": np.zeros(100)} - with asdf.AsdfFile(tree) as af: + with asdf.AsdfFile(tree, memmap=False) as af: # Make sure we're actually writing to an internal array for this test af.write_to(tmpfile, all_array_storage="internal") @@ -1002,7 +1002,7 @@ def test_readonly(tmp_path): af.write_to(tmpfile, all_array_storage="internal") # Opening in read mode (the default) should mean array is readonly - with asdf.open(tmpfile) as af: + with asdf.open(tmpfile, memmap=True) as af: assert af["data"].flags.writeable is False with pytest.raises(ValueError, match=r"assignment destination is read-only"): af["data"][0] = 41 @@ -1046,7 +1046,7 @@ def test_block_data_change(pad_blocks, tmp_path): with asdf.AsdfFile(tree) as af: af.write_to(tmpfile, pad_blocks=pad_blocks) - with asdf.open(tmpfile, mode="rw") as af: + with asdf.open(tmpfile, mode="rw", memmap=True) as af: assert np.all(af.tree["data"] == 0) array_before = af.tree["data"].__array__() af.tree["data"][:5] = 1 diff --git a/asdf/_tests/test_api.py b/asdf/_tests/test_api.py index b33059511..6c849db0c 100644 --- a/asdf/_tests/test_api.py +++ b/asdf/_tests/test_api.py @@ -60,7 +60,7 @@ def test_open_readonly(tmp_path): os.chmod(tmpfile, 0o440) assert os.access(tmpfile, os.W_OK) is False - with asdf.open(tmpfile) as af: + with asdf.open(tmpfile, memmap=True) as af: assert af["baz"].flags.writeable is False with pytest.raises(PermissionError, match=r".* Permission denied: .*"), asdf.open(tmpfile, mode="rw"): diff --git a/asdf/_tests/test_array_blocks.py b/asdf/_tests/test_array_blocks.py index e8d8cf3c4..e162e668d 100644 --- a/asdf/_tests/test_array_blocks.py +++ b/asdf/_tests/test_array_blocks.py @@ -811,9 +811,11 @@ def filename_with_array(tmp_path_factory): @pytest.mark.parametrize( "open_kwargs,should_memmap", [ - ({}, True), - ({"memmap": True}, True), - ({"memmap": False}, False), + ({}, False), + ({"lazy_load": True, "memmap": True}, True), + ({"lazy_load": False, "memmap": True}, True), + ({"lazy_load": True, "memmap": False}, False), + ({"lazy_load": False, "memmap": False}, False), ], ) def test_open_no_memmap(filename_with_array, open_kwargs, should_memmap): @@ -823,7 +825,7 @@ def test_open_no_memmap(filename_with_array, open_kwargs, should_memmap): default (no kwargs) memmap """ - with asdf.open(filename_with_array, lazy_load=False, **open_kwargs) as af: + with asdf.open(filename_with_array, **open_kwargs) as af: array = af.tree["array"] if should_memmap: assert isinstance(array.base, np.memmap) diff --git a/asdf/_tests/test_generic_io.py b/asdf/_tests/test_generic_io.py index fea00295f..25158b632 100644 --- a/asdf/_tests/test_generic_io.py +++ b/asdf/_tests/test_generic_io.py @@ -110,7 +110,7 @@ def get_read_fd(): f.read(0) return f - with _roundtrip(tree, get_write_fd, get_read_fd) as ff: + with _roundtrip(tree, get_write_fd, get_read_fd, read_options={"memmap": True}) as ff: assert len(ff._blocks.blocks) == 2 assert isinstance(ff._blocks.blocks[0].cached_data, np.memmap) @@ -133,7 +133,7 @@ def get_read_fd(): assert f._uri == path.as_uri() return f - with _roundtrip(tree, get_write_fd, get_read_fd) as ff: + with _roundtrip(tree, get_write_fd, get_read_fd, read_options={"memmap": True}) as ff: assert len(ff._blocks.blocks) == 2 assert isinstance(ff._blocks.blocks[0].cached_data, np.memmap) @@ -173,7 +173,7 @@ def get_read_fd(): assert f._uri == path.as_uri() return f - with _roundtrip(tree, get_write_fd, get_read_fd) as ff: + with _roundtrip(tree, get_write_fd, get_read_fd, read_options={"memmap": True}) as ff: assert len(ff._blocks.blocks) == 2 assert isinstance(ff._blocks.blocks[0].cached_data, np.memmap) ff.tree["science_data"][0] = 42 diff --git a/asdf/_tests/test_reference.py b/asdf/_tests/test_reference.py index f618865fd..6b4c384a4 100644 --- a/asdf/_tests/test_reference.py +++ b/asdf/_tests/test_reference.py @@ -78,7 +78,9 @@ def do_asserts(ff): assert_array_equal(ff.tree["internal"], exttree["cool_stuff"]["a"]) - with asdf.AsdfFile({}, uri=(tmp_path / "main.asdf").as_uri()) as ff: + with asdf.AsdfFile({}, uri=(tmp_path / "main.asdf").as_uri(), memmap=True) as ff: + # avoid passing tree to AsdfFile to avoid the deprecation warning, this can be updated + # when automatic find_references on AsdfFile.__init__ is removed ff.tree = tree ff.find_references() do_asserts(ff) From 787cd551d1b603081370423cdd5d0f192c3df84d Mon Sep 17 00:00:00 2001 From: zacharyburnett Date: Fri, 8 Nov 2024 10:12:40 -0500 Subject: [PATCH 3/9] add to what's new page --- docs/asdf/whats_new.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/asdf/whats_new.rst b/docs/asdf/whats_new.rst index 8f9328cbf..7d3c9d2be 100644 --- a/docs/asdf/whats_new.rst +++ b/docs/asdf/whats_new.rst @@ -46,6 +46,13 @@ Removed API New Defaults ------------ +File I/O +^^^^^^^^ + +- calls to ``asdf.open()`` and ``AsdfFile()`` do NOT memory-map the + underlying arrays by default; the relevant parameter defaults to + ``memmap=False`` + .. _whats_new_4.0.0_validation: Validation From d107b4698af6f8fc20d7d75ed13a9f98d41216ff Mon Sep 17 00:00:00 2001 From: Zach Burnett Date: Fri, 8 Nov 2024 10:13:29 -0500 Subject: [PATCH 4/9] Update asdf/_asdf.py --- asdf/_asdf.py | 1 - 1 file changed, 1 deletion(-) diff --git a/asdf/_asdf.py b/asdf/_asdf.py index 8eb220b3e..434733f87 100644 --- a/asdf/_asdf.py +++ b/asdf/_asdf.py @@ -57,7 +57,6 @@ def __init__( extensions=None, version=None, ignore_unrecognized_tag=False, - ignore_implicit_conversion=NotSet, memmap=False, lazy_load=True, custom_schema=None, From 06e0d6616269e7385f0cd134d0e9b1bf4179f42a Mon Sep 17 00:00:00 2001 From: Zach Burnett Date: Fri, 8 Nov 2024 12:55:22 -0500 Subject: [PATCH 5/9] apply review suggestion Co-authored-by: Brett Graham --- asdf/_tests/test_reference.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/asdf/_tests/test_reference.py b/asdf/_tests/test_reference.py index 6b4c384a4..f618865fd 100644 --- a/asdf/_tests/test_reference.py +++ b/asdf/_tests/test_reference.py @@ -78,9 +78,7 @@ def do_asserts(ff): assert_array_equal(ff.tree["internal"], exttree["cool_stuff"]["a"]) - with asdf.AsdfFile({}, uri=(tmp_path / "main.asdf").as_uri(), memmap=True) as ff: - # avoid passing tree to AsdfFile to avoid the deprecation warning, this can be updated - # when automatic find_references on AsdfFile.__init__ is removed + with asdf.AsdfFile({}, uri=(tmp_path / "main.asdf").as_uri()) as ff: ff.tree = tree ff.find_references() do_asserts(ff) From 6d9c525338e6028435bf0a92e9c390d0bf2a1640 Mon Sep 17 00:00:00 2001 From: Zach Burnett Date: Fri, 8 Nov 2024 12:56:03 -0500 Subject: [PATCH 6/9] better rewording Co-authored-by: Brett Graham --- docs/asdf/whats_new.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/asdf/whats_new.rst b/docs/asdf/whats_new.rst index 7d3c9d2be..35d3eb20e 100644 --- a/docs/asdf/whats_new.rst +++ b/docs/asdf/whats_new.rst @@ -46,12 +46,12 @@ Removed API New Defaults ------------ -File I/O -^^^^^^^^ +.. _whats_new_4.0.0_memmap -- calls to ``asdf.open()`` and ``AsdfFile()`` do NOT memory-map the - underlying arrays by default; the relevant parameter defaults to - ``memmap=False`` +Memory mapping disabled by default +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Calls to ``asdf.open() and ``AsdfFile()`` will now default to ``memmap=False``, disabling memory mapping of arrays by default. .. _whats_new_4.0.0_validation: From a45f2c5e71eaaa8ab650eae222c828184c2267c9 Mon Sep 17 00:00:00 2001 From: Zach Burnett Date: Fri, 8 Nov 2024 12:56:22 -0500 Subject: [PATCH 7/9] review suggestion Co-authored-by: Brett Graham --- changes/1801.general.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/1801.general.rst b/changes/1801.general.rst index ca218ddc5..36c8bc4f7 100644 --- a/changes/1801.general.rst +++ b/changes/1801.general.rst @@ -1 +1 @@ -set ``memmap=False`` by default +Set ``memmap=False`` to default for ``asdf.open`` and ``AsdfFile.__init__``. From 1a84d419f51e48c13c1c6e7447f376ac498412aa Mon Sep 17 00:00:00 2001 From: zacharyburnett Date: Fri, 8 Nov 2024 14:07:21 -0500 Subject: [PATCH 8/9] remove read-only from asserts --- asdf/_tests/test_reference.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/asdf/_tests/test_reference.py b/asdf/_tests/test_reference.py index f618865fd..37dee54f4 100644 --- a/asdf/_tests/test_reference.py +++ b/asdf/_tests/test_reference.py @@ -56,9 +56,6 @@ def do_asserts(ff): assert_array_equal(ff.tree["science_data"], exttree["cool_stuff"]["a"]) assert len(ff._external_asdf_by_uri) == 1 - with pytest.raises((ValueError, RuntimeError), match=r"assignment destination is read-only"): - # Assignment destination is readonly - ff.tree["science_data"][0] = 42 assert_array_equal(ff.tree["science_data2"], exttree["cool_stuff"]["a"]) assert len(ff._external_asdf_by_uri) == 2 From 145e781878c8f3397979b26aabae0446748634c1 Mon Sep 17 00:00:00 2001 From: Zach Burnett Date: Fri, 8 Nov 2024 17:23:06 -0500 Subject: [PATCH 9/9] never forget your colon Co-authored-by: Brett Graham --- docs/asdf/whats_new.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/asdf/whats_new.rst b/docs/asdf/whats_new.rst index 35d3eb20e..5db2ecdc1 100644 --- a/docs/asdf/whats_new.rst +++ b/docs/asdf/whats_new.rst @@ -46,7 +46,7 @@ Removed API New Defaults ------------ -.. _whats_new_4.0.0_memmap +.. _whats_new_4.0.0_memmap: Memory mapping disabled by default ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^