add a depfile support to cythonize #4563

ev-br · 2022-01-09T20:22:34Z

Here's an initial WIP attempt at addressing #4510 (comment), gh-1214 and gh-1459; see also mesonbuild/meson#9049

This PR is deliberately simplistic : given a foo.pyx which cimport bar.pxd and includes baz.pxi,

$ cythonize -M foo.pyx

produces foo.c.dep file in with foo.pyx dependencies along with foo.c.

An immediate question to mainainers: is this a reasonable place to plug it? Is this simple interface OK?
I'd also appreciate a pointer to how to best add tests for this. I can of course write out several tempfiles, or is there a better way?

Also, @eli-schwartz what would meson need --- in the example above, does it need a list of dependencies for the foo.c file (thus foo.c: foo.pyx bar.pxd baz.pxi) or for the pyx file (thus foo.pyx: bar.pxd baz.pxi)?

eli-schwartz · 2022-01-09T20:36:00Z

First, a note on the correct place for this argument.

There are two tools:

cython, which converts a pyx to a C source
cythonize, which also tries to use setuptools to build it

It is possible to use cythonize without building, as they are both just providing an interface into similar (shared) functionality, but the UX of each CLI command is, I think, geared towards its primary use. ;)

Hence I believe both tools might benefit from the command line argument, and particularly, meson currently uses cython not cythonize.

eli-schwartz · 2022-01-09T20:38:21Z

does it need a list of dependencies for the foo.c file (thus foo.c: foo.pyx bar.pxd baz.pxi) or for the pyx file (thus foo.pyx: bar.pxd baz.pxi)?

The produced file is foo.c, and you want to add rules for what files force foo.c to be regenerated.

foo.pyx is checked into git, so you do not need dependency rules that say "bar.pxd baz.pxi got modified, so foo.pyx needs to be regenerated".

tl;dr

The rule to create is foo.c: foo.pyx bar.pxd baz.pxi

scoder · 2022-01-09T21:39:57Z

Thanks. Regarding tests, you can add a srctree file under tests/build/. Those are just a bunch of text files stuffed into one, that the test runner splits into a directory structure. Look at the other files there as an example.

eli-schwartz · 2022-01-09T21:51:21Z

Addendum to "the correct place for the argument": this was discussed a bit in mesonbuild/meson#8706 (comment) and @scoder's comment there is relevant.

That's reasonable. Seems like it doesn't matter which you choose then, and cython wins by the simpler interface.

scoder · 2022-01-09T21:54:34Z

* cython, which converts a pyx to a C source
* cythonize, which also tries to use setuptools to build it

I'd rather consider the main feature of cythonize that it accepts file path wildcards and checks the dependencies to rebuild only from modified sources, like "here are my files, here is what I want, do what's necessary". And yes, it also builds the final extension module(s) for you if that's what you want. It's more or less a CLI for the cythonize() function that people use in their setup.py files.

