NetCDF thread safety take two #5095

trexfeathers · 2022-12-06T16:21:07Z

🚀 Pull Request

Description

Taking the lead from #5061, I have minimised the footprint of this change, making it hopefully easier for future maintenance and better suited to a patch release (our goal).

I still get very occasional segfaults and invalid ID NetCDF errors. These were guaranteed before, but I would now estimate they fail a full test run ~ 1 in 20 times. This of course makes them very difficult to investigate - I don't know if there are any commits in this PR's sequence where there was genuinely no problem, or whether it was just luck. The only working theory I have is that they show up when certain combinations of tests are being run on separate processors, but I don't know how I would test such a theory.

I'm putting this up now because I need help with what's left:

Investigate remaining intermittent errors (see above) and decide if they are a problem.
Write test(s) specifically aimed at thread safety.
Arrange for some real world user testing.

Consult Iris pull request check list

pp-mo · 2023-02-14T18:31:13Z

Ok for your latest !
I'm totally 👍 on the change to a context managerdask.config.set -- I probably should have spotted that + raised it myself.
Still looking forward to some notes about when they fail...

trexfeathers · 2023-02-15T12:38:44Z

Test results

Tests: pytest lib/iris/tests/integration/netcdf/test_thread_safety.py
Code base: either trexfeathers/netcdf_segs or v3.4.0
Environment specification: either trexfeathers/netcdf_segs or v3.4.0
NetCDF4 versions:
- netcdf_segs: 1.6.2
- v3.4.0: 1.6.0

(☟ code) (environment ☞)	`netcdf_segs`	`v3.4.0`
`netcdf_segs`	✔️ x5	✔️ x5
`v3.4.0`	❌ `test_realise_data()`, `test_comparison()` - seg-fault *	✔️ x5

Notes

Needed to use pytest -k to identify the specific seg-faulting test.
test_thread_safety.py obviously doesn't exist in v3.4.0 so I had to bring that in manually when running with the v3.4.0 code base.

trexfeathers · 2023-02-15T17:49:34Z

Discoveries from investigating tests

More, smaller threads increase the chance of thread collision. So tests using tiny_chunks fail more reliably than _multisource tests.
Tests involving da.store() rather than da.compute() - so those for saving and streaming - NEVER fail, even when streaming from multiple sources. This suggests that Dask's [current] strategy is to serialise the WHOLE operation whenever file writing is involved.
Good to have those tests in there should the situation ever change in future.
Inserting a sleep(random()) line into the NetCDFDataProxy does not produce more thread collisions. In fact it looks like it decreases them, possibly by desynchronising some moments during computation that are naturally synched up.

trexfeathers · 2023-02-20T11:53:20Z

Closes #5016

valeriupredoi

very very many thanks for tackling this @trexfeathers and everyone else who reviewed and tested - I am approving this from the side of us doing testing with ESMValCore, tests that show this fixes the issue completely; of course, you guys decide when to merge, but it'd be very cool to have this in a new iris release soon 🍻 🥳

lib/iris/tests/integration/netcdf/test_thread_safety.py

trexfeathers · 2023-02-20T17:11:30Z

Do you need anything else @pp-mo?

pp-mo · 2023-02-20T17:57:32Z

Do you need anything else @pp-mo?

No, all good now the tests have passed.
Huge applause ! 👏 👏 🥇

