feat: way to only recompile changed files

[henryiii]

Description

Adding a "cache" feature that assists in development. If a source has not changed since the file has been created, you can just reuse the old compile. Opt-in, and really only affects development builds (pip install -e . or similar), since otherwise everything is built in a clean, new directory. Requested by @HDembinski for the HEP/Astro project iMinuit. I've based it on the existing code there, and verified that it works on boost-histogram (the python_example only has one file, which makes it rather uninformative for testing this). Added notes in the docs about CCache, too.

from pybind11.setup_helpers import ParallelCompile, simple_recompile

# Or, custom function:
# def simple_recompile(obj, src):
#     return os.stat(obj).st_mtime < os.stat(src).st_mtime

ParallelCompile("NPY_NUM_BUILD_JOBS", needs_recompile = simple_recompile).install()

Suggested changelog entry:

* Added ``needs_recompile`` optional function to the ParallelCompiler helper, to allow a recompile to be skipped based on a user-defined function.

[henryiii]

PS: this is a very minor feat, so I'd be okay with it going into 1.6.1, what do you think?

[YannickJadoul]

Why don't we just always advise ccache? That seems like the obvious way to do this, taking all kinds of subtleties into account, rather than adding unrelated code to pybind11. (I know that's putting it rather harshly; intentionally so, since I expect you have a good answer ;-) )

Same as with the ParallelCompile thing: I'm a bit weary on why pybind11 should offer this, and why it's not a separate package/project that can be pip installed/included as CMake submodule or subtree/...

[henryiii]

CCache is a separate dependency, and not very officially supported on Windows. This is a simpler (and off-by-default) workaround that works everywhere, and enabling it doesn't affect "normal" users, as a non-editable install is in a new build folder anyway. It's also easy for us to include here, but hard for a user to inject.

It's not trivial to include dependencies at setuptime, each method of doing so (PEP 518, setup_requires, submodules, copy-and-paste) has drawbacks and doesn't work everywhere (conda disables most of them, for example). IMO, pybind11 should offer reasonable assistance with users wanting to use setuptools, because that's a key part of writing a good pybind11 module.

If, for example, we added ParallelCompile to a package, then it would not be available for the non-setuptools users, because we can't ask for dependencies like a normal Python package can. If we add it as a submodule, it breaks users on GitLab. Either way, it wouldn't work for add_submodule usage. If we require uses to also use that package to get the feature, then they would have to solve the problem of adding the new package again, after adding pybind11.

[YannickJadoul]

This is a simpler (and off-by-default) workaround that works everywhere, and enabling it doesn't affect "normal" users, as a non-editable install is in a new build folder anyway.

IMO, pybind11 should offer reasonable assistance with users wanting to use setuptools

That's true, so I'm not really opposed to it; you managed to convince me of ParallelCompile as well.

It's not trivial to include dependencies at setuptime

But ... pybind11 is already a dependency at setuptime? One that users either require with pip or conda, or include as submodule/subtree, or vendor, or ... Why would this be so much different? If users can install pybind11, why couldn't they install in a very similar way "henryiii/setuptoolstools" (or something like that; I'm not pinning you to this horrible name :-p ). You already have this whole explanation of setup_helpers, and we're not using this ParallelCompile ourselves in pybind11, are we?

But, as I said, I don't oppose including it. To some degree this is just an additional feature/fix to the already existing ParallelCompile, so...

(EDIT: I just find it weird that users would need to install pybind11 to use this feature.)

[henryiii]

My point was any external solution will significantly complicate usage from pybind11, where I expect most users will be. This makes life much easier for simple to intermediate pybind11 users (advanced users probably will set everything up for themselves anyway).

[HDembinski]

Hi, just to chime in, @YannickJadoul you are right, of course, that this functionality and parallel builds should ideally come from setuptools, but that's unfortunately not the case. @henryiii made a good case against ccache and it is a rather small patch with a lot of convenience value, so not really adding bloat to pybind11. I think everyone here would be happy to rip this out again once this functionality is available somewhere upstream.

