-
Notifications
You must be signed in to change notification settings - Fork 18
497 ingestion memory #558
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
497 ingestion memory #558
Changes from 7 commits
d27879f
30a6472
a23fb84
7bf7767
942c27e
4274561
6cb0efe
4500bfa
fa60f14
4e35a13
a7ab60e
5d38dbb
f58593b
61fd99d
d4deac7
1532d67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -394,13 +394,19 @@ def segy_to_mdio( # noqa: PLR0913, PLR0915 | |
| grid_density_qc(grid, num_traces) | ||
| grid.build_map(index_headers) | ||
|
|
||
| # Check grid validity by comparing trace numbers | ||
| if np.sum(grid.live_mask) != num_traces: | ||
| # Check grid validity by ensuring every trace's header-index is within dimension bounds | ||
| valid_mask = np.ones(grid.num_traces, dtype=bool) | ||
| for d_idx in range(len(grid.header_index_arrays)): | ||
| coords = grid.header_index_arrays[d_idx] | ||
| valid_mask &= (coords < grid.shape[d_idx]) | ||
| valid_count = int(np.count_nonzero(valid_mask)) | ||
| if valid_count != num_traces: | ||
| for dim_name in grid.dim_names: | ||
| dim_min, dim_max = grid.get_min(dim_name), grid.get_max(dim_name) | ||
| dim_min = grid.get_min(dim_name) | ||
| dim_max = grid.get_max(dim_name) | ||
| logger.warning("%s min: %s max: %s", dim_name, dim_min, dim_max) | ||
| logger.warning("Ingestion grid shape: %s.", grid.shape) | ||
| raise GridTraceCountError(np.sum(grid.live_mask), num_traces) | ||
| raise GridTraceCountError(valid_count, num_traces) | ||
|
|
||
| if chunksize is None: | ||
| dim_count = len(index_names) + 1 | ||
|
|
@@ -446,13 +452,58 @@ def segy_to_mdio( # noqa: PLR0913, PLR0915 | |
| data_array = data_group[f"chunked_{suffix}"] | ||
| header_array = meta_group[f"chunked_{suffix}_trace_headers"] | ||
|
|
||
| # Write actual live mask and metadata to empty MDIO | ||
| meta_group["live_mask"][:] = grid.live_mask[:] | ||
| nonzero_count = np.count_nonzero(grid.live_mask) | ||
| live_mask_array = meta_group["live_mask"] | ||
| # 'live_mask_array' has the same first N–1 dims as 'grid.shape[:-1]' | ||
| # Build a ChunkIterator over the live_mask (no sample axis) | ||
| from mdio.core.indexing import ChunkIterator | ||
|
|
||
| chunker = ChunkIterator(live_mask_array, chunk_samples=False) | ||
| for chunk_indices in chunker: | ||
| # chunk_indices is a tuple of N–1 slice objects | ||
| trace_ids = grid.get_traces_for_chunk(chunk_indices) | ||
| if trace_ids.size == 0: | ||
| continue | ||
|
|
||
| # Build a temporary boolean block of shape = chunk shape | ||
| block_shape = tuple(sl.stop - sl.start for sl in chunk_indices) | ||
| block = np.zeros(block_shape, dtype=bool) | ||
|
|
||
| # Compute local coords within this block for each trace_id | ||
| local_coords: list[np.ndarray] = [] | ||
| for dim_idx, sl in enumerate(chunk_indices): | ||
| hdr_arr = grid.header_index_arrays[dim_idx] | ||
| # Optimize memory usage: hdr_arr and trace_ids are already uint32, | ||
| # sl.start is int, so result should naturally be int32/uint32. | ||
| # Avoid unnecessary astype conversion to int64. | ||
| indexed_coords = hdr_arr[trace_ids] # uint32 array | ||
| local_idx = indexed_coords - sl.start # remains uint32 | ||
| # Only convert dtype if necessary for indexing (numpy requires int for indexing) | ||
| if local_idx.dtype != np.intp: | ||
| local_idx = local_idx.astype(np.intp) | ||
| local_coords.append(local_idx) | ||
|
|
||
| # Mark live cells in the temporary block | ||
| block[tuple(local_coords)] = True | ||
|
|
||
| # Write the entire block to Zarr at once | ||
| live_mask_array.set_basic_selection(selection=chunk_indices, value=block) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the performance implication of this when writing to cloud storage? This will be A LOT of write requests in sequence. We could probably optimize the size of the block iteration (e.g. give a mock array to ChunkIterator with larger virtual chunks)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intended to operate on That does bring up the issue that I seem to have accidentally nuked the internal logic to chunk it in the first place though.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: I confused myself between the internal Grid chunking and the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool, makes sense then. can you validate how many write requests it does for a 100GB-sh file?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've validated that it only reaches this block once per live mask chunk. For a 100GB file, this is once. Reduced the maximum size of the live mask and saw the appropriate amount of iterations. |
||
|
|
||
| nonzero_count = grid.num_traces | ||
|
|
||
| write_attribute(name="trace_count", zarr_group=root_group, attribute=nonzero_count) | ||
| write_attribute(name="text_header", zarr_group=meta_group, attribute=text_header.split("\n")) | ||
| write_attribute(name="binary_header", zarr_group=meta_group, attribute=binary_header.to_dict()) | ||
|
|
||
| # Clean up live mask related variables | ||
| del live_mask_array | ||
| del chunker | ||
| del block | ||
| del local_coords | ||
| del trace_ids | ||
| del chunk_indices | ||
| import gc | ||
| gc.collect() | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't necessarily have to do all this; can you profile it end-to-end without this and see if its necessary
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deeper dive into why I did this Here's a zoomed in view of commit Here's a zoomed in view after adding the eager garbage collection These two plots show the period between It helped decrease heap allocation by ~200MiB to eagerly delete and garbage collect the objects. *I failed to drop the cache on the |
||
| # Write traces | ||
| stats = blocked_io.to_zarr( | ||
| segy_file=segy, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,9 @@ def __post_init__(self) -> None: | |
| self.dim_names = tuple(dim.name for dim in self.dims) | ||
| self.shape = tuple(dim.size for dim in self.dims) | ||
| self.ndim = len(self.dims) | ||
| # Prepare attributes for lazy mapping; they will be set in build_map | ||
| self.header_index_arrays: tuple[np.ndarray, ...] = () | ||
| self.num_traces: int = 0 | ||
|
|
||
| def __getitem__(self, item: int) -> Dimension: | ||
| """Get a dimension by index.""" | ||
|
|
@@ -106,47 +109,64 @@ def from_zarr(cls, zarr_root: zarr.Group) -> Grid: | |
| return cls(dims_list) | ||
|
|
||
| def build_map(self, index_headers: HeaderArray) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't understand the need to change in this function and the next. the map and live_mask are compressed zarr arrays in memory and headers are already processed in batch.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was actually the majority of the reason behind the change. The for start in range(0, total_live_traces, batch_size):
end = min(start + batch_size, total_live_traces)
live_dim_indices = []
# Compute indices for the batch
for dim in self.dims[:-1]:
dim_hdr = index_headers[dim.name][start:end]
indices = np.searchsorted(dim, dim_hdr).astype(np.uint32)
live_dim_indices.append(indices)
live_dim_indices = tuple(live_dim_indices)
# Assign trace indices
trace_indices = np.arange(start, end, dtype=np.uint64)
self.map.vindex[live_dim_indices] = trace_indices # <--- This vindexing assignment
self.live_mask.vindex[live_dim_indices] = True
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, if i understood correctly, if we are writing the live mask from distributed tasks (which are chunked same as seismic) it will cause race conditions and corrupt the live mask (which has larger chunks). e.g. multiple workers may write to the same chunk at the same time. Have you checked if live mask is identical before/after this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not writing the live mask in a distributed manner, and the Grid.live_mask chunking also differs from the live mask which is written to durable media. I believe this is 100% safe behavior. |
||
| """Build trace mapping and live mask from header indices. | ||
| """Compute per-trace grid coordinates (lazy map). | ||
|
|
||
| Instead of allocating a full `self.map` and `self.live_mask`, this computes, for each trace, | ||
| its integer index along each dimension (excluding the final sample dimension) and stores them in | ||
| `self.header_index_arrays`. The full mapping can then be derived chunk-by-chunk when writing. | ||
|
|
||
| Args: | ||
| index_headers: Header array containing dimension indices. | ||
| index_headers: Header array containing dimension indices (length = number of traces). | ||
| """ | ||
| # Number of traces in the SEG-Y | ||
| self.num_traces = int(index_headers.shape[0]) | ||
|
|
||
| # For each dimension except the final sample dimension, compute a 1D array of length | ||
| # `num_traces` giving each trace's integer coordinate along that axis (via np.searchsorted). | ||
| # Cast to uint32. | ||
| idx_arrays: list[np.ndarray] = [] | ||
| for dim in self.dims[:-1]: | ||
| hdr_vals = index_headers[dim.name] # shape: (num_traces,) | ||
| coords = np.searchsorted(dim, hdr_vals) # integer indices | ||
| coords = coords.astype(np.uint32) | ||
| idx_arrays.append(coords) | ||
|
|
||
| # Store as a tuple so that header_index_arrays[d][i] is "trace i's index along axis d" | ||
| self.header_index_arrays = tuple(idx_arrays) | ||
|
|
||
| # We no longer allocate `self.map` or `self.live_mask` here. | ||
| # The full grid shape is `self.shape`, but mapping is done lazily per chunk. | ||
| return | ||
|
|
||
| def get_traces_for_chunk(self, chunk_slices: tuple[slice, ...]) -> np.ndarray: | ||
| """Return all trace IDs whose grid-coordinates fall inside the given chunk slices. | ||
|
|
||
| Args: | ||
| chunk_slices: Tuple of slice objects, one per grid dimension. For example, | ||
| (slice(i0, i1), slice(j0, j1), ...) corresponds to a single Zarr chunk | ||
| in index space (excluding the sample axis). | ||
|
|
||
| Returns: | ||
| A 1D NumPy array of trace indices (0-based) that lie within the hyper-rectangle defined | ||
| by `chunk_slices`. If no traces fall in this chunk, returns an empty array. | ||
| """ | ||
| # Determine data type for map based on grid size | ||
| grid_size = np.prod(self.shape[:-1], dtype=np.uint64) | ||
| map_dtype = np.uint64 if grid_size > UINT32_MAX else np.uint32 | ||
| fill_value = np.iinfo(map_dtype).max | ||
|
|
||
| # Initialize Zarr arrays | ||
| live_shape = self.shape[:-1] | ||
| chunks = get_constrained_chunksize( | ||
| shape=live_shape, | ||
| dtype=map_dtype, | ||
| max_bytes=self._INTERNAL_CHUNK_SIZE_TARGET, | ||
| ) | ||
| self.map = zarr.full(live_shape, fill_value, dtype=map_dtype, chunks=chunks) | ||
| self.live_mask = zarr.zeros(live_shape, dtype=bool, chunks=chunks) | ||
|
|
||
| # Calculate batch size | ||
| memory_per_trace_index = index_headers.itemsize | ||
| batch_size = max(1, int(self._TARGET_MEMORY_PER_BATCH / memory_per_trace_index)) | ||
| total_live_traces = index_headers.size | ||
|
|
||
| # Process headers in batches | ||
| for start in range(0, total_live_traces, batch_size): | ||
| end = min(start + batch_size, total_live_traces) | ||
| live_dim_indices = [] | ||
|
|
||
| # Compute indices for the batch | ||
| for dim in self.dims[:-1]: | ||
| dim_hdr = index_headers[dim.name][start:end] | ||
| indices = np.searchsorted(dim, dim_hdr).astype(np.uint32) | ||
| live_dim_indices.append(indices) | ||
| live_dim_indices = tuple(live_dim_indices) | ||
|
|
||
| # Assign trace indices | ||
| trace_indices = np.arange(start, end, dtype=np.uint64) | ||
| self.map.vindex[live_dim_indices] = trace_indices | ||
| self.live_mask.vindex[live_dim_indices] = True | ||
| # Initialize a boolean mask over all traces (shape: (num_traces,)) | ||
| mask = np.ones((self.num_traces,), dtype=bool) | ||
|
|
||
| for dim_idx, sl in enumerate(chunk_slices): | ||
| arr = self.header_index_arrays[dim_idx] # shape: (num_traces,) | ||
| start, stop = sl.start, sl.stop | ||
| if start is not None: | ||
| mask &= (arr >= start) | ||
| if stop is not None: | ||
| mask &= (arr < stop) | ||
| if not mask.any(): | ||
| # No traces remain after this dimension's filtering | ||
| return np.empty((0,), dtype=np.uint32) | ||
|
|
||
| # Gather the trace IDs that survived all dimension tests | ||
| trace_ids = np.nonzero(mask)[0].astype(np.uint32) | ||
| return trace_ids | ||
|
|
||
|
|
||
| class GridSerializer(Serializer): | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this use more memory? grid.live_mask is a compressed bool zarr array in memory, but we just allocated the full size with np.ones. Doesn't this increase memory consumption?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does use more memory in the main process compared to what is live in 0.9.2 (below), but it has a lower peak memory consumption. This should only account for a few megabytes though. I'll look into any other low-hanging fruit as far as memory consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may become problematic if like mask is 4-5GB. We can probably still store it as a zarr array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally,
valid_maskis freed immediately following the validation check (and I've eagerly collected it as well now). I don't believe it is necessary to hold the compressed mask at all.