Conversation
|
Github was complaining about merge conflicts in the dataset_utils file. Resolved them by keeping the zarr-related changes, but it can be undone if it was unneeded. |
|
Tested the functionality of both the map_app and ngiab_data_cli, both seem to function mostly as intended. Edit: No real error. Using the map_app's provided pip command gave me a false negative as it used a cached old version of the package and then failed. |
There was a problem hiding this comment.
Pull request overview
This PR switches the “raw gridded forcing cache” artifact from an uncompressed NetCDF file to a local Zarr archive, aiming to reduce disk footprint, speed up writes, and reduce Dask task-graph size/stability issues when downloading larger forcing windows.
Changes:
- Replaced NetCDF cache pathing with a
cached_zarr_file(raw_gridded_data.zarr) throughout CLI + map app flows. - Updated the caching implementation to write Zarr (via a temporary “.saving.zarr” store + rename) and reopen via the Zarr engine.
- Adjusted forcings-directory cleanup logic to preserve the Zarr cache (but cleanup currently needs a fix for directory handling).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/ngiab_data_cli/forcing_cli.py | Updates the standalone forcing CLI to use the new cache mechanism (but currently still names the cache *.nc). |
| modules/ngiab_data_cli/main.py | Switches the main CLI pipeline to use paths.cached_zarr_file for caching. |
| modules/map_app/views.py | Updates the map app forcing download path to use paths.cached_zarr_file and includes formatting/import cleanup. |
| modules/data_processing/forcings.py | Preserves the Zarr cache during forcings-dir cleanup (but currently uses unlink() which breaks on directory-based Zarr artifacts). |
| modules/data_processing/file_paths.py | Replaces cached_nc_file with cached_zarr_file pointing at raw_gridded_data.zarr. |
| modules/data_processing/dataset_utils.py | Implements Zarr cache writing (temp store + rename) and updates cache open/check logic to use the Zarr engine. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cached_zarr_path = args.output_file.parent / (args.input_file.stem + "-raw-gridded-data.nc") | ||
| print(cached_zarr_path) |
| # delete everything in the forcing folder except the cached nc file | ||
| for file in forcing_paths.forcings_dir.glob("*.*"): | ||
| if file != forcing_paths.cached_nc_file: | ||
| if file != forcing_paths.cached_zarr_file: | ||
| file.unlink() |
| except: | ||
| logger.info("Cache produced with outdated backend, redownloading") | ||
| return | ||
| cached_data = xr.open_mfdataset(cached_zarr_path, parallel=True, engine="zarr") |
|
Did this ever move out of draft stage |
saving it as zarr seems to reduce the file size down to ~20% of the netcdf, it runs faster, and reduces the amount of operations that get added to the dask task graph which makes it more stable at downloading larger quantities of data.
a more nuclear option than #172 but we should do this regardless at some point. also should finally close out #107 #132
needs a more testing