add memmap option to asdf.open
#1667
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
braingram
force-pushed
the
memmap_arrays
branch
2 times, most recently
from
405cf1c
to
2e09f0f
Compare
braingram
force-pushed
the
memmap_arrays
branch
3 times, most recently
from
9a8346e
to
28c99c4
Compare
zacharyburnett
approved these changes
Nov 14, 2023
eslavich
reviewed
Dec 4, 2023
asdf/asdf.py
Outdated
| @@ -116,6 +117,14 @@ def __init__( | |||
| When `False`, when reading files, attempt to memmap underlying data | |||
| arrays when possible. | |||
|
|
|||
| memmap_arrays : bool, optional | |||
There was a problem hiding this comment.
Is it more accurate to call this memmap_blocks? Or is it truly only arrays that are memory-mapped?
There was a problem hiding this comment.
Hmmm. I think blocks is a better match as arrays can be inline (and not memapped). Thanks! I'll update this PR.
There was a problem hiding this comment.
On second thought, should we just make this memmap?
There was a problem hiding this comment.
I updated this PR to use memmap. I'm happy to discuss alternatives (like memmap_blocks) and don't feel strongly about the naming of this argument. I went with memmap for a few reasons:
- it's shorter to type than
memmap_blockswhich still describing the basic option (memory mapping) - in case we come up with some new crazy way to memory map things (like the tree) the option should be forward compatible
- although not a general goal of asdf, it does more closely match the astropy API for fits files
braingram
force-pushed
the
memmap_arrays
branch
from
28c99c4
to
451a7a5
Compare
braingram
changed the title
asdf.openmemmap option to asdf.open
eslavich
approved these changes
Dec 18, 2023
braingram
force-pushed
the
memmap_arrays
branch
from
451a7a5
to
1ee4b99
Compare
This was referenced Dec 18, 2023
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first in a sequence of changes to switch
copy_arraystomemmapand to change the default to not memmap. I'm proposing that we do the following:memmapas a new argument toasdf.open(this PR) which when set, overridescopy_arrays. Introducing this first will allow libraries that usecopy_arraysto switch tomemmap. The documentation formemmapincludes a warning that the default for this option will change in an upcoming version of asdf.copy_arrayswith a warning that describing usingmemmapand that the default will change.copy_arraysand switchmemmapto default toFalseIf the above plan is agreeable I will open issues to track the second and third changes and milestone them for 3.2 and 4.0 respectively.
devdeps failures are unrelated and addressed in: #1665