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

Sisyphus fails to hash numpy bools #165

Open
NeoLegends opened this issue Jan 2, 2024 · 1 comment
Open

Sisyphus fails to hash numpy bools #165

NeoLegends opened this issue Jan 2, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@NeoLegends
Copy link
Contributor

image

I believe this check fails because it's not using isinstance but checking for the type itself, which leads the code to try to invoke get_object_state, which then fails:

elif type(obj) in (int, float, bool, str, complex):
byte_list.append(repr(obj).encode())

The numpy bool behaves very similarly to normal python bools and therefore I would consider this behavior very surprising and I believe it should be fixed. Maybe checking for isinstance(obj, bool) as the only check using isistance is valid? I think this would be a possible fix here. Or we just add all numpy types to the list that's checked there.

@NeoLegends NeoLegends added the bug Something isn't working label Jan 2, 2024
@critias
Copy link
Contributor

critias commented Jan 2, 2024

type is used at this point to explicitly not match subclasses since there is no control what there __repr__ function returns. e.g.:

class Foo(str):
     def __str__(self):
         return super().__str__()*2

would return the same value if repr is called but str(Foo('bar')) != str('bar'). Maybe this is more of an academic problem and to strict. Should we just accept that if a object is an instance of one these types and returns the same value it's ok to return the same hash? Any opinions?

Adding all numpy types is a bad idea since it would introduce a dependency to numpy slowing down the sisyphus startup process even if numpy is not even part of the recipes.

We could also make this configurable in the settings file and change the check to something like this (and use a better variable name):

elif type(obj) in (int, float, bool, str, complex) or type(obj) in gs.OTHER_ACCEPTED_HASH_TYPES: 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants