Skip to content
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

Use partial to make cube pickleable #4377

Merged
merged 7 commits into from
Jan 6, 2022

Conversation

wjbenfold
Copy link
Contributor

@wjbenfold wjbenfold commented Oct 15, 2021

🚀 Pull Request

Description

Where previously a function was defined within cube.convert_units if the cube had lazy data, now a partial object is created instead.

The definition of a function at runtime was preventing pickling of a cube that had lazy data and was awaiting a convert_units operation. This is no longer the case. (fixes #4354)


Consult Iris pull request check list

Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjbenfold This is lovely 🤩

But let's add some pickling test coverage, as there's clearly a gap that definitely needs filled 👍

Then let's bank this. Great job!

@wjbenfold
Copy link
Contributor Author

Not sure this is the right place to put these tests - it's a bit odd that we're not testing a function of cube.

Checked that the third test fails against the version of cube.py on main.

Not sure if it's worth checking any other cube methods preserve the ability to pickle, none stood out as likely to affect it.

Will rebase and update what's new when @bjlittle is happy with everything else

@pp-mo
Copy link
Member

pp-mo commented Oct 19, 2021

Not sure this is the right place to put these tests

It might be better to add them onto the existing : iris.tests.test_pickling or iris.tests.integration.test_pickle.
I really wouldn't add any more to iris.tests.unit.cube.test_Cube, if it can be helped !

@wjbenfold
Copy link
Contributor Author

It might be better to add them onto the existing : iris.tests.test_pickling

Done, thanks!

@lbdreyer
Copy link
Member

@wjbenfold Do you mind also resolving the conflicts on this PR?

@wjbenfold wjbenfold force-pushed the wjbenfold_pickle_fix branch from aaf4030 to 5347161 Compare November 26, 2021 17:19
@wjbenfold
Copy link
Contributor Author

After more faffing than strictly necessary, this is now rebased

@bjlittle
Copy link
Member

bjlittle commented Jan 5, 2022

@wjbenfold Absolutely not your doing... but due to our tardy efforts to review this PR it's gone stale again 😢

Please rebase to resolve the conflict, then we'll get it across the line 🙏

@wjbenfold wjbenfold force-pushed the wjbenfold_pickle_fix branch from bfc409a to 7b209f8 Compare January 5, 2022 15:39
@bjlittle bjlittle merged commit a18a57c into SciTools:main Jan 6, 2022
@wjbenfold wjbenfold deleted the wjbenfold_pickle_fix branch January 6, 2022 16:25
tkknight added a commit to tkknight/iris that referenced this pull request Jan 6, 2022
* main:
  Use partial to make cube pickleable (SciTools#4377)
  Add edit via github method to Iris docs (SciTools#4461)
  Broken cartopy links in docs (SciTools#4464)
  rtd with latest mambaforge image for faster building (SciTools#4476)
  Show acceptable image test results in the docs (SciTools#4392)
  Nc load latlon fix (SciTools#4470)
  update matplotlib links (SciTools#4474)
  Whatsnew for PR 4462 (SciTools#4475)
  Clarify the return type of iris.load (AVD-1899) (SciTools#4462)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4472)
  Updated environment lockfiles (SciTools#4467)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert_units breaks pickle/multiprocessing for lazy data
4 participants