[chrishavlin]

Update: this implements just the BoolArrayCollection c-api. Related to #216

This is a draft PR with a couple of small implementations of enforcing opaque classes via a new C api. It's much smaller than the full implementation would be, but I think hits the functional changes and clarifies how we'd have to update yt.

I've been relying on claude a bit to understand the problem better and what potential solutions there are, but words and implementation here are all my own.

origin of the build compatibility problems

as far as I understand it, the underlying issue is that when yt cimport's the ewah_bool classes, yt has to carry a copy of ewah_bool's internal object layout. So changes in Cython version, build settings, etc. on either side create a mismatch that can lead to import errors (yt-project/yt#5232).

the proposed fix: a wrapper api

The gist is that if we write a bunch of wrapper functions that accept an instantiated ewah_bool classes typed as a plain object that are cast to their appropriate ewah classes internally in the api, then yt would never have to cimport the cdef classes, reach into class attributes or touch C++ types.

e.g., in api.pyx, we have:

cdef object bool_array_collection_new():
    return BoolArrayCollection()
cdef bint bool_array_collection_loads(object coll, bytes s) except -1:
    return (<BoolArrayCollection>coll)._loads(s)

and the yt side would be updated to use the new api functions only. e.g.,

cimport ewah_bool_utils.api as ewah_api

cdef object bac = ewah_api.bool_array_collection_new()
cdef bytes s = ...
ewah_api.bool_array_collection_loads(bac, s)

instead of the current state:

from ewah_bool_utils.ewah_bool_wrap cimport BoolArrayCollection
cdef BoolArrayCollection bac = BoolArrayCollection()
cdef bytes s = ...
bac._loads(s)

One important limitation: these wrapper functions should not rely on numpy ctypes or buffer as it would re-introduce a point of incompatibility. So libc.stdint types have to be used along with typed memoryviews.

scope and transition

The minimum change is to add wrapper functions in api.pyx for all the functions used by yt's particle_oct_container.pyx. once they exist, we can release ewah_bool_utils 1.4.0 and then update yt to use the new functions in 4.5.0. ewah_bool_utils 2.0 could stop shipping ewah_bool_wrap.pxd. We'd need around 40 functions in api.pyx to cover just yt's uses -- if we want to expand to cover the whole ewah_bool_utils package (which we probaby should?), it's closer to 100 functions.

Note that the vast majority of api functions would be pretty simple one or two line forwarding functions, but there are a few sections in particle_oct_container.pyx that would require new functions on the ewah_bool_utils api side -- i.e., bool_array_collection_coarse_indices in this PR would get used in

https://github.com/yt-project/yt/blob/51c862ab474af6ed005a5d7cc6956539e308dd8f/yt/geometry/particle_oct_container.pyx#L370-L399

(and a couple other similar spots).

ai disclosure

been using claude for understanding how to do this... implementations are my own though. can discuss if this would qualify as a good use of AI to build out the new api in one go.

[neutrinoceros]

thanks for tackling this Chris
Seeing this is still a draft, is there anything I should be waiting for on your end before I can review ?

[chrishavlin]

oops! left that part out @neutrinoceros ... tagged you and @matthewturk at this stage cause I wanted to double check on the approach here. was thinking of it as a an early proposal PR of sorts? so an in depth review is not required yet, but if you see anything glaringly wrong at this stage definitely let me know! or if you want to wait til I undraft that's fine too.

[neutrinoceros]

the approach is definitely sound. I guess my one question at this point is where can I look for documentation about the object type in Cython (or is it c++ ?) ?

[chrishavlin]

it's Cython : https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#python-objects-as-parameters-and-return-values

The type name object can also be used to explicitly declare something as a Python object. This can be useful if the name being declared would otherwise be taken as the name of a type...

As I understand it, using a plain object avoids the need for Cython to build in the class (e.g., BoolArrayCollection) during compilation so it doesn't depend on the class layout and consumers like yt won't need to pull in the corresponding .pxd files at compile time (only the api.pxd gets baked in, and it's got only plain C types).

[chrishavlin]

note to self based on some more reading/digging: I should add some strict type checks internally to the api here since moving to an opaque function-based api effectively removes the type checking that would happen during compilation. so if you hand one of these functions the wrong type of object you could get some hard to diagnose errors below the python level (segfaults, other garbage). it's unlikely given that the api will have the methods for getting the proper objects (*_new()), but would be very annoying to debug.

[chrishavlin]

other notes based on experimenting now:

1. we can split out api's for each ewah class into namespaces. this will preserve some of the object-oriented design in the new functional layout. so we could do, e.g.:

cimport ewah_bool_utils.capi.bool_array_collection as bac_api

cdef object bac = bac_api.new_collection()
cdef bytes s = ...
bac_api.loads(bac, s)

2. this is a c-api, let's make that explicit (ewah_bool_utils.capi in the example above instead of ewah_bool_utils.api.

EDIT: implemented both of the above with the initial functions.

[chrishavlin]

ooook. so i wrote a few more of the BoolArrayCollection methods. and then worked through the rest more quickly with claude... at this point, the PR includes everything for BoolArrayCollection including two new functions to replace sections in yt that reach into the ewah_bool_utils class attributes.

[neutrinoceros]

it's reached a size I cannot review efficiently in iterative fashion, so I think we need to coordinate before moving forward. I suggest one of two approaches:

1. spin out a minimal version of this (restricted to apis actually used in yt) and focus review/iterations there at first
2. "freeze" this PR until I can review it entirely.

if you want to go with 2, just undraft the PR (and ping me, otherwise I don't think I'll be notified)

[chrishavlin]

yup! good call. i was thinking about taking what I have here now and creating a yt branch to utilize what's in the current c-api here. lemme do that and push up a draft branch to yt then I'll undraft here. I think it will be easier to review with that yt branch in place.

this pr as is doesn't cover all of yt's c level ewah calls, but it's enough to demonstrate clearly how to use the new c api.

and I think we can do this in stages here too: subsequent PRs for the ewah classes that still need to be covered in the c-api .

[chrishavlin]

oh and one modification that would help I think: im gonna rollback one of the changes here so that I'm only including the methods that yt needs in the c-api. I can add all the ewah class methods later for completeness, but I think for now just having those that yt uses would make sense to keep the prs a bit shorter.

[chrishavlin]

Ok, yt-project/yt#5601 is up! and i noticed more functions on this side that yt doesn't need, so i just removed them. I can pull them from my git history later if we want a complete class mapping to the c-api.

[chrishavlin]

in terms of review and proceeding to other classes: this PR (and subsequent ones) are fine to merge to main since they won't touch internals of the classes and only add new c-api namespaces. So we won't introduce abi incompatibilities if for some reason we need to cut a release before the full c-api is complete.

[neutrinoceros]

This should probably be in install_data instead. This may work as is but it's a bit confusing; a pyx file isn't a python source

[chrishavlin]

ok. looking back at my code i'm not sure why i put it in _sources. ya, _data makes more sense and matches what we're doing with the other extension modules. will update!

[chrishavlin]

moved this to an install_data. note, I also moved 'src/ewah_bool_utils/capi/__init__.py' to ` 'src/ewah_bool_utils/capi/init.pxd' since we have no python API there (and all the module resolution still works without the .py)

[neutrinoceros]

I'd prefer a more robust impl using symbolic expressions here. Maybe even some parametrization to avoid having a single case passing by accident.

[chrishavlin]

so i don't mind adjusting these tests some: but for the c-api functions that are purely forwarding functions I'm a bit worried about having to write full tests. IMO they should be testing just the forwarding behavior (does it compile, can we call the method) and not the underlying methods. I know that that there's not a lot of pre-existing tests on the underyling methods, and I do think it'd be good to improve coverage, but I worry about how much that will delay this PR.

[chrishavlin]

just added a new test function for the second new function introduced by this PR (test_coarse_indices below) and another simple test for the one forwarding function i had missed.

so, opinions on how much I need to beef up the tests of the straight forwarding functions (essentially adding tests for pre-existing functionality that is not currently covered by tests)?

[neutrinoceros]

Without knowning anything about the api under test, this test is practically meaningless to me as a reader. It may be fine, maybe I just need to understand the api, but I really like tests that can teach the reader about it when feasible

[chrishavlin]

will add some comments!

[chrishavlin]

(or make the tests more self descriptive)

[chrishavlin]

ooook, rewrote and beefed this one up. now got:

• a helper function that does what yt does when it wants to mark a sub-cell as refined so i'm not writing out bits manually
• parametrization of which cells to refine and how that corresponds to the bit-indices
• lots more comments and renamed variables for clarity

[neutrinoceros]

I don't get the intention behind these functions, they look like an additional wrapping layer on top of the one you're building and actively testing

[chrishavlin]

just so they can be called from tests: without them from pytest we'd have to call the python api which would never hit the new c-api.

[chrishavlin]

note, i looked into using cython.inline or compiling _testing.pyx as part of the pytest config... and things got complicated. cython.inline is a bit out dated and we'd need to add a setuptools dependency for python 3.12+ to the test stuie, pytest config didn't seem any less complicated that what we're doing now.

[chrishavlin]

pre-commit.ci autofix