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.

Sign up for GitHub

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

Jump to bottom

Cython depfile support #11369

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Cython depfile support #11369

merged 3 commits into from
Feb 10, 2023

Conversation

eli-schwartz
Copy link
Member

Cython 0.29.33 is now out, and supports emitting depfiles for incremental rebuild detection. When support is detected, emit this in the compiler rules.

@rgommers @lithomas1 @dnicolodi

Fixes #9049

@codecov
Copy link

codecov bot commented Feb 9, 2023 •
edited
Loading

Codecov Report

Merging #11369 (5cf8fa4) into master (a846fa3) will decrease coverage by 0.80%.
The diff coverage is n/a.

❗ Current head 5cf8fa4 differs from pull request most recent head 94a190b. Consider uploading reports for the commit 94a190b to get more accurate results

@@            Coverage Diff             @@
##           master   #11369      +/-   ##
==========================================
- Coverage   67.04%   66.25%   -0.80%     
==========================================
  Files         414      207     -207     
  Lines       90542    45276   -45266     
  Branches    20096     9369   -10727     
==========================================
- Hits        60703    29996   -30707     
+ Misses      25177    12888   -12289     
+ Partials     4662     2392    -2270
Impacted Files Coverage Δ
modules/qt6.py 0.00% <0.00%> (-100.00%) ⬇️
scripts/clangtidy.py 0.00% <0.00%> (-93.75%) ⬇️
modules/cuda.py 0.00% <0.00%> (-72.65%) ⬇️
templates/cudatemplates.py 39.39% <0.00%> (-60.61%) ⬇️
compilers/cuda.py 19.94% <0.00%> (-44.95%) ⬇️
dependencies/cuda.py 19.23% <0.00%> (-42.79%) ⬇️
modules/icestorm.py 57.14% <0.00%> (-40.00%) ⬇️
compilers/d.py 38.33% <0.00%> (-20.89%) ⬇️
dependencies/coarrays.py 46.34% <0.00%> (-17.08%) ⬇️
cmake/traceparser.py 71.16% <0.00%> (-9.08%) ⬇️
... and 262 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dnicolodi
dnicolodi reviewed Feb 9, 2023
View reviewed changes
mesonbuild/backend/ninjabackend.py Outdated
if depargs:
depfile = '$out.dep'
else:
depfile = None
Copy link
Member

Choose a reason for hiding this comment

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

I would write that as

depfile = '$out.dep' if depargs else None

as I find it more readable this way.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, and LGTM otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@eli-schwartz eli-schwartz added this to the 1.0.1 milestone Feb 9, 2023
@rgommers
Copy link
Contributor

rgommers commented Feb 9, 2023

Thanks @eli-schwartz!

This PR looks fine, but I just ran into a potentially annoying issue when testing it. Inspecting a depfile shows that the contents contain ..../site-packages/numpy/ instead of the local source tree:

$ cat build/numpy/random/_bounded_integers.cpython-39-darwin.so.p/numpy/random/_bounded_integers.pyx.c.dep

numpy/random/_bounded_integers.cpython-39-darwin.so.p/numpy/random/_bounded_integers.pyx.c: \
  numpy/random/_bounded_integers.pxd \
  /Users/rgommers/mambaforge/envs/numpy-dev/lib/python3.9/site-packages/numpy/random/__init__.pxd \
  /Users/rgommers/mambaforge/envs/numpy-dev/lib/python3.9/site-packages/numpy/random/bit_generator.pxd \
  /Users/rgommers/mambaforge/envs/numpy-dev/lib/python3.9/site-packages/Cython/Includes/libc/stdio.pxd \
  /Users/rgommers/mambaforge/envs/numpy-dev/lib/python3.9/site-packages/Cython/Includes/cpython/type.pxd \
  numpy/random/_bounded_integers.pyx \
  /Users/rgommers/mambaforge/envs/numpy-dev/lib/python3.9/site-packages/numpy/__init__.pxd \

If I uninstall numpy from the conda env (it was there because numpy doc build depends on scipy and matplotlib, which depend on numpy), then the paths disappear completely. The root cause seems to be absolute paths like from numpy.random cimport xxx, which I think works in the setup.py builds because of this bit from the docs:

Also, whenever you compile a file modulename.pyx/modulename.py, the corresponding definition file modulename.pxd is first searched for along the include path (but not sys.path), and if found, it is processed before processing the .pyx file.

I think I'm missing the needed --include-dir @BUILD_ROOT@ in the NumPy build. I'll have to fiddle with that later - that may take me a while, because I'm on holiday, so I'd say just assume that this PR is good to go for NumPy.

For SciPy nothing with this PR, because there we're not using cython as a compiler (but rather as a command line tool), and hence there's no depfile = $out.dep rule in build.ninja.

@eli-schwartz
backends: handle cython ninja rules a bit more idiomatically
6ea24ee 
We want to use as much default ninja behavior as we can, so reuse $out
instead of repeating the output file as a string in $ARGS, and raise
that into the build rule so it is only listed once.
@eli-schwartz
cython: wire up support for emitting and using depfiles
84dd78e 
This solves rebuild issues when e.g. importing a .pxd header from a .pyx
file, just like C/C++ source headers. The transpiler needs to run again
in this case.

This functionality is present in the 3.0.0 alphas of cython, and is also
backported to 0.29.33.

Fixes mesonbuild#9049
@eli-schwartz
cython: add unittest to verify that depfiles work
94a190b
@eli-schwartz
Copy link
Member Author

I think I'm missing the needed --include-dir @BUILD_ROOT@ in the NumPy build. I'll have to fiddle with that later - that may take me a while, because I'm on holiday, so I'd say just assume that this PR is good to go for NumPy.

Indeed, sounds like Meson is successfully getting depfiles to work so all the rest is going to be passing the correct cython args to make things work.

... but does that mean that examining the depfile caused a numpy bug to be noticed? Was your build genuinely loading the wrong numpy pxds?

@dnicolodi
Copy link
Member

I think it is a bit of a surprising behavior of Cython due to the fact that it does not distinguish between local includes and system includes. Ultimately it seems a bug in the NumPy build definition as I would have expected the compilation to fail in an isolated environment where the NumPy .pxds are not available in the system path, but I haven't verified this.

@eli-schwartz eli-schwartz merged commit 94a190b into mesonbuild:master Feb 10, 2023
@eli-schwartz eli-schwartz deleted the cython-depfile branch February 10, 2023 19:31
@rgommers
Copy link
Contributor

... but does that mean that examining the depfile caused a numpy bug to be noticed? Was your build genuinely loading the wrong numpy pxds?

I think so, yes - but need to verify. If not, then it's a bug in the Cython depfile generation - but that seems less likely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnicolodi dnicolodi dnicolodi left review comments

@nirbheek nirbheek nirbheek left review comments

@dcbaker dcbaker Awaiting requested review from dcbaker dcbaker is a code owner

@jpakkane jpakkane Awaiting requested review from jpakkane jpakkane is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.1
Development

Successfully merging this pull request may close these issues.

Cython dependency handling in .pxd and .pxi files
4 participants
@eli-schwartz @rgommers @dnicolodi @nirbheek