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

persistent_dict: make siphash24 optional #250

Merged
merged 1 commit into from
Jul 23, 2024
Merged

persistent_dict: make siphash24 optional #250

merged 1 commit into from
Jul 23, 2024

Conversation

ahesford
Copy link
Contributor

From #242:

I don't know how much of a bottleneck the hash computation is in mirgecom compiles, but at the same time, I can't think of a good reason to not do this.

I maintain pytools and pyopencl on Void Linux and, as long as the hash interface is equivalent, making siphash24 optional will slightly reduce the maintenance burden of carrying these packages alongside the rest of the numerical stack.

@inducer
Copy link
Owner

inducer commented Jul 23, 2024

Thanks for the contribution. I can see where you're coming from. At the same time, I'm a bit torn. Suppose a user fills a PersistentDict, then (say as a dependency of another package) installs siphash24. Suddenly the content of their PersistentDict became inaccessible. OTOH, they're used as caches most of the time, so who cares? 🤷 I don't know.

pytools/persistent_dict.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner

inducer commented Jul 23, 2024

Could you look into the CI failures?

@ahesford
Copy link
Contributor Author

CI failures were a mindless typo. I don't have a strong defense for this change, so if you think the value of enforcing consistency even as the package graph changes, I'll most likely just package siphash24 for Void and be done with it.

@inducer
Copy link
Owner

inducer commented Jul 23, 2024

I'd be willing to give this a shot. But be warned: If it blows up on me in some way, I'll probably just revert it. Also, Ruff had some more complaints. Could you take a look?

@ahesford
Copy link
Contributor Author

That seems reasonable. The latest patch should pass all checks.

@inducer inducer enabled auto-merge (rebase) July 23, 2024 18:55
@inducer inducer merged commit 95fef99 into inducer:main Jul 23, 2024
17 checks passed
@inducer
Copy link
Owner

inducer commented Jul 23, 2024

@ahesford
Copy link
Contributor Author

Thanks!

@ahesford ahesford deleted the thats_what_makes_a_combo branch July 23, 2024 19:04
@matthiasdiener
Copy link
Contributor

matthiasdiener commented Jul 23, 2024

I don't know about this- now users of PersistentDict won't get the benefits of siphash24 unless they install siphash24 manually (but instead get a warning that does not clearly describe how to resolve the issue)...

This affects downstream packages too (see e.g. https://github.com/matthiasdiener/skvlite/actions/runs/10066243771/job/27827181383?pr=13, which tests the size of a PersistentDict-based sqlite file, where the size now depends on whether siphash24 is installed or not).

inducer added a commit that referenced this pull request Jul 23, 2024
@inducer
Copy link
Owner

inducer commented Jul 23, 2024

To be fair, the size of that SQLite file could be considered an implementation detail. 523ff36 rewords the warning so as to be more actionable.

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