Possible fix for issue2294: don't pickle hooks#2295
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2295 +/- ##
==========================================
- Coverage 62.75% 62.72% -0.03%
==========================================
Files 130 130
Lines 16835 16847 +12
==========================================
+ Hits 10564 10568 +4
- Misses 6271 6279 +8
Continue to review full report at Codecov.
|
|
This, works, but I don't totally understand why. If masked quantities are un-pickleable, why does the deleting the hooks before pickling help? |
|
The masked quantities are only present in the hooks. The intent of the caching is to pickle the raw |
|
Oh, that makes sense. Then this fix looks great and I don't see why it would break anything else. But definitely @bsipocz should way in before you merge. |
bsipocz
left a comment
There was a problem hiding this comment.
I wonder why we didn't see this before, did anything change upstream? Also, I wonder whether we could test this?
I don't see any issues with the PR itself, but share the sentiment that this is a weird error and am not sure whether the PR would not break accidentally something. I run the remote test, they don't look more broken than before this PR.
Could you add a changelog entry to the last, infra section though?
|
I think the reason we never saw it before is that it is not very common for astroquery objects to include quantities - this seems to be a thing only for MPC, and only sometimes? And maybe there are only some rare objects that are unpicklable? I'll add the changelog entry |
yes, maybe it's worth reporting upstream to core. |
|
Rather than opening a separate issue I did the upstream reporting here, as this seems something very similar: astropy/astropy#12214 |
|
Hello @keflavich! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-02-19 02:15:43 UTC |
bsipocz
left a comment
There was a problem hiding this comment.
I've added a test that fails prior to this PR, and fixed the changelog.
|
Thanks @keflavich for the investigation and fix! |
|
Thanks, @keflavich ! |
This PR deletes hooks before pickling.
It fixes #2294 but I haven't checked if it breaks anything else.
@bsipocz @ceb8 thoughts? #2294 is a weird problem.