Unload datasets by default - #587
Draft
gtrevisan wants to merge 5 commits into
Draft
Conversation
gtrevisan
requested review from
nbarbour13 and
samc24
and
a balanced review from Copilot
August 18, 2026 15:53
Contributor
There was a problem hiding this comment.
Pull request overview
Improves output memory efficiency by sharding per-shot datasets and retaining lazy-loaded references.
Changes:
- Writes temporary NetCDF shards and moves them to final paths.
- Loads shards before single-output concatenation and removes them afterward.
- Adds MaxRSS workflow logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
disruption_py/workflow.py |
Adds memory watermark logging. |
disruption_py/settings/output_setting.py |
Implements dataset sharding, lazy loading, finalization, and cleanup. |
Suppressed comments (3)
disruption_py/workflow.py:229
- This MaxRSS conversion is incorrect on macOS, where
ru_maxrssis bytes; the logged MB value will be 1,024× too large. Apply the same platform-aware conversion here.
mem=resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024,
disruption_py/workflow.py:237
- This also mislabels macOS's byte-valued
ru_maxrssafter saving, producing an MB value 1,024× too large. Normalize the unit by platform here as well.
mem=resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024,
disruption_py/settings/output_setting.py:301
- This second SingleOutputSetting watermark has the same macOS unit error: bytes divided by 1,024 are not MB. Use the platform-aware conversion here too.
mem=resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or 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
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.
we want
DictOutputSetting, which is our under-the-hood de facto base class, to be memory-efficient and therefore more reliable for large-scale workflows.Pool-processed dataset is fed tooutput_shot:furthermore, for the (abstract) subclass
SingleOutputSetting:as a consequence:
DictOutputSettingshould complete the workflow with a low memory footprint, namely just enough to have lazy-loaded datatasets as dictionary values,DictOutputSettingrequires explicit downstream concatenation.SingleOutputSettingshould work effectively as before, although the new logic is to unload each process results to disk and to load them up again from disk just before concatenation. this results in a minor performance hit which is negligible due to the no-encoding and no-compression defaults of xarray'sto_netcdf, and which is overshadowed by the newly-introduced impossibility of losing data during the concatenation stage,example runs for 1,000 C-MOD shots:
DatasetOutputSetting:DictOutputSetting:I already have a plan to develop a new
OutputSettingto efficiently concat large-scale workflow results.@samc24 thoughts on the architecture? for sure we could change many things, including fully revamping the multiprocessing data exchange and all the output settings, but this felt a great bang-for-the-buck improvement to me.
@nbarbour13 tests should already pass, but feel free to play around with the various output settings to make sure the results still end up where they are supposed to, and no shards get left behind.