Page MenuHomePhabricator

eliminate Persistable
Needs RevisionPublic

Authored by sandyryza on Mon, Dec 28, 9:41 PM.

Details

Summary

typing.NamedTuples don't play well with double inheritance. Eliminating Persistable will allow us to convert all our serializable types to typing.NamedTuples, which makes them way easier to grok.

Depends on D5768.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
typing-events-namedtuple (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sandyryza added a reviewer: alangenfeld.
sandyryza added a reviewer: max.
alangenfeld added a subscriber: prha.
alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/events.py
34

coming from https://dagster.phacility.com/D3809 there was an angle on this "persistence" name that I don't think we ever followed up on, and so far its just been used to override the serialization behavior

given this diff, I think it may makes sense just to consolidate to only having whitelist_for_serdes with an optional serializer (or custom_serializer?) argument

cc @prha

i will defer to alex on this; maybe i should just mention that i continue to suspect that we might be well served by adopting the pickle protocol (__getnewargs_ex__/__getstate__/__setstate__)

typing.NamedTuples don't play well with double inheritance

thats unfortunate, but Peristable is really the only time we do this currently?

python_modules/dagster/dagster/core/definitions/events.py
34

Yeah, didn't ever get around to it, but the plan was to mark every persisted named tuple as persistable and enforce via checks at the storage persistence boundaries.

That's so each data type is documented as either serializable so that it can be persisted, or serializable just to pass across a process boundary (helping with decisions about how backwards-compatible changes need to be).

Given that the vision was never realized, I think I'm okay with dropping that. We can figure out a different way to pass along a persistence flag.

lets go for the consolidation while we're making changes, should just be easy enough

python_modules/dagster/dagster/core/definitions/events.py
34

I think the separate decorators would be a good API - just needs to come with the enforcement

This revision now requires changes to proceed.Tue, Jan 12, 9:34 PM