Skip to content

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