Skip to content

Add types to elementwise.py #850

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

Merged
merged 8 commits into from
Jul 9, 2025
Merged

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Jul 9, 2025

This adds types pretty much all things in elementwise.py. There are some remaining errors that I'm not sure how to best handle:

  1. context_dependent_memoize expects something hashable as a first argument, mostly due to this:
    return cache_dict[cl_object][args]

However, the cl.Context that the functions in elementwise use is very much not hashable (?).

  1. Added a debatable ScalarLike alias for some functions in array.py.
  2. Deprecated range and slice in ElementwiseKernel and used named that don't shadow global functions instead. Went for this so that their types are not hidden in the kwargs.

@alexfikl alexfikl force-pushed the type-elementwise branch from 0707a8c to ab7ae49 Compare July 9, 2025 07:56
@alexfikl alexfikl marked this pull request as ready for review July 9, 2025 08:36
@inducer
Copy link
Owner

inducer commented Jul 9, 2025

However, the cl.Context that the functions in elementwise use is very much not hashable (?).

🤔 What makes you say that? Context is supposed to be hashable.

pyopencl/pyopencl/_cl.pyi

Lines 1004 to 1005 in 2acc9f7

@override
def __hash__(self) -> int: ...

@alexfikl
Copy link
Contributor Author

alexfikl commented Jul 9, 2025

Hm, mostly an unwavering trust in pyright:

pyopencl/pyopencl/elementwise.py:483:2 - error: Argument of type "(context: Context, dtype: dtype[Any], idx_dtype: dtype[Any], vec_count: int = 1) -> Kernel" cannot be assigned to parameter "func" of type "(Hashable, **P@first_arg_dependent_memoize) -> RetT@first_arg_dependent_memoize" in function "first_arg_dependent_memoize"
Type "(context: Context, dtype: dtype[Any], idx_dtype: dtype[Any], vec_count: int = 1) -> Kernel" is not assignable to type "(Hashable, **P@first_arg_dependent_memoize)-> RetT@first_arg_dependent_memoize"
Parameter 1: type "Hashable" is incompatible with type "Context"
"Hashable" is not assignable to "Context" (reportArgumentType)

Not quite sure what it doesn't like then, since it should see the __hash__ override. Any ideas?

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx! Only two minor nits.

@inducer
Copy link
Owner

inducer commented Jul 9, 2025

Not quite sure what it doesn't like then, since it should see the __hash__ override. Any ideas?

Note that this goes the other way! "Hashable" is not assignable to "Context", not "Context" is not assignable to "Hashable"

@alexfikl alexfikl force-pushed the type-elementwise branch from ab7ae49 to f7c969d Compare July 9, 2025 17:27
@alexfikl
Copy link
Contributor Author

alexfikl commented Jul 9, 2025

Note that this goes the other way! "Hashable" is not assignable to "Context", not "Context" is not assignable to "Hashable"

🤦 It makes a lot more sense when you actually read it, yeah :( Should be fixed now!

@alexfikl alexfikl force-pushed the type-elementwise branch from f7c969d to bcd30d5 Compare July 9, 2025 17:39
@alexfikl alexfikl force-pushed the type-elementwise branch from bcd30d5 to 2b799d7 Compare July 9, 2025 18:30
@inducer inducer merged commit 6a6d0ea into inducer:main Jul 9, 2025
18 checks passed
@inducer
Copy link
Owner

inducer commented Jul 9, 2025

Thx!

@alexfikl alexfikl deleted the type-elementwise branch July 9, 2025 19:02
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.

2 participants