I agree that both tools should support dependency file generation. I guess that cythonize could also make use of depfiles itself, although in order to determine the target file extension (c/cpp), it may still have to look at the source file first (specifically a # distutilts: ... header line that defines the target language).

scoder · 2022-01-09T21:58:07Z

… That said, it's probably easier to make cythonize generate depfiles than cython, because cython does not care about dependencies at all. I'd be ok with making only cythonize generate the files, if adding this to cython proves harder.

(As I mentioned before.)

eli-schwartz · 2022-01-09T22:00:04Z

Ah yeah, I guess a better way of phrasing it would be "also implements a bit of a basic build system, powered under the hood by setuptools, which collects all the files for you, and optionally builds them while detecting out of date files etc. etc."

I agree that an MVP to have cythonize create depfiles is useful on its own (meson could always switch to that, if it needs to), just thought I'd make sure the cython tool didn't get overlooked without even meaning to. :D

So that $ cythonize -M foo.pyx produces `foo.c.dep` file in with foo.pyx dependencies (`foo.c` is produced as well).

ev-br · 2022-01-11T14:27:43Z

Regarding tests, you can add a srctree file under tests/build/

Thanks! I've updated the PR with a simple srctree test. It passes locally, but needs somebody with the commit bit to please approve the CI run.

Re: correct place for this. So far it's only implemented in cythonize. Duplicating the functionality in the cython executable looks easy if Cython/Compile is allowed to depend on Cython/Build : cython uses the former while cythonize uses the latter. I don't know if the difference is purely historical or there's some other reason these two should be kept separate.

Cython/Build/Dependencies.py

tests/build/depfile.srctree

Co-authored-by: scoder <stefan_ml@behnel.de>

Also rework the paths in depfiles: paths below the source are relative, others are absolute (e.g. system dependencies, numpy, cython itself etc)

ev-br · 2022-01-12T16:33:10Z

Thanks Stefan. The PR's updated with Stefan's edits and two more tests : with numpy headers and cythonizing a package (just a slightly modified cythonize_script_package.srctree).
Tests seem to pass locally, but CI sadly needs an approval again (GH is having best intentions, I'm sure; who knows maybe I'm trying to mine some crypto under the hood).

From my side, the PR is ready for review.

rgommers · 2022-01-12T16:34:31Z

CI sadly needs an approval again (GH is having best intentions, I'm sure; who knows maybe I'm trying to mine some crypto under the hood).

submit a trivial typo fix PR to sidestep this issue on your next pushes?

scoder · 2022-01-12T15:31:00Z

Cython/Build/Dependencies.py

@@ -957,6 +957,8 @@ def cythonize(module_list, exclude=None, nthreads=0, aliases=None, quiet=False,
    :param compiler_directives: Allow to set compiler directives in the ``setup.py`` like this:
                                ``compiler_directives={'embedsignature': True}``.
                                See :ref:`compiler-directives`.
+
+    :param depfile: produce depfiles for the sources if True.


It's funny that we list options here that are not actually arguments of the function. I'm not sure if that's really a good idea. I think we should stick to the explicit options that we allow and only state that we also accept arbitrary CompilationOptions.

This new option is not currently a part of CompilationOptions. It would probably be better if it was, but then we'd have to make sure that we support it wherever we accept CompilationOptions in the programmatic interface. And that's currently not the case. So …

Choices I see:

make this an explicit argument only for cythonize and leave it as a frontend feature (which could then optionally be copied to the cython command – not decided yet if we should do that)

move it into CompilationOptions and make it a general compiler feature (which would then also be available to the cython command) – requires a different implementation then, though.

Would you be open to considering the current implementation (of a scotch-tape-and-chewing-gum variety, yes) to unblock downstream and deferring a cleanup to a follow-up?

The interface is fine and won't change in the future. Let's keep it for now, stick with cythonize only, and see if users can adapt. We can still extend it or reimplement it later if they can't.

scoder · 2022-01-12T16:57:47Z

Cython/Build/Dependencies.py

+                    # paths below the base_dir are relative, otherwise absolute
+                    paths = []
+                    for fname in dependencies:
+                        if src_base_dir in fname:


This doesn't seem safe. Rather use prefix comparisons. I'm sure we already do this … somewhere. Probably something to have in Cython/Utils.py

scoder · 2022-01-12T17:05:18Z

Cython/Build/Dependencies.py

+                    # manually add missing *.pyx files for each *.pxd :
+                    # `cimport foo` means a possible dependence on a `foo.pyx`
+                    # but `all_dependencies(...)` above only list a `foo.pxd`


Changes in a .pyx file should not trigger a rebuild of files that only depend on the .pxd. That's intentional. If anything changes in the binary interface, that should be reflected by the .pxd (or fail when building the other module itself).

Not sure I follow. As a silly example (tests/build/depfile_package.srctree below) : pkg/sub/test.pyx does from ..test cimport get_str; the get_str function is declared in pkg/test.pxd and implemented in pkg/test.pyx. If the implementation changes (i.e. pkg/test.pyx), I'd naively expect a rebuild of the module which is built from pkg/sub/test.pyx, is not so?

If the implementation changes, but the interface doesn't, then nothing that uses the interface needs to be rebuilt.

Note: You may get different results at runtime, but that's not a build issue.

Ah, so a full build system notices that pkg/test.pyx changes and marks its targets for a rebuild. Thanks, am rolling back this part of the patch. I think it's nearly done now, the last thing is to figure out what you meant by the prefix comparisons. Will push after that.

tests/build/depfile_numpy.srctree

ev-br · 2022-01-12T21:12:36Z

OK, review comments addressed, test finally pass locally, let's see if CI agrees (needs approval, again).

Cython/Build/Dependencies.py

…ctory.

tests/build/depfile.srctree

tests/build/depfile_numpy.srctree

tests/build/depfile_package.srctree

tests/build/depfile.srctree

ev-br · 2022-01-14T12:28:34Z

What I'm not sure about though is 'np_prefix/init.cython-30.pxd'

Checked locally on python 2.7 + numpy 1.16 : it's __init__.pxd. Not sure about python 3.x with x<5

scoder · 2022-01-14T12:35:34Z

What I'm not sure about though is 'np_prefix/__init__.cython-30.pxd'

Checked locally on python 2.7 + numpy 1.16 : it's __init__.pxd. Not sure about python 3.x with x<5

Oh, it doesn't depend on the Python version, only on the Cython version (0.x or 3.x) and NumPy version (whether it ships the cython-30 file or not).

Thinking about that a bit more, it means that our test would potentially fail if NumPy decided to stop shipping that file or if it started providing an additional cython-31 etc. file. That's not a decision that should break tests on our side. We should rather filter out the concrete version suffix.

- Avoid hadcoding the version in np_prefix/__init__.cython-30.pxd - Recognize cy_prefix/Include/numpy/__init__.pxd fallbacks for legacy numpy versions.

ev-br · 2022-01-14T16:22:51Z

Trying it on python 2.7 was mostly to pick a definitely old numpy version (which needs a fallback Numpy/__init__.pxd). A desire to be able to debug things a bit faster than on the CI was a secondary factor :-).
Anyway, the last commit tries to both filter the version in cython-30 and recognize various locations for the numpy-related pxd file.

I've to say I'm amazed by how easy it is to switch Cython between python 2.7 and 3.8. Wow.

scoder · 2022-01-14T17:21:29Z

I'd be happy with just stripping out any .cython-[0-9]+ parts. We already test that lookup mechanism elsewhere and shouldn't depend on NumPy's exact choice here.

And yes, older NumPy versions that do not ship a numpy/__init__.pxd at all make Cython use the one it provides itself. Yeah, that's a bit annoying to validate. We can happily allow both interchangeably, as you pretty much did.

scoder · 2022-01-14T17:23:45Z

That being said, what about the header files? If there is a cdef extern from "someheader.h": ... in a source file (doesn't have to be a .pxd), shouldn't that show up in the list of dependencies as well? For NumPy, for example, that would be numpy/arrayobject.h. Your PR doesn't generate .dep files for .pxd files, does it? I think we should make sure that all #include files are added to the dependencies list of the .pyx file as well.

EDIT: No, hang on. Actually not. They are dependencies of the .c file, not of the .pyx file. We should not rerun Cython when a header file changes, but only when a .pxd file etc. changes. If a header file changes, we only need to trigger a run of the C compiler, not of Cython. So it's right to leave them out.

ev-br · 2022-01-14T17:32:22Z

just stripping out any .cython-[0-9]+ parts. We already test that lookup mechanism elsewhere

My search-foo failed to find it, mind giving me a pointer? Or is it OK the way it is at the moment?

scoder · 2022-01-14T17:33:30Z

just stripping out any .cython-[0-9]+ parts. We already test that lookup mechanism elsewhere

https://github.com/cython/cython/blob/master/tests/run/versioned_pxds.srctree

scoder · 2022-01-14T17:35:51Z

Or is it OK the way it is at the moment?

I think a re.sub() that removes any [.]cython-[0-9]+ would be better (safer, more future proof…).

ev-br · 2022-01-14T19:36:07Z

Thanks. Did not know about pxd versioning! The last commit uses re.sub, as requested.

scoder · 2022-01-14T21:14:12Z

Thanks. Let's see what users make of this.

ev-br · 2022-01-14T21:23:47Z

Thanks Stefan!

rgommers · 2022-01-15T10:52:03Z

Awesome, thanks all!

@scoder can I ask how you feel about backporting a PR like this? We're going to release SciPy 1.9.0 with Meson as the build system around June, and for that we could really use this feature and gh-4548. And it's unlikely we'll be able to start depending on a 3.0 alpha or beta release without creating significant issues for packagers.

eli-schwartz · 2022-01-16T01:35:52Z

Yes please. It would be great if we can include support for this in meson as soon as possible .

scoder · 2022-01-16T10:00:09Z

can I ask how you feel about backporting a PR like this?

It feels like a new feature, but I'd say that build system improvements are important enough to make an exception. And they don't hurt. Users who need them can just depend on the point version and others will (hopefully) not be impacted.

However, the patch doesn't apply cleanly since it is based off a couple of earlier changes in the master branch (which shouldn't be backported). Anyone up for a backport PR?

$ cythonize -M foo.pyx produces a file `foo.c.dep` with dependencies of foo.pyx, in addition to `foo.c`. Try to write relative paths as much as possible. Backport of cythongh-4563

ev-br · 2022-01-16T12:34:12Z

Anyone up for a backport PR?

#4576

Add a new command line option so that $ cythonize -M foo.pyx produces a file `foo.c.dep` with the dependencies of foo.pyx, in addition to `foo.c`. Try to write relative paths as much as possible. Backport of #4563

eli-schwartz · 2022-07-22T21:51:55Z

Update: after exploration, it turns out that the cythonize tool does not allow specifying the output files to use (makes sense if you think about it, it is more or less setuptools). So it doesn't seem feasible to use it directly.

In the meantime, I poked around at the frontends and hacked depfile support into the cython tool. I also fixed a bug in the Makefile generation.

eli-schwartz · 2022-07-22T21:58:53Z

See #4916

ev-br mentioned this pull request Jan 9, 2022

cython / c++ meson upstream tracking issue ev-br/mc_lib#36

Open

scoder added Build System enhancement labels Jan 9, 2022

Add a depfile support to cythonize

3d1121b

So that $ cythonize -M foo.pyx produces `foo.c.dep` file in with foo.pyx dependencies (`foo.c` is produced as well).

ev-br force-pushed the depfile branch from ad167c4 to 3d1121b Compare January 11, 2022 14:10

ev-br changed the title ~~WIP: add a depfile support to cythonize~~ add a depfile support to cythonize Jan 11, 2022

scoder reviewed Jan 12, 2022

View reviewed changes

Cython/Build/Dependencies.py Outdated Show resolved Hide resolved

Cython/Build/Dependencies.py Outdated Show resolved Hide resolved

tests/build/depfile.srctree Outdated Show resolved Hide resolved

ev-br and others added 3 commits January 12, 2022 16:02

Update Cython/Build/Dependencies.py

b6ec84d

Co-authored-by: scoder <stefan_ml@behnel.de>

Update Cython/Build/Dependencies.py

0ca25a4

Co-authored-by: scoder <stefan_ml@behnel.de>

add a depfile test for numpy cimports

441686f

Also rework the paths in depfiles: paths below the source are relative, others are absolute (e.g. system dependencies, numpy, cython itself etc)

ev-br force-pushed the depfile branch from 19210b8 to 2770eea Compare January 12, 2022 16:26

scoder reviewed Jan 12, 2022

View reviewed changes

test depfile on a package

bd2c192

ev-br force-pushed the depfile branch from 558a6ff to edd44a9 Compare January 12, 2022 21:07

address review comments

3602df8

ev-br force-pushed the depfile branch from edd44a9 to 3602df8 Compare January 12, 2022 21:08

scoder reviewed Jan 13, 2022

View reviewed changes

Cython/Build/Dependencies.py Outdated Show resolved Hide resolved

Prevent partial path prefixes from being interpreted as a parent dire…

e2e6f94

…ctory.

Normalise formatting to reduce future unrelated line changes.

72d82f7

scoder reviewed Jan 14, 2022

View reviewed changes

Make the test output helpful on failures.

ed7c132

depfile: tweak tests

fcb9a6d

- Avoid hadcoding the version in np_prefix/__init__.cython-30.pxd - Recognize cy_prefix/Include/numpy/__init__.pxd fallbacks for legacy numpy versions.

depfile: simplify testing

38ef24a

scoder added this to the 3.0 milestone Jan 14, 2022

scoder merged commit 9db1fc3 into cython:master Jan 14, 2022

scoder mentioned this pull request Jan 14, 2022

Add flag for Makefile dependency rules generation #1214

Closed

ev-br mentioned this pull request Jan 16, 2022

Backport gh-4563 : generate dependency files #4576

Merged

lesteve mentioned this pull request Jul 17, 2024

[BUG] Cython depfile should use relative paths #6291

Closed

lesteve mentioned this pull request Aug 31, 2024

Use relative path for depfile #6345

Merged

Uh oh!

add a depfile support to cythonize #4563

add a depfile support to cythonize #4563

Uh oh!

Conversation

ev-br commented Jan 9, 2022

Uh oh!

eli-schwartz commented Jan 9, 2022

Uh oh!

eli-schwartz commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder commented Jan 9, 2022

Uh oh!

eli-schwartz commented Jan 9, 2022

Uh oh!

scoder commented Jan 9, 2022

Uh oh!

scoder commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eli-schwartz commented Jan 9, 2022

Uh oh!

ev-br commented Jan 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ev-br commented Jan 12, 2022

Uh oh!

rgommers commented Jan 12, 2022

Uh oh!

scoder Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ev-br Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

scoder Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

scoder Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

scoder Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

ev-br Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

scoder Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ev-br Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ev-br commented Jan 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ev-br commented Jan 14, 2022

Uh oh!

scoder commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ev-br commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder commented Jan 14, 2022

Uh oh!

scoder commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

eli-schwartz commented Jan 9, 2022 •

edited

Loading

scoder commented Jan 9, 2022 •

edited

Loading

scoder Jan 12, 2022 •

edited

Loading

scoder Jan 12, 2022 •

edited

Loading

scoder commented Jan 14, 2022 •

edited

Loading

ev-br commented Jan 14, 2022 •

edited

Loading

scoder commented Jan 14, 2022 •

edited

Loading