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

More thoroughly type *Handle classes #501

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

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Oct 16, 2024

  • Type *Handle classes as much as possible
  • Add .pxd files for *Handle objects
  • Convert functions to cpdef where appropriate
  • Convert sizes to Py_ssize_t and use -1 or 0 for default values
  • Simplify and optimize parse_buffer_argument
  • Use asarray on the Python side to ensure we have Arrays in Cython to speedup access

@jakirkham jakirkham added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 16, 2024
@jakirkham jakirkham force-pushed the typ_rem_hdl branch 27 times, most recently from 0e78416 to 0643403 Compare October 17, 2024 02:29
@jakirkham jakirkham force-pushed the typ_rem_hdl branch 5 times, most recently from c4bbe93 to 6e1d3b7 Compare October 17, 2024 05:14
@jakirkham jakirkham changed the title More thoroughly type RemoteHandle More thoroughly type *Handle classes Oct 17, 2024
@jakirkham jakirkham marked this pull request as ready for review October 17, 2024 06:11
@jakirkham jakirkham requested a review from a team as a code owner October 17, 2024 06:11
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

I am not sure it is a good idea to expose the Cython API. Currently, all of _lib is private and is exposed and documented in kvikio explicitly.

If we go down this route, we should replace all of kvikio with _lib, and add .pyi files and docstrings.

I guess it depends on whether we expect anyone to use the Cython API? Currently, people either use the Python API or the C++ API.

@jakirkham
Copy link
Member Author

There is arr.pxd in _lib. That said, point taken on the rest of them

At a first pass was thinking about internal usage and simply making that experience better

If we don't ship the .pxd, then it is not actually public for consumers of these packages. CuPy follows this practice currently ( cupy/cupy#130 )

It is possible this could be useful externally. For example could imagine a performant Zarr implementation in Cython might use this

@madsbk
Copy link
Member

madsbk commented Oct 22, 2024

There is arr.pxd in _lib. That said, point taken on the rest of them

True, we have cdef functions that takes Array so we need it in a .pxd.

At a first pass was thinking about internal usage and simply making that experience better
If we don't ship the .pxd, then it is not actually public for consumers of these packages. CuPy follows this practice currently ( cupy/cupy#130 )

Right, but I really like to have the external and class declarations close together. If we only use something in one place, it is nice to have it together in a single file IMO.

It is possible this could be useful externally. For example could imagine a performant Zarr implementation in Cython might use this

I suspect that the overhead of calling a Python vs. a Cython function is negligible at this level but it might avoid a GIL grab. I think we should wait until this becomes an issue.

@jakirkham
Copy link
Member Author

Ok so is your main recommendation to fold the .pxd files back into .pyx for now?

Are there other things I should consider when doing a second pass?

@madsbk
Copy link
Member

madsbk commented Oct 22, 2024

Ok so is your main recommendation to fold the .pxd files back into .pyx for now?

Yes

Are there other things I should consider when doing a second pass?

I also prefer Optional[int] over a magical number like -1

@jakirkham
Copy link
Member Author

Ok can look at that

The Optional[int] bit is a little odd though as we do assign default values (0) for file_offset and dev_offset, which come after. Could we make these consistent somehow? On the Cython side it would be easier if these all had default values that were ints

@madsbk
Copy link
Member

madsbk commented Oct 22, 2024

IMO, having 0 as a default offset argument is good because 0 is a valid argument whereas -1 is not a valid size.
Also note, none of the Cython functions has any defaults. It is the pure Python functions that introduces defaults like here so that we don't have to keep multiple defaults in sync.

I get why it is faster with int arguments but I am not sure it is worth the added complexity?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants