Rewrite logic in vlen to drop NumPy build dependency
#555
Conversation
Add workaround for lack of `const object[:]` in Cython by using a trick SciPy figured out using `fused` types. This in turn lets us relax the NumPy build dependency.
As `vlen` now solves the mutability issue without a NumPy dependency, drop the NumPy dependency to simplify building and deploying Numcodecs.
| # Hacky workaround for lack of `const object[:]`. | ||
| # Appears hiding `object` in a fused type works. | ||
| # xref: https://github.com/cython/cython/issues/2485 | ||
| # xref: https://github.com/scipy/scipy/pull/18192#pullrequestreview-1359581611 | ||
| ctypedef fused object_fused_t: | ||
| object | ||
| uintptr_t |
There was a problem hiding this comment.
This is the crux of the workaround
For fusing, used a uintptr_t. As object is effectively PyObject*, fusing with a generic pointer/address type seemed most appropriate
Note this differs from the SciPy approach where they used double. Though it shouldn't matter much what the other type is as it is unused
Whenever Cython fixes this issue, we can drop this and just use const object[:] where needed
There was a problem hiding this comment.
Well as evidenced by CI, this is not enough. Think what we need to do is get the fused types inside some kind of function signature. Then it might work
However looking at the code here. There are multiple places this would need to go
Wonder if the better path would be...
- Move most of this logic into pure Python (Cython is only needed for a small portion of this code)
- Refactor the code to reuse common bits across cases
- Have a small Cython function that deals with the bits that need to be lower-level
|
As per comments on conda-forge/numcodecs-feedstock#108, I don't think we need this any more |
As noted in issue ( https://github.com/zarr-developers/numcodecs ),
vlenhad problems with read-only dataInternally this occurred because we assigned the input array to
object[:], which expects mutability. Ideally we would just replace this withconst object[:]. However Cython does not support this ( cython/cython#2485 ). Over time various blockers have been resolved on that front, but it is not yet fixedFortunately an astute SciPy developer noted that
const <fused_type>[:]is allowed ( scipy/scipy#18192 (review) ). We use that same trick hereThis in turn should allow us to continue to accept read-only arrays. Though this does so without a NumPy build dependency, which can introduce more complexity to the build process
TODO: