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.

Sign up for GitHub

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

Jump to bottom

replace pyrsistent.PMap, immutables.Map, immutabledict with constantdict #884

Merged
merged 16 commits into from
Jan 30, 2025
Merged

replace pyrsistent.PMap, immutables.Map, immutabledict with constantdict #884

merged 16 commits into from
Jan 30, 2025

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Nov 21, 2024 •
edited
Loading

Followup of #832

Tree benchmark (#887) performance (Python 3.12, Mac M1):

Implementation Time [s]
pyrsistent.pmap 1.51
immutables.Map 0.41
constantdict 0.80
immutabledict 3.84

Please squash

@matthiasdiener matthiasdiener self-assigned this Nov 21, 2024
@alexfikl
Copy link
Contributor

I haven't kept up with the immutable benchmarking and stuff. Is immutabledict the best/fastest/preferred one now?

@matthiasdiener
Copy link
Collaborator Author

matthiasdiener commented Nov 22, 2024 •
edited
Loading

I haven't kept up with the immutable benchmarking and stuff. Is immutabledict the best/fastest/preferred one now?

I think my constantdict package is the best ;-), but loopy is already using immutabledict in some places...

@inducer
Copy link
Owner

inducer commented Dec 18, 2024

I'm OK with going to constantdict for everything, until it emerges that its incorrect complexity for modification (vs. the HAMT implementations) becomes a real problem.

@matthiasdiener matthiasdiener mentioned this pull request Jan 28, 2025
Open
@alexfikl
Copy link
Contributor

Just a thought on my side: constantdict seems to be a small one-file sort of library.. any chance we can put it in pytools instead? 😁

@inducer
Copy link
Owner

inducer commented Jan 29, 2025 •
edited
Loading

Just a thought on my side: constantdict seems to be a small one-file sort of library.. any chance we can put it in pytools instead? 😁

I actually like it where it is. While it seems like it is leading the pack of its competitors, it's not without issues. And IMO anyway, pytools already has too much stuff in it for its own good, it should get smaller, not bigger.

@inducer inducer mentioned this pull request Jan 29, 2025
Merged
matthiasdiener
matthiasdiener commented Jan 29, 2025
View reviewed changes
loopy/kernel/function_interface.py Outdated
Comment on lines 351 to 352
if arg_id_to_dtype is None:
arg_id_to_dtype = {}
Copy link
Collaborator Author

@matthiasdiener matthiasdiener Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure whether this is the intended behavior here (or whether arg_id_to_dtype should just be set to None). This bug seems to have come in with 70cc100#diff-c2536267212ab595d8fbafdc40257bf749f171cc1e23c6c5113fcf2e41546b02 (seems like typing with immutabledict is not strict enough so that mypy did not catch this earlier)

Same below.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this can just be assert arg_id_to_dtype is not None (since None is hashable, we should not end up in this branch).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed in ba93a2f

@matthiasdiener matthiasdiener changed the title replace pyrsistent.PMap, immutables.Map with immutabledict replace pyrsistent.PMap, immutables.Map, immutabledict with constantdict Jan 29, 2025
@matthiasdiener
Copy link
Collaborator Author

This is ready for a first look @inducer.

inducer
inducer approved these changes Jan 29, 2025
View reviewed changes
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Two minor things, and then I think this is ready! 🎉

loopy/kernel/function_interface.py Outdated
Comment on lines 351 to 352
if arg_id_to_dtype is None:
arg_id_to_dtype = {}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this can just be assert arg_id_to_dtype is not None (since None is hashable, we should not end up in this branch).

@matthiasdiener matthiasdiener marked this pull request as ready for review January 30, 2025 03:39
@inducer inducer merged commit a3c7eea into main Jan 30, 2025
18 checks passed
@inducer inducer deleted the constantdict2 branch January 30, 2025 15:03
@inducer
Copy link
Owner

inducer commented Jan 30, 2025

Thx!

inducer added a commit that referenced this pull request Jan 30, 2025
@inducer
Bump data model version for constantdict
e3b96cc 
This should have been part of
#884 / 87d6ccf
but was missed.

Without this, data from dictionary types that are not installed can
linger in caches and fail to unpickle when unpickling is attempted.
@inducer
Copy link
Owner

inducer commented Jan 30, 2025

Whoops. This should have included a data model bump: e3b96cc (see explanation in commit message).

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

@inducer inducer Awaiting requested review from inducer

Assignees

@matthiasdiener matthiasdiener

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthiasdiener @alexfikl @inducer