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

Add optional support for conversion from indexmap::IndexMap #1728

Merged
merged 23 commits into from
Jul 22, 2021
Merged

Add optional support for conversion from indexmap::IndexMap #1728

merged 23 commits into from
Jul 22, 2021

Conversation

IvanIsCoding
Copy link
Contributor

Add support for converting indexmap::IndexMap into &PyDict under an optional feature

IndexMap is a hash table that is closely compatible with the standard HashMap, with the difference that it preserves the insertion order when iterating over keys. It was inspired by Python's 3.6+ dict implementation.

The use case for this new dict conversion is for having a more deterministic iteration behaviour when passing hash tables between Rust and Python. Because of the guarantees Python makes about dictionaries, some users are puzzled that looping through a dict or calling .items() is not identical on multiple runs of a function that originally returned a HashMap

@IvanIsCoding
Copy link
Contributor Author

Also, this PR is almost identical to @mtreinish's #1114 but with indexmap::IndexMap instead of hashbrown::HashMap. I cut indexmap::IndexSet from this though because converting into &PySet does not preserve the insertion order, it didn't make as much sense to include it.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I agree it's a useful feature and worth supporting. It could also be worth adding a note in the guide somewhere in the conversions section, as I suspect users might want this from time to time.

Few nits to follow...

src/types/dict.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@mejrs
Copy link
Member

mejrs commented Jul 17, 2021

Thanks!

Would you be willing to add something like this with a) an example of the cargo.toml that people can copy paste b) a doc example illustrating why people would want to use it (probably a quick example of the conversion maintaining insertion order)?

Also, python dictionaries maintaining insertion order is only guaranteed on 3.7 and up - from https://docs.python.org/3/library/stdtypes.html :

Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.

It doesn't look like a problem to me, but is this something we should be concerned about?

@IvanIsCoding
Copy link
Contributor Author

Thanks!

Would you be willing to add something like this with a) an example of the cargo.toml that people can copy paste b) a doc example illustrating why people would want to use it (probably a quick example of the conversion maintaining insertion order)?

Also, python dictionaries maintaining insertion order is only guaranteed on 3.7 and up - from https://docs.python.org/3/library/stdtypes.html :

Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.

It doesn't look like a problem to me, but is this something we should be concerned about?

I will try to add an example like you listed, I will come up with some use cases.

For the 3.6 vs 3.7 detail. CPython has insertion order since 3.6, and PyPy had it even before. That covers all Python 3.6 implementations PyO3 supports no?

So I think we can add in the indexmap section that:

Since Python 3.7, Dictionary order is guaranteed to be insertion order and the conversions with IndexMap will reflect that guarantee. In practice, the guarantee will also be mainted in Python 3.6 because of the implementation details of CPython and PyPy.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, just some tiny nits and it might be nice to squash the commits if you're willing to do that too.

EDIT: I see there is a suggestion for more documentation too, which would also be much appreciated. It can go at the top of indexmap.rs as an inline comment.

src/conversions/indexmap.rs Outdated Show resolved Hide resolved
src/conversions/mod.rs Outdated Show resolved Hide resolved
src/conversions/indexmap.rs Outdated Show resolved Hide resolved
}

#[test]
fn test_indexmap_indexmap_insertion_order_round_trip() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is great!

Cargo.toml Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

👍 looking great. Just waiting on a doc at the top of indexmap.rs as per #1728 (comment), and then this is ready to merge!

Thanks very much for your work and refinements on this!

@IvanIsCoding
Copy link
Contributor Author

looking great. Just waiting on a doc at the top of indexmap.rs as per #1728 (comment), and then this is ready to merge!

Thanks very much for your work and refinements on this!

I have added the doc at the top of the file, hopefully it gets the point accross

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 💖

Is this documentation reachable in the docs? It's in a private module, so it's not (?) visible in the documentation, and there's no link to it like the other features for conversions.

src/conversions/indexmap.rs Outdated Show resolved Hide resolved
src/conversions/indexmap.rs Outdated Show resolved Hide resolved
@IvanIsCoding
Copy link
Contributor Author

IvanIsCoding commented Jul 20, 2021

Thanks

Is this documentation reachable in the docs? It's in a private module, so it's not (?) visible in the documentation, and there's no link to it like the other features for conversions.

I edited lib.rs to export the module, so the documentation should be visible now

@davidhewitt
Copy link
Member

Looks brilliant. Thank you very much for your work on this!

@davidhewitt davidhewitt merged commit bd0e0d8 into PyO3:main Jul 22, 2021
@IvanIsCoding IvanIsCoding deleted the indexmap-support branch July 24, 2021 00:34
@IvanIsCoding IvanIsCoding restored the indexmap-support branch July 24, 2021 00:34
@IvanIsCoding IvanIsCoding deleted the indexmap-support branch August 12, 2021 15:42
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.

3 participants