I missed why lazy was renamed to only_changed, I would prefer the shorter lazy if it is opt-in, because that means anyone who actually wants to use this has to type it a lot. And it is pretty much the usual term for this sort of thing

[henryiii]

lazy sounds like it's delaying compilation or changing the parallel method somehow; it's not quite as clear as only_changed, which describes exactly what it's going to do if you turn it on (only recompile changed files). It's not something you type often (assuming even with the most rapid prototyping you do not make a new setup.py every day? And copy-paste is rather common in setup code), and it's rather specialized, so I'd rather have it read like documentation than save a few characters. I'm not strongly against lazy either, though.

I have tested this locally and verified it works with pip install -e . -v; it's a nice speed boost even in a parallel environment if you edit a quicker-to-compile file!

Remember, it will look like this:

# Use the environment variable NPY_NUM_BUILD_JOBS
ParallelCompile("NPY_NUM_BUILD_JOBS", only_changed=True).install()
# or
ParallelCompile("NPY_NUM_BUILD_JOBS", lazy=True).install()

Once this is out of context (not in this thread), I could see the latter line being misconstrued as some setting on the parallel method rather than just only recompiling changed files.

These were/are the suggested names:

• lazy=True
• cached=True
• incremental=True
• only_changed=True
• recompile_all=False
• always_recompile=False

only_changed sits about in the middle as far as length goes. :)

[YannickJadoul]

@HDembinski

this functionality and parallel builds should ideally come from setuptools, but that's unfortunately not the case.

True, or some independent plugin. E.g., I don't see why this couldn't be interesting for Cython project. And since our docs suggest vendoring the file or using PEP 518, there's not a lot of extra complexity if a user has to add this ParallelCompile in the same way?

@henryiii made a good case against ccache and it is a rather small patch with a lot of convenience value, so not really adding bloat to pybind11. I think everyone here would be happy to rip this out again once this functionality is available somewhere upstream.

Agreed; I see the ccache argument, yes (though ccache still seems to be the correct way to set something up on the long term, to me?), and indeed, it's just tweaking ParallelCompile, so I'm fine to see this merged. I'm more concerned about re-usability and separation of concerns than about this not being maintainable :-)

And yes, lazy was nice and short, but Henry correctly summarized my doubts about that name. If it's decide that that connotation to lazy is not a problem, I'm fine with that as well, though.

[henryiii]

I don't see why this couldn't be interesting for Cython project

ParallelCompile is already available (in a less configurable form) in NumPy, which Cython users have to have at compile time anyway, while pybind11 users do not. Agree that eventually we might also make then available in a separate package, but that will complicate usage here. We probably would want to follow pip's method of including dependencies - it has dozens of dependencies, but they vendor them in - probably with git subtree or something similar (submodules have drawbacks for something so integral as pybind11).

[HDembinski]

It's not something you type often (assuming even with the most rapid prototyping you do not make a new setup.py every day?

Sorry, I misunderstood how this works, I thought I have to add only_changed as a command-line option to pip install. Forget my remark then. In fact, only_changed seems like the most literal description of what this does. 👍

[henryiii]

So I think we are around 2-3 votes for only_changed?

[henryiii]

I've noticed that NumPy does this by default: https://github.com/numpy/numpy/blob/cfee2df31bdc127c345894aee594cbcbb8cddd0e/numpy/distutils/ccompiler.py#L303

Should we consider changing the default from only_changed=False to only_changed=True in pybind11 2.7.0? Or here?

This check we have is not recursive; that is, we don't look up the headers this file depends on to see if those changed. For many pybind11 projects, this is fine - there sometimes aren't headers. Maybe it should remain optional, so that projects that aren't worried about headers changing can turn it on, but some projects may want it off?

[henryiii]

