Skip to content

Conversation

@mariusvniekerk
Copy link
Collaborator

Alternate to #2000

@mrocklin
Copy link
Member

This is cool, however I worry slightly about going outside of the standard msgpack spec. Hypothetically systems in other languages should be able to talk to the scheduler and workers over msgpack. The more complex we make things here the harder that becomes. I'm inclined to keep things as close to stock as possible if there is no strong need otherwise. Do we have strong need to move around sets and frozensets?

@mariusvniekerk
Copy link
Collaborator Author

So clients that don't implement special handling for these types will receive an ext_type which is part of the spec, which is just (code, msgpackabe-payload), If we make that payload looks a bit nicer say something like {''__type__": "tuple", "val": val} clients that do want to somewhat interact with extension types should be safe.
The one thing that is true that under the msgpack spec client implementations are free to do with ext types what they will, discard them etc.
As for the sets / frozensets i dont think that is too important, It might be handy to have one set-like representation, but two is probably overkill.

@mariusvniekerk
Copy link
Collaborator Author

One other problematic area here is we lose the ability to serialize defaultdict and ordereddict without some additional work, which is kinda silly

@mrocklin
Copy link
Member

Perhaps we first try the approach in #2000 and see if it suffices? If so then we call it a day?

Things like defaultdicts are, I think, entirely out of scope for how we use msgpack. I think that it's important to keep in mind that we only use msgpack here to serialize administrative messages. We shouldn't need that much variety when using it.

@mariusvniekerk
Copy link
Collaborator Author

mariusvniekerk commented May 24, 2018

Yep, lets try that (also the test failures here are from serializing defaultdicts :P )

@mrocklin
Copy link
Member

OK to close this @mariusvniekerk ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants