maintenance updates#125
Merged
jameshiebert merged 31 commits intomasterfrom Jun 11, 2025
Merged
Conversation
6 tasks
jameshiebert
requested changes
May 20, 2025
Contributor
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
I'm concerned about the various warnings that SQLAlchemy is complaining about the various relationships not being properly specified. If we're going through the trouble of doing the upgrades, these warnings should be much more sparse than they are. Can you double check that the relationships are coded correctly? The warnings advise "consider if these relationships should be linked with back_populates, or if viewonly=True should be applied to one or more if they are read-only"
An attempt to mitigate the following errors: Exception ignored in: 'netCDF4._netCDF4.Dataset.__dealloc__' AttributeError: __getattribute__ AttributeError: __getattribute__ This patch includes code to attempt to avoid memoizing open NetCDF files as well as some timing code to benchmark test results using (or not) memoization.
The memoization code in index_netcdf.py was both problematic and ineffective. As part of the keys to the cache, it used open NetCDF file objects and as part of the values of the cache were Netcdf variable objects (which also contained references to the NetCDF files). On exit, the main branch of the code would close and clean up the files, leaving everything in the cache stranged. When the cache got cleaned up, it would leave a long string of error messages: Exception ignored in: 'netCDF4._netCDF4.Dataset.__dealloc__' AttributeError: __getattribute__ AttributeError: __getattribute__ for each stranded object. While this didn't necessarily cause any real errors at execution time, it was messy. Furthermore, the expensive part of indexing is I/O: going out to read data from the disk and going out to the database. The memoization, as written, didn't actually mitigate this. It only cached objects, referencing the file. I measured the differences in the test suite between memoized and non-memoized and it was on the order of miliseconds (noise). I recommend simply removing this code, as this patch does.
Contributor
Author
|
Demo using this versiom, with data indexed by this version. Seems to be working. |
Contributor
|
Amazing! Everything looks great to me. Thanks so much @corviday . I'll go ahead and merge this, run black and do the release. |
jameshiebert
approved these changes
Jun 11, 2025
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
2 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.
Task list:
resolves #124
resolves #110
blackshould be run after review and before merge, but we don't need a bunch of formatting changes cluttering up this PR.