I've restructured this quite a bit. With the observation that this is not recursive (it can't tell if you changed a header file), I've pulled the check function out and instead provided a way for users to add their own, with the "simple" one as a suggestion. This a) doesn't assume anything about a user's project structure, b) keeps the code that we provide simple, and c) keeps us from falling down a rabbit hole if users start complaining about changing a header and not having it show up when they rebuild.

Many projects are probably fine and mostly .cpp files, but not all. As a counter-example, boost-histogram has more pybind11 code in headers than it does in .cpp files.

New usage:

from pybind11.setup_helpers import ParallelCompile

class SmartCompile(ParallelCompile):
    @staticmethod
    def needs_recompile(obj, src):
        return os.stat(obj).st_mtime < os.stat(src).st_mtime

SmartCompile("NPY_NUM_BUILD_JOBS").install()

Note that in my original reasoning I mentioned that one of the key reasons was that users couldn't modify ParallelCompile to add this. Now they can. :)

[henryiii]

Hopefully this will make @YannickJadoul happier, and @HDembinski if you could "review and approve" if you like it, that would be helpful too. :) (or, of course, you could make suggestions, etc.)

[rwgk]

Looks awesome to me :-)

[YannickJadoul]

Hopefully this will make @YannickJadoul happier

I wasn't per se unhappy about this PR ;-) Especially not about this part, because I didn't actually notice the recursive/header issue.
How does numpy handle this recursive case, then, btw?

Two suggestions:

• Rather than just in the docs, we could also provide this extra simple class, to avoid different project reimplementing this? NaiveIncrementalCompile/SimpleIncrementalCompile or so?
• Instead of subclassing, you could also accept a callable should_recompile? (Not convinced this is better, but throwing this out there, if anyone would prefer this to subclassing.)

Apart from that, sure, go ahead and merge, as far as I'm concerned :-)

[henryiii]

we could also provide this extra simple class

Sure, that would be useful, I think.

Instead of subclassing, you could also accept a callable

I thought about it when designing this - either way works, I slightly prefer the extra structure provided by subclassing (plus, that way we could provide a single import "SimpleIncrementalCompile" instead of having to get and use a second import for the function to pass it in. But no strong preference here. @HDembinski do you have an opinion?

The "import" version would have looked like this:

from pybind11.setup_helpers import ParallelCompile, simple_recompile

# Or, custom function:
# def simple_recompile(obj, src):
#     return os.stat(obj).st_mtime < os.stat(src).st_mtime

ParallelCompile("NPY_NUM_BUILD_JOBS", needs_recompile = simple_recompile).install()

[YannickJadoul]

The "import" version would have looked like this:

Yep, something like that is what I was thinking! No strong opinion either way, though (this is slightly more cumbersome, but also avoids subclassing, so potentially a slight increase in future combinatoriality?). naive might be better than simple, though, as a warning to users, or do you think that's too negative?

[HDembinski]

I thought about it when designing this - either way works, I slightly prefer the extra structure provided by subclassing (plus, that way we could provide a single import "SimpleIncrementalCompile" instead of having to get and use a second import for the function to pass it in. But no strong preference here. @HDembinski do you have an opinion?

I favor the "pass a callable" approach, because of minimal coupling and minimal design. The function is independent of the class. Requiring me to subclass something to add a staticmethod is not a minimal elegant design. Subclassing is the preferred solution only if it is conceivable that the callable may need to access privates of ParallelCompile, but it is designed anyway to not access self, so...

[HDembinski]

naive might be better than simple, though, as a warning to users, or do you think that's too negative?

I also like naive, gives a fair warning.

[henryiii]

Okay, you've convinced me - I've pushed a free function version.

[rwgk]

Nice!

[YannickJadoul]

Looks pretty clean!

[henryiii]

FYI, I've just added this section, as this is likely a common issue related to caching. @YannickJadoul

[HDembinski]

I am not an official reviewer, but for the record, it looks all good to me, thank you for the great work, @henryiii

[henryiii]

I think we have enough approvals, then. Thanks everyone!