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

Ensured that dtype subclasses are hashable #5657

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

superbobry
Copy link

@superbobry superbobry commented Jan 20, 2025

Python data model requires a class to implement both __eq__ and
__hash__ to be considered hashable. If a class only implements
__eq__, it gets an auto-generated __hash__ = None, which makes
it non-hashable.

So, as things stand, tl.pointer_type and tl.block_type instances fail
to hash. The fix is to define __hash__ explicitly in the class body, as
suggested in *:

If a class that overrides __eq__ needs to retain the implementation of
__hash__ from a parent class, the interpreter must be told this explicitly
by setting __hash__ = <ParentClass>.__hash__.


New contributor declaration

  • I am not making a trivial change, such as fixing a typo in a comment.

  • I have written a PR description following these
    rules.

  • I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • I have added tests.
      • /test for lit tests
      • /unittest for C++ tests
      • /python/test for end-to-end tests
    • This PR does not need a test because FILL THIS IN.
  • Select one of the following.

    • I have not added any lit tests.
    • The lit tests I have added follow these best practices,
      including the "tests should be minimal" section. (Usually running Python code
      and using the instructions it generates is not minimal.)

@superbobry superbobry requested a review from ptillet as a code owner January 20, 2025 22:37
Python data model requires a class to implement both `__eq__` **and**
`__hash__` to be considered hashable. If a class only implements
`__eq__`, it gets an auto-generated `__hash__ = None`, which makes
it non-hashable.

So, as things stand, `tl.pointer_type` and `tl.block_type` instances fail
to hash. The fix is to define `__hash__` explicitly in the class body, as
suggested in [*]:

> If a class that overrides `__eq__` needs to retain the implementation of
> `__hash__` from a parent class, the interpreter must be told this explicitly
> by setting `__hash__ = <ParentClass>.__hash__`.

[*]: https://docs.python.org/3/reference/datamodel.html#object.__hash__
@Moerafaat
Copy link
Contributor

@ThomasRaoux can you please take a look at this fix? Thanks!

@pawelszczerbuk
Copy link
Contributor

Wouldn't this map scalar type and tensor with elements of the same type (like float32 and tensor of float32) and also different shape tensors to be the same? I would think we should include shape in the hash.

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