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

Msgpack 1.0 compatibility #3494

Merged
merged 8 commits into from
Feb 18, 2020
Merged

Msgpack 1.0 compatibility #3494

merged 8 commits into from
Feb 18, 2020

Conversation

jrbourbeau
Copy link
Member

@jrbourbeau jrbourbeau commented Feb 18, 2020

In msgpack 1.0 the default value for strict_map_key was changed from False to True (see https://github.com/msgpack/msgpack-python#major-breaking-changes-in-msgpack-10). This causes failures for us as we use map keys which are not bytes or str. This PR updates our use of msgpack.loads to include strict_map_key=False

Closes #3491

@mrocklin
Copy link
Member

Thanks for handling this @jrbourbeau

@jrbourbeau
Copy link
Member Author

CI has passed against both msgpack 1.0 and a pre-1.0 release to check for backwards compatibility

I've also set 0.6.0 as the minimum supported msgpack version (released in Nov 2018) because that's when the strict_map_key option was added. This seems reasonable to me, but if there's reason to think this 0.6.0 restriction is too aggressive we can add some additional logic surrounding the msgpack version to support older versions too

@jrbourbeau jrbourbeau changed the title Update default msgpack options Msgpack 1.0 compatibility Feb 18, 2020
@TomAugspurger
Copy link
Member

Requiring>=0.6 sounds fine.

@jrbourbeau
Copy link
Member Author

Great, I just set the minimum version for our CI environments and will merge this PR when the current set of CI builds pass

@jrbourbeau
Copy link
Member Author

The test failure seems unrelated and I've raised a separate issue for it (xref #3498)

@jrbourbeau jrbourbeau merged commit a04a632 into dask:master Feb 18, 2020
@jrbourbeau jrbourbeau deleted the msgpack-compat branch February 18, 2020 22:47
@Carreau
Copy link
Contributor

Carreau commented Feb 18, 2020

May I suggest a new build/build number on conda-forge that update the metadata info to pin msgpack to <1.0.0 ? I've read recently that there should be a simpler way to do that via repodata patch, but I haven't used that before.

@mrocklin
Copy link
Member

mrocklin commented Feb 18, 2020 via email

@jakirkham
Copy link
Member

A PR to the conda-forge feedstock to do that would be welcome 🙂

@Carreau
Copy link
Contributor

Carreau commented Feb 19, 2020

A PR to the conda-forge feedstock to do that would be welcome 🙂

Do you know how to patch the repodata, or do I just bump the build number ?

@Carreau
Copy link
Contributor

Carreau commented Feb 19, 2020

See conda-forge/distributed-feedstock#114

jakirkham pushed a commit to conda-forge/distributed-feedstock that referenced this pull request Feb 19, 2020
@jakirkham
Copy link
Member

Thanks for doing that. Merged! 😄

@jakirkham
Copy link
Member

Do you know how to patch the repodata, or do I just bump the build number ?

Ah sorry missed this bit.

The patches live here. Though bumping the build number works also and may be a bit more straightforward.

@jrbourbeau
Copy link
Member Author

Thanks for updating the conda-forge recipe @Carreau!

@Carreau
Copy link
Contributor

Carreau commented Feb 19, 2020

Jakirkham, I'm more thinking that all previous version of distributed (or at least a few of those) should get msgpack<1;
Bumping the build only affect latest one. It's something that I think will come up more and more often to update dependencies compat after the releases. Hence why I was thinking about repodata patch.

@jakirkham
Copy link
Member

Yeah hot-fixing would still be appropriate Matthias 🙂

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.

distributed breaks with msgpack 1.0.0rc1
5 participants