* Unpin netcdf4. * Temporarily enable GHA on this branch. * Temporarily enable GHA on this branch. * Temporarily enable GHA on this branch. * Experiment to disable wheel CI on forks. * Disable segfaulting routines. * More temporary changes to get CI passing. * More temporary changes to get CI passing. * Finessed segfault skipping. * Bring in changed from SciTools#5061. * Re-instate test_load_laea_grid. * Adaptations to get the tests passing. * Use typing.Mapping instead. * Get doctests passing. * CF only resolve non-url filenames. * Confirm thread safety fixes. * Remove dummy assignment. * Restored plot_nemo What's New entry. * _add_aux_factories temporarily release global lock. * Remove per-file locking. * Remove remaining test workarounds. * Remove remaining comments. * Correct use of CFReader context manager. * Refactor for easier future maintenance. * Rename netcdf _thread_safe, add header. * Full use of ThreadSafeAggregators. * Full use of ThreadSafeAggregators. * Remove remaining imports of NetCDF4. * Test to ensure netCDF4 is via _thread_safe module. * More refined netcdf._thread_safe classes. * _thread_safe docstrings. * Restore original NetCDF code where possible. * Revert changes to 2.3.rst. * Update lockfiles. * Additions to _thread_safe.py * Remove temporary CI shims. * New locking stategy for NetCDFDataProxy. * NetCDFDataProxy simpler use of netCDF4 lock. * Update lock files. * Go back to using a Threading Lock. * Remove superfluous pass commands in test_cf.py. * Rename _thread_safe to _thread_safe_nc. * Rename thread safe classes to be 'Wrappers'. * Better contained getattr and setattr pattern. * Explicitly name netCDF4 module in _thread_safe_nc docstring. * Better docstring for _ThreadSafeWrapper. * Better comment about THREAD_SAFE_FLAG. * list() wrapping within _GLOBAL_NETCDF4_LOCK, to account for generators. * More accurate thread_safe docstrings in netcdf.saver. * Split netcdf integration tests into multiple modules. * Tests for non-thread-safe NetCDF behaviour. * Docstring accuracy. * Correct use of dask config set (context manager). * Update dependencies. * Review - don't need the first-class import of iris.tests. * Better name for the loading test. * Better selection of data to load. * What's New entry. * Improve tests. * Update lock files. * Increase chunking on test_save. --------- Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>

* upstream/main: (23 commits) Lockfiles and pydata-sphinx-theme fix (SciTools#5188) Allow smarter weights (cubes, coordinates, cell measures, or ancillary variables) for aggregation (SciTools#5084) removed cell measure mask check and error (SciTools#5181) Updated environment lockfiles (SciTools#5177) Lazy weighted RMS calculation (SciTools#5017) Add coverage badge to README.md (SciTools#5176) Add coverage testing (SciTools#4765) Whats new updates for v3.4.1 . NetCDF thread safety take two (SciTools#5095) Updated environment lockfiles (SciTools#5163) Plugin support (SciTools#5144) Expand scope of common contributor links (SciTools#5159) Replace apparently retired UDUNITS documentation link. (SciTools#5153) [pre-commit.ci] pre-commit autoupdate (SciTools#5150) Fixing typo's in Gitwash. (SciTools#5145) add readme #showyourstripes (SciTools#5141) [pre-commit.ci] pre-commit autoupdate (SciTools#5143) Iris ❤ Xarray docs page. (SciTools#5025) [pre-commit.ci] pre-commit autoupdate (SciTools#5136) Updated citation (SciTools#5116) ...

trexfeathers and others added 30 commits November 22, 2022 14:48

Unpin netcdf4.

6caae04

Temporarily enable GHA on this branch.

9dae5d0

Temporarily enable GHA on this branch.

cdcd0e2

Temporarily enable GHA on this branch.

1512708

Experiment to disable wheel CI on forks.

2dc3aff

Disable segfaulting routines.

c5ab0e4

More temporary changes to get CI passing.

235e5fa

More temporary changes to get CI passing.

efaab93

Finessed segfault skipping.

301dbc6

Bring in changed from SciTools#5061.

9ece95c

Re-instate test_load_laea_grid.

5103545

Adaptations to get the tests passing.

95f337a

Use typing.Mapping instead.

fa82db7

Get doctests passing.

b6756d4

CF only resolve non-url filenames.

256936c

Confirm thread safety fixes.

cf9ba29

Remove dummy assignment.

aed32c8

Restored plot_nemo What's New entry.

835c02d

_add_aux_factories temporarily release global lock.

4e2b590

Remove per-file locking.

a8819b4

Remove remaining test workarounds.

8207d6d

Remove remaining comments.

c0faa6f

Correct use of CFReader context manager.

8010b2f

Refactor for easier future maintenance.

2fb274f

Rename netcdf _thread_safe, add header.

3aaaa1a

Full use of ThreadSafeAggregators.

02d9a8c

Full use of ThreadSafeAggregators.

b301120

Remove remaining imports of NetCDF4.

6b7c15c

Test to ensure netCDF4 is via _thread_safe module.

ab733a7

More refined netcdf._thread_safe classes.

d066d39

trexfeathers added 3 commits February 15, 2023 11:53

Review - don't need the first-class import of iris.tests.

f6eb131

Better name for the loading test.

2df6278

Better selection of data to load.

2095658

trexfeathers added 2 commits February 15, 2023 14:01

What's New entry.

e03659a

Improve tests.

93ac4c4

valeriupredoi approved these changes Feb 20, 2023

View reviewed changes

trexfeathers linked an issue Feb 20, 2023 that may be closed by this pull request

Investigate segfaults with NetCDF4 >1.6.0 #5016

Closed

trexfeathers added 2 commits February 20, 2023 15:03

Merge remote-tracking branch 'upstream/main' into netcdf_segs

3ffe3dc

Update lock files.

94da55c

pp-mo approved these changes Feb 20, 2023

View reviewed changes

lib/iris/tests/integration/netcdf/test_thread_safety.py Outdated Show resolved Hide resolved

Increase chunking on test_save.

a6b893c

pp-mo merged commit 11da71b into SciTools:main Feb 20, 2023

github-actions bot mentioned this pull request Feb 21, 2023

Performance Shift(s): 11da71b7 #5167

Closed

rcomer mentioned this pull request Feb 21, 2023

Segmentation fault when differencing cubes #2063

Closed

This was referenced Feb 21, 2023

NetCDF thread safety for v3.4.1 patch release #5169

Merged

v3.4.1 'merge-back' - What's New changes only #5170

Merged

trexfeathers deleted the netcdf_segs branch February 24, 2023 13:54

ocefpaf mentioned this pull request Mar 13, 2023

use numpy >=1.20 pyoceans/pocean-core#70

Merged

pp-mo mentioned this pull request Mar 14, 2023

HDF5-DIAG warnings from libnetcdf v4.9.1 #5187

Closed

This was referenced Mar 27, 2023

Bridge support #5216

Closed

Support netcdf variable emulation #5212

Merged

pp-mo mentioned this pull request Apr 17, 2023

Connect netCDF4 locks to Xarray #5251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetCDF thread safety take two #5095

NetCDF thread safety take two #5095

trexfeathers commented Dec 6, 2022 •

edited

Loading

pp-mo commented Feb 14, 2023

trexfeathers commented Feb 15, 2023 •

edited

Loading

trexfeathers commented Feb 15, 2023 •

edited

Loading

trexfeathers commented Feb 20, 2023

valeriupredoi left a comment

trexfeathers commented Feb 20, 2023

pp-mo commented Feb 20, 2023

NetCDF thread safety take two #5095

NetCDF thread safety take two #5095

Conversation

trexfeathers commented Dec 6, 2022 • edited Loading

🚀 Pull Request

Description

pp-mo commented Feb 14, 2023

trexfeathers commented Feb 15, 2023 • edited Loading

Test results

Notes

trexfeathers commented Feb 15, 2023 • edited Loading

Discoveries from investigating tests

trexfeathers commented Feb 20, 2023

valeriupredoi left a comment

Choose a reason for hiding this comment

trexfeathers commented Feb 20, 2023

pp-mo commented Feb 20, 2023

trexfeathers commented Dec 6, 2022 •

edited

Loading

trexfeathers commented Feb 15, 2023 •

edited

Loading

trexfeathers commented Feb 15, 2023 •

edited

Loading