inducer / islpy

Python wrapper for isl, an integer set library
http://pypi.python.org/pypi/islpy
73 stars 19 forks source link

Stop using pybind's implicit `__hash__` #103

Open inducer opened 1 year ago

inducer commented 1 year ago

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

kaushikcfd commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

So... it turns out that this is what has kept https://github.com/inducer/loopy/issues/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 commented 1 year ago

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 https://github.com/inducer/loopy/issues/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 commented 1 year ago

@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.