-
Notifications
You must be signed in to change notification settings - Fork 167
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
handle dataclass field type sentinels #513
handle dataclass field type sentinels #513
Conversation
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.
We started running into this over in Dask when sending dataclasses around. @ogrisel @pierreglaser I wonder if this is something you have bandwidth to take a look at? I'm not super familiar with the dataclass logic here, but can confirm it resolves the serialization issue I've seen recently.
@jrbourbeau do you have a minimal reproducer that involves dask? That would help me review this PR. EDIT: hopefully the new test in e15fb62 covers both the original bug report in #386 and your use case with dask. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #513 +/- ##
==========================================
+ Coverage 83.28% 83.44% +0.16%
==========================================
Files 4 4
Lines 718 725 +7
Branches 157 157
==========================================
+ Hits 598 605 +7
Misses 95 95
Partials 25 25
☔ View full report in Codecov by Sentry. |
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.
The dowstream CI config is broken (for unrelated causes) for joblib, dask and ray and would require some maintenance effort.
The PR itself looks good to me: the changes are not invasive and I checked that it actually fixes problem from the original report.
Thank you very much @rmorshea! |
Closes: #386
In short, the
dataclasses
module relies on the identity of certain sentinel values being preserved. To preserve their identity, this PR relies on a minimal set of internal dataclasses APIs to reduce those sentinels to their_FIELD_BASE.name
. Then, when reconstituting them, the names key into a_DATACLASSE_FIELD_TYPE_SENTINELS
mapping that contains the expected sentinel instances.The implementation accesses the following private APIs:
dataclasses._FIELD_BASE
dataclasses._FIELD
dataclasses._FIELD_CLASSVAR
dataclasses._FIELD_INITVAR