NetCDF thread safety take two #5095
Merged
Conversation
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
Member
|
Ok for your latest ! |
Contributor
Author
Test results
Notes
|
Contributor
Author
Discoveries from investigating tests
|
Contributor
Author
|
Closes #5016 |
valeriupredoi
approved these changes
Feb 20, 2023
Contributor
valeriupredoi
left a comment
valeriupredoi
left a comment
There was a problem hiding this comment.
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 🍻 🥳
pp-mo
approved these changes
Feb 20, 2023
Contributor
Author
|
Do you need anything else @pp-mo? |
Member
No, all good now the tests have passed. |
trexfeathers
added a commit
to trexfeathers/iris
that referenced
this pull request
Feb 21, 2023
* 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 <[email protected]>
This was referenced Feb 21, 2023
This was referenced Mar 27, 2023
Closed
tkknight
added a commit
to tkknight/iris
that referenced
this pull request
Apr 22, 2023
* 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) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
🚀 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 andinvalid IDNetCDF 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:
Consult Iris pull request check list