fix(datasets): parse CMIP ingest in processes so --n-jobs > 1 is safe#796
Conversation
|
Warning Review limit reached
Next review available in: 13 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Catalog parsing fanned files across a ThreadPoolExecutor, but the netCDF4/HDF5 wheels published on PyPI are built without --enable-threadsafe. The default `complete` CMIP6 parser opens every file, so `ref datasets ingest --n-jobs > 1` opened HDF5 files from multiple threads concurrently and reliably crashed the interpreter with SIGSEGV/SIGBUS. Parsing now runs in worker processes via ProcessPoolExecutor using the "spawn" start method (matching the LocalExecutor). Each interpreter opens files single-threaded, which is safe, and sidesteps the GIL so I/O-bound parsing scales across a parallel filesystem. Worker count is capped at the file count and -1 still means all CPUs. Shared picklable stub parsers moved into conftest_plugin so spawn/forkserver workers can re-import them; added regression tests that prove parsing runs out-of-process and that the file-opening complete parser survives parallel parsing and matches sequential output. Constraint: netCDF4/HDF5 PyPI wheels are not thread-safe (no --enable-threadsafe) Rejected: ThreadPoolExecutor | segfaults on concurrent HDF5 opens Rejected: forkserver default | inconsistent with LocalExecutor's spawn choice Confidence: high Scope-risk: narrow Not-tested: behaviour on an HDF5 build compiled with --enable-threadsafe
lewisjared
force-pushed
the
fix/cmip-ingest-parallel-process-pool
branch
from
5bd6d46 to
9e0ae24
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Problem
ref datasets ingest --n-jobs > 1reliably crashes the interpreter (SIGSEGV) with the defaultcompleteCMIP6 parser.Catalog parsing fanned files across a
ThreadPoolExecutor, but the netCDF4/HDF5 wheels published on PyPI are built without--enable-threadsafe. Thecompleteparser opens every file withnetCDF4.Dataset(...), so parsing across threads opened HDF5 files concurrently and corrupted HDF5's global state.Fix
Parsing now runs in worker processes via
ProcessPoolExecutorusing thespawnstart method (matchingLocalExecutor). Then each open is single-threaded, but parallel.On a fast local filesystem the parallism isn't worth the overhead of setting up the process pool, but for parallel filesystems such as Lustere or NFS the parallisation helps.
Tests
New
TestParallelParsingSafetyintest_catalog_builder.py:completeparser in parallel over genuine HDF5 files and asserts no crash + correctness,Shared picklable stub parsers moved into
conftest_pluginsospawn/forkserverworkers can re-import them.Not covered
Behaviour on an HDF5 build compiled with
--enable-threadsafe(processes are safe there too, just not the failure mode being fixed). Maybe that is useful for some HPCs, but I deferred this until we have a request.