-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Cython does not respect dependency for generating .pyx.c
file
#8961
Comments
So the problem is, a py3.extension_module() with cython inputs requires the |
Yes exactly. |
A few people have run into this now with the SciPy build, and it also looks like it's worse on Windows, maybe because we have to copy files there with a Python script which is slower than invoking
|
I think this is actually a problem for vala as well. My understanding is that for languages like vala and cython that transpile to another language which is then compiled, is that some dependencies need to be provided for the transpile step, and some for the compile step. Currently they're provided to the compile step. This likely needs to change to the Meson DSL, in that BuildTargets need to have something like |
I agree with that analysis... a test case would involve making a minimal example that exercises the need for transpile dependencies, and then having the test case attempt to build The alternatives to
|
The generic buildorder function would not really be just an alternative. It could be complementary. More efficient specific options for transpiling are nice. But the generic feature is just the way to not be blocked when you discover new use cases (blocked e.g. if we have to wait for meson to add specific options). Just saying. 😉 |
Just to check I understand - the underlying problem here is that the |
yes indeed, not
Yes, the potential solutions are the last comments above of @dcbaker and @eli-schwartz. |
Sorry - yes - I saw the earlier suggestions. What I was floating was a slight variant of @dcbaker's suggestions, which was to add a In any case - what's the best way forward from here? |
Possible solution: |
While digging how cython works in Meson I also noticed this issue. I think the fix is actually pretty easy, for any other source file (e.g. C++ files), all unknown generated sources is considered a "header" and added into order_deps:
The issue is simply that for cython sources, that code path is not reached. I'm currently working on exactly that. But while working on it, I also noticed that cython does not generate a depfile when compiling, there is no CLI option to generate one? |
The lack of a CLI option is in fact exactly why we had to implement cython support without using such an option. It's a known issue which I am tracking. |
Re depfile support in Cython: this was added to |
We are hitting this in pandas as well, and building the .o file individually works correctly, while building all targets doesn't work, since the .pyx.c target might get built before the .o target. Maybe a solution could be to mark the .pyx.c file generation as not built by default? It will get built anyways since the .o target will depend on it, but only attempt to build it once the order-only deps(.pxi) are satisfied. |
@xclaesse @eli-schwartz Any thoughts? I'm going to look into this soon. |
I don't think that will work... the .pxi, creation rule, the .c creation rule, and the .o compiler rule all end up in the build graph either way, queued for building, and we need a relationship rule for .pxi -> .c, the same way we have a .c -> .o relationship rule. We can do that with the generated headers approach (add .pxi output files as additional sources), we just need a way to "push" that from the "before .o" to "before .c" stage. |
We now handle this with pxi header-style order dependencies. Thanks @lithomas1! I think this is solved now. |
Describe the bug
Building a Python extension from a generated Cython file (
_ufuncs.pyx
) fails with:The relevant
meson.build
content is:What seems to be happening is that a ninja order dependency goes missing. These are the build rules generated:
If I inspect the files in the build dir, the
.pxi
files are indeed missing:If I understand what Ninja is doing correctly, this line:
is missing the
|| scipy/__init__.py .... scipy/special/_ufuncs_extra_code_common.pxi ...
order dependency that the next line does have.To Reproduce
I can write a standalone test case, but given that it's a bit of work and I'm not 100% sure what the best way is to write a test for such a timing sensitive order dependency issue, I wanted to describe my actual failure first. Should the test case use
-j 1
, somehow inspect the generated ninja build rules, or something else?In the mean time, this is reproducible from https://github.com/rgommers/scipy/tree/meson-issue-ufuncs-pyx with:
Expected behavior
Build should respect order dependency, and hence Cython should not fail on a missing
.pxi
file that is a dependency.system parameters
meson --version
: 0.58.999ninja --version
: 1.10.2@dcbaker I know gh-8775 is supposed to address this in the long term, but I believe there will still be cases where we will need
custom_target
in combination with compiling Cython code - we have a lot of different cases where we codegen.pyx
or.pxd
files.EDIT: we also need it for adding--cplus
for example.The text was updated successfully, but these errors were encountered: