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

Fix usage of Cython API and Cython tests #396

Open
ajfriend opened this issue Sep 4, 2024 · 3 comments
Open

Fix usage of Cython API and Cython tests #396

ajfriend opened this issue Sep 4, 2024 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@ajfriend
Copy link
Contributor

ajfriend commented Sep 4, 2024

We'll break the Cython API with #378, but I think we should still merge it and fix it from there.

@ajfriend ajfriend added the help wanted Extra attention is needed label Sep 4, 2024
@ajfriend
Copy link
Contributor Author

Actually, it seems like https://github.com/ajfriend/h3_example_package still works with v4.1.0.b3! Thanks to @jongbinjung for checking!

However, we still fail the Cython test here: https://github.com/uber/h3-py/tree/master/tests/test_cython (As indicated by our current 0.56% gap in coverage.)
First appeared here: #378 (comment)

I'd still like to fix this issue so that we have a working example of Cythonizing a small script (as opposed to a whole package, as in h3_example_package).

@jongbinjung
Copy link
Collaborator

Took an unsuccessful stab:

TL;DR

Issue seems to be with cythonize not knowing where to put the *.so file, and having src/ as a PREFIX. So, when building tests/test_cython/cython_example.pyx, it tries to create src/tests/test_cython/cython_example.so instead of tests/test_cython/cython_example.so.

I can't for the life of me figure out where it's getting that src prefix ......

Replicating the issue

Run

make test

and see that coverage for tests/test_cython/test_cython.py is missing lines 18-28, indicating an early exit due to failing to import the cython_example module; i.e., the test coverage report output includes

...
tests/test_cython/test_cython.py                                     18      8    56%   18-28
...

Stuff I tried

  1. Looks like the step to build cython_example.pyx in the makefile doesn't actually build the .so file, so added -i flag to build "inplace", i.e., change that line to
    ./env/bin/cythonize -i tests/test_cython/cython_example.pyx

  2. This results in error:

    ./env/bin/cythonize -i tests/test_cython/cython_example.pyx
    error: could not create 'src/tests/test_cython/cython_example.cpython-311-darwin.so': No such file or directory
    
  3. Note, it's trying to create the .so file in src/tests/ instead of just tests—and the error seems to be more about the directory not existing.

  4. To make sure that's actually what's happening—and I'm not crazy—manually created that directory with

    mkdir -p src/tests/test_cpython
    
  5. Now things compile, but coverage still bad:

    $ make test
    ./env/bin/pip install cython
    Requirement already satisfied: cython in ./env/lib/python3.11/site-packages (3.0.11)
    ./env/bin/cythonize -i tests/test_cython/cython_example.pyx
    ./env/bin/pytest
    ...
    tests/test_cython/test_cython.py                                     18      8    56%   18-28
    ...
    
  6. This is expected, because the .so file is in src/tests/... instead of tests/.... The only reasonable thing to do, then is

    $ mv -v src/tests/test_cython/*.so tests/test_cython
    src/tests/test_cython/cython_example.cpython-311-darwin.so -> tests/test_cython/cython_example.cpython-311-darwin.so
    
  7. BEHOLD:

    $ make test
    ./env/bin/pip install cython
    Requirement already satisfied: cython in ./env/lib/python3.11/site-packages (3.0.11)
    ./env/bin/cythonize -i tests/test_cython/cython_example.pyx
    ./env/bin/pytest
    ...
    tests/test_cython/test_cython.py                                     18      4    78%   7-8, 15-16
    ...
    

    (the lines that are not executed are now lines other than 18-28—the lines that should run if the cython build fails)

@ajfriend
Copy link
Contributor Author

That's progress! I wonder what the src/ issue has to do with #378, since this test worked fine before we moved to pyproject.toml and scikit-build-core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants