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

Stop using pybind's implicit __hash__ #103

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

inducer
Copy link
Owner

@inducer inducer commented Dec 30, 2022

No description provided.

@inducer
Copy link
Owner Author

inducer commented Dec 30, 2022

The SSL certificate on https://gmplib.org/ is broken, breaking all the wheel builds. It looks like they're aware of it.

@kaushikcfd
Copy link
Collaborator

Thanks! Looks like most of loopy's errors come from islpy.Space being unhashable. Maybe we can overload in islpy/__init__.py as:

def _isl_space_get_hash(x):
    return hash((x.dim()), x.params()))
Space.__hash__ = _isl_space_get_hash

It is unlikely that ISL will accept a corresponding native function as this defines new private API without any users.

@inducer inducer force-pushed the no-implicit-hash branch 2 times, most recently from c1cee84 to aba6644 Compare January 1, 2023 20:51
@inducer
Copy link
Owner Author

inducer commented Jan 1, 2023

@isuruf Was there a big reason why we used gmp instead of imath-32 in the wheel? I'm getting ready to drop gmp here because I'm tired of waiting for them to fix their certificate.

@inducer
Copy link
Owner Author

inducer commented Jan 2, 2023

So... it turns out that this is what has kept inducer/loopy#576 from biting as hard as it deserved to.

https://gist.github.com/inducer/9eb97cb5a6064baee63d0ef64233ee3b

together with this patch to loopy illustrates the issue:

diff --git a/loopy/kernel/tools.py b/loopy/kernel/tools.py
index 99f5f350..32e11eba 100644
--- a/loopy/kernel/tools.py
+++ b/loopy/kernel/tools.py
@@ -355,6 +355,7 @@ class SetOperationCacheManager:
 
         for bkt_set, result in bucket:
             if set_.plain_is_equal(bkt_set):
+                assert result == op(set_, *args)
                 return result
 
         result = op(set_, *args)

@kaushikcfd
Copy link
Collaborator

I was thinking I could make a quick PR to loopy which would also add the condition bkt_set.get_var_dict() == set_.get_var_dict(), but the problem is deeper as inducer/loopy#576 points :/. I guess decoupling loopy's ISL usage from relying on dim-names seems the only (practical) option. The other (dirty) option is to make loopy depend on a different islpy that overrides isl_xxx_is_equal.

@isuruf
Copy link
Contributor

isuruf commented Jan 2, 2023

@isuruf Was there a big reason why we used gmp instead of imath-32 in the wheel? I'm getting ready to drop gmp here because I'm tired of waiting for them to fix their certificate.

Nope. gmp was faster than imath (no small-integer optimization), but imath-32 (with small integer optimization) should be better.

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.

None yet

3 participants