Unpin dask and pin it to >=2025.1.0 and unSKIP doctests#6312
Closed
valeriupredoi wants to merge 4 commits intoSciTools:mainfrom
Closed
Unpin dask and pin it to >=2025.1.0 and unSKIP doctests#6312valeriupredoi wants to merge 4 commits intoSciTools:mainfrom
dask and pin it to >=2025.1.0 and unSKIP doctests#6312valeriupredoi wants to merge 4 commits intoSciTools:mainfrom
Conversation
|
|
trexfeathers
requested changes
Feb 11, 2025
Contributor
There was a problem hiding this comment.
Despite what the README says, please update these lock-files by using conda-lock or conda list --explicit
Contributor
Author
shall do now 🍺 |
github-actions
bot
added
the
benchmark_this
Contributor
Author
updated them in ee6825e by using your own Lemme know if I should rebuild them with |
trexfeathers
previously approved these changes
Feb 12, 2025
trexfeathers
dismissed
their stale review
I didn't realise the tests hadn't run yet!
Contributor
⏱️ Performance Benchmark Report: 0f83317Performance shiftsFull benchmark resultsGenerated by GHA run |
Contributor
Author
|
thanks for looking @trexfeathers - you guys know what's the hap with those three failed tests, or should I investigate the fails meself? 🍺 |
Contributor
Author
|
SOF suggests a -1 pickle protocol |
Contributor
I know no more than you do on this one, so if you have time to investigate, it won't be a waste! |
Contributor
Author
|
perfect, shall do! Pytest fails are my specialty, shoot them down like turkeys 🦃 😁 |
Contributor
Author
|
OK so the MRE for this bugger is this: import pickle
import iris
def repro_failed_test():
pth = "/home/valeriu/iris_fork/lib/iris/test_data/NetCDF/global/xyt/SMALL_hires_wind_u_for_ipcc4.nc"
cube = iris.load(pth)[0]
with open("./testpkl", "wb") as f:
pickle.dump(cube, f, 1)
repro_failed_test()obv that file is the same as tested for, from the iris test data repo. I shall now proceed to figuring out what package is causing this. |
Contributor
Author
|
Yup! It is DASK at fault here:
'Twas a Christmas change that brought the bug down the chimney! Full deps diff: Now, there is a workaround for this if you guys want to adopt it - use only |
Contributor
|
See also: #6258 Sorry I didn't remember it before - it was pre-holiday! Anyway all I had done was write it up with no investigation, so no time wasted. |
Contributor
Author
|
BTW same behaviour if using But, here's the thing: using protocols 2 onward until and inclusive of maximum 5 all works well, am thinking 0 and 1 are veeerry old, maybe there's no need to use them at all? |
Contributor
|
Had a conversation with @bjlittle and @pp-mo. We're comfortable altering the test to only test protocols We don't expect the above to be a breaking change, although it would be worth warning users about in a What's New entry. Not sure how to solve this one final error:
Unpinning is scheduled for our upcoming Iris 3.12 work, so we're happy to take it from here if you want. But you're welcome to keep going if you prefer. |
Contributor
Author
|
sounds very reasonable that way indeed! Honestly, I wouldn't know how to get the protocol value in a dynamical manner, but the |
Contributor
Author
|
ah that Task-related fail is pretty clear if we look at what
They changed massively the graphing in 2024.12 - see dask/dask#11569 The test shouldn't index what is now a Task, so I think that's also doable easily. Ah the joys of Dask making frequent massive backend and API changes, not very well tested, and letting everyone in the dust 😁 |
Contributor
Author
|
@trexfeathers @bjlittle how do you guys plan on proceeding from here? I can make the changes in the test - for pickling it's a straightforward case, but for the |
Contributor
We can take it from here. Thanks for all your work on this |
Contributor
Author
sounds good! Feel free to use this branch too if it's more convenient 🍻 Let me know how else I can help with ie testing etc |
Contributor
⏱️ Performance Benchmark Report: 24780c0Performance shiftsFull benchmark resultsGenerated by GHA run |
Merged
Contributor
|
I believe the work here was built upon and completed in #6342, so there's no longer any need for this PR. |
Contributor
Author
|
yeps, cheers, folks 🍺 |
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
4 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
Dask solved their severe issue reported in #6251 in 2025.1.0, and unpinning - and pinning it to the earliest version that solves that issue was raised in #6264
I am also unSKIPping the doctests that @trexfeathers mentioned in #6264 (comment)
Closes #6264
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: