-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lazy netcdf saves #5191
Lazy netcdf saves #5191
Conversation
Although there are no tests yet, I have created a simple routine to demonstrate function. Possibly a bit poor, but there it is ! |
Problem with fill-value checkingI hit a glitch here due to the different behaviour of dask execution with a distributed scheduler. So for now, I've completely fudged the issue:
This presents us with a bit of a backwards-compatibilty problem, in that I don't see how we can't check the data for fill-value collisions in the "new world". And while that may clearly be okay when using the new lazy-saving, I'm less comfortable that it will simply behave differently with a distributed scheduler. Another option is to continue to do the as-is inline streaming, but only if the scheduler is threaded. |
…distributed schedulers.
Rebased, and pinned libnetcdf<4.9, as found for #5187. |
Changed message on previous commit, and added a fix. There is one remaining test failure, The log looks like this ... Click to expand this section...
|
Ok so now all the existing tests are "fixed", for python < 3.10, and those logs don't seem to show any "HDF5-DIAG" warnings output. But we are still getting an error in the doctests, and specifically in the python3.10 tests Click to expand this section...
The occurrence in the actual tests for python 3.10 looks very similar -- and is not using NETCDF3. Click to expand this section...
Does this seem familiar territory @trexfeathers ??
|
…taProxy fix; use one lock per Saver; add extra up-scaled test
Updated with results of investigation by @pp-mo @trexfeathers. Hopefully this may fix the outstanding test failures. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5191 +/- ##
==========================================
+ Coverage 89.31% 89.32% +0.01%
==========================================
Files 88 89 +1
Lines 22279 22390 +111
Branches 5355 5374 +19
==========================================
+ Hits 19898 20000 +102
- Misses 1635 1640 +5
- Partials 746 750 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@bouweandela do you have time to run your checks through this, as you did with the original #5031 ? I know the tests still aren't all passing, but it's only code coverage warnings. |
Sincere apologies, we had a few remaining problems with this code in our recent work and it is still not merged. I think the problems are now resolved, and we expect to get it completed + merged quite soon now. I've put this at No.1 on the Iris 3.6 board.
Apologies again for the delay @bouweandela. We could make a special release for this, but I'm hoping that won't be necessary for your requirements ? |
Apologies for spurious mention of "1.6" and "1.7" releases -- now fixed ! |
Note on PR checking status: I think we can now accept that tests did pass+ will pass, |
Great to see that the release pace of iris is increasing @bjlittle! The reduced waiting time for new features will definitely make it more attractive to contribute and easier to use.
Unfortunately the ESMValTool release schedule currently has a release in June and the next release in October, so then our October release would be the first release where our users could benefit from the new feature, which would be disappointingly late. Let me check with the @ESMValGroup/esmvaltool-coreteam though, maybe we could postpone our release by a month. How sure are you that your next release will not be pushed back further? We would need at least 2 weeks between the iris release with this feature and our release for testing. |
I think this could be remedied by setting up a merge queue, maybe that could be an interesting feature for you? |
This is great to hear. Thanks for the feedback, it makes a big difference 👍
Again, thanks for sharing. This is really useful to know. It gives me the context I need to make some decisions about the next release. Just so you know, we have a 2 week (9 days only, thanks to the bank holiday for King Charles III coronation) development sprint scheduled, dedicated to the I just need to get a clear understanding from our side about the absolute minimal features that we want to include for the I see that your forthcoming It would be interesting to see if we could make that happen for you guys. It's quite tight, but not impossible, and pretty much all depends on content and resourcing. Leave it with me. I'll definitely be in touch very soon 😉 |
I believe that would still require @pp-mo to be aware of which pull requests affect |
Thanks people. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no outstanding comments so I believe this is now ready to merge!
@pp-mo could you sort out the lockfiles/whatsnew then ping me to let me now that I can merge it in?
Phew 😅 @lbdreyer please re-review when you can |
Merged 🥳 with quite possibly the longest commit message I've witnessed |
Thanks a lot for all the work on this! |
* upstream/main: Updated environment lockfiles (SciTools#5270) Drop python3.8 support (SciTools#5269) build wheel from sdist, not src (SciTools#5266) Lazy netcdf saves (SciTools#5191) move setup.cfg to pyproject.toml (SciTools#5262) Support Python 3.11 (SciTools#5226) Remove Resolve test workaround (SciTools#5267) add missing whatsnew entry (SciTools#5265)
* upstream/main: (61 commits) Updated environment lockfiles (SciTools#5270) Drop python3.8 support (SciTools#5269) build wheel from sdist, not src (SciTools#5266) Lazy netcdf saves (SciTools#5191) move setup.cfg to pyproject.toml (SciTools#5262) Support Python 3.11 (SciTools#5226) Remove Resolve test workaround (SciTools#5267) add missing whatsnew entry (SciTools#5265) make help (SciTools#5258) automate pypi manifest checking (SciTools#5259) drop sphinxcontrib-napoleon (SciTools#5263) add missing test result data (SciTools#5260) fix indentation and remove ref to ssstack (SciTools#5256) review actions update .git-blame-ignore-revs adopt codespell Adopt sphinx design (SciTools#5127) Bump scitools/workflows from 2023.04.2 to 2023.04.3 (SciTools#5253) refresh manual pypi publish instructions (SciTools#5252) Updated environment lockfiles (SciTools#5250) ...
Here at last, a followup to #5031 .
Note that, in addition to adding lazy saving, this adds support for the dask 'distributed' scheduler.
Unfortunately, this is still very much preliminary work, since I haven't added any testing yet
-- and doubtless various existing non-integration tests will break + need fixing, which could take some effort
It has important unfinished business :
no tests !(done)it now appears that, we can no longer support checking for inadvertent fill_value collisions, which is a serious backwards-incompatibily. Though possibly we can continue to do this for a threaded scheduler only. See below(done)no support for a (local) process-based scheduler. Though we can't do that (i.e. save with one) at present, so maybe no problem.(done : accept + document that we do not support it)we might also want to consider possible different process "start_method" settings : These are fork/spawn/forkserver as supported by multiprocessing, and dask also has a control for this -- see around "multiprocessing.context", here.(done : probably only matters for local-process-scheduling, which we are not doing)Closes #4190