ENH: don't rebuild unchanged files #8897

juliantaylor · 2017-04-05T20:04:29Z

Only rebuild files that are newer than their corresponding object or if
older than their dependencies.

juliantaylor · 2017-04-05T20:05:36Z

I could have sworn distutils already did that and it stopped working at some point, am I remembering wrong?

rgommers · 2017-04-05T20:19:24Z

It has been doing that semi-reliably for a long time. Are you seeing complete rebuilds? What Python and setuptools versions?

juliantaylor · 2017-04-05T20:20:22Z

both python 2.7.13 and 3.5.3

rgommers · 2017-04-05T20:21:44Z

Hmm, I'll check tonight with my currently installed versions.

eric-wieser · 2017-04-06T09:33:44Z

numpy/distutils/ccompiler.py

+        return 0
+    newest = 0
+    for f in files:
+        newest = max(os.stat(f)[ST_MTIME], newest)


Or just os.stat(f).st_mtime, and skip the stat import. Same below

In fact, return max(files, key=lambda f: os.stat(f).st_mtime) could replace the last four lines

this returns the newest filename I guess the function should be called newest_file_time

In which case, return max(os.stat(f).st_mtime for f in files) is better. Yep, rename sounds sensible

rgommers · 2017-04-06T11:23:36Z

distutils does check file timestamps, you should see that if you rebuild the exact same commit twice in a row, then the second build does nothing (takes 1 sec). The issue (or feature?) you're likely seeing is that all files are rebuilt that together make up a single .so even if only one file was touched. This is due to the newer_group check here (I think - just reading the code, haven't verified) which is implemented here

juliantaylor · 2017-04-06T11:29:36Z

distutils does not rebuild if nothing changed, but it rebuilds everything if one file changed. I thought it didn't do that in the past, but maybe I am imagining that as every other buildsystem works like that ...
This fixes this, but it has the drawback that the depends need to be set correctly as there is no automatic determination.

juliantaylor · 2017-04-06T13:35:46Z

should we add automatic dependency determination? its not difficult on unix platforms

juliantaylor · 2017-04-06T17:08:17Z

This is how it would look like.
If we are paranoid we could add a check for compiler support. I guess CCompiler_customize would be the right place for that?

rgommers · 2017-04-10T08:54:31Z

distutils does not rebuild if nothing changed, but it rebuilds everything if one file changed. I thought it didn't do that in the past, but maybe I am imagining that as every other buildsystem works like that ...

If with "everything" you mean all files for a given extension module, then we are saying the same thing.

You can verify that different modules are independent

time python setup.py build_ext -i   # ~1min 30s
time python setup.py build_ext -i  # ~1s
touch numpy/core/src/npysort/binsearch.c
time python setup.py build_ext -i  # ~18s

This PR still looks fairly straightforward with the automatic dependency checking, so +1 to give it a go.

rgommers · 2017-04-10T08:54:53Z

How did you test this?

rgommers · 2017-04-10T08:55:40Z

numpy/distutils/unixccompiler.py

@@ -44,8 +44,9 @@ def UnixCCompiler__compile(self, obj, src, ext, cc_args, extra_postargs, pp_opts
            self.linker_so = llink_s.split() + opt.split()

    display = '%s: %s' % (os.path.basename(self.compiler_so[0]), src)
+    deps = ['-MMD', '-MF', obj + '.d']


this deserves a comment explaining what's going on here. I could figure it out, but those are not flags one uses every day

rgommers · 2017-04-10T08:58:16Z

If we are paranoid we could add a check for compiler support.

You mean checking that the rebuild is using the same compiler as the previous build?

juliantaylor · 2017-04-10T09:41:15Z

no, I meant checking if the compiler supports the commandline flags

rgommers · 2017-04-10T09:58:52Z

Ah, that's probably a healthy idea. We have people building on AIX, Raspberry Pi, and other assorted exotic platforms. CCompiler_customize should be the right place.

This would mess up automatic compile dependency determination.

juliantaylor · 2017-04-10T10:48:41Z

Updated with more comments and a flag support check. It is skipped for gcc and clang which always support it as the customize is run on every extension so it actually costs measurable time in a single file recompile.

juliantaylor · 2017-04-10T11:25:41Z

probably release note worthy if we still want it in the next release

Use these dependencies to avoid unnecessary recompilations of unchanged files.

rgommers · 2017-04-10T19:18:27Z

CI is failing

probably release note worthy if we still want it in the next release

agreed

juliantaylor · 2017-04-10T19:21:53Z

ci looks good, it failed earlier due to msvc in distutils being stupid. Fixed that and I removed even trying on non posix.

rgommers · 2017-04-19T11:56:00Z

Okay, let's give this a try. In it goes. Thanks Julian!

rgommers · 2017-04-19T11:56:19Z

Want to send a follow up PR for release notes edit?

charris · 2017-05-01T17:56:36Z

This broke SciPy testing, see scipy/scipy#7321. I don't know if this is a problem with the SciPy setup, but in any case i'm inclined to revert it until we can figure out what is going on.

This reverts commit 3b2a7a7, it was causing errors in Scipy testing. Closes numpy#9035.

pv · 2017-05-01T18:13:41Z

numpy/distutils/ccompiler.py

@@ -191,7 +230,8 @@ def CCompiler_compile(self, sources, output_dir=None, macros=None,

    def single_compile(args):
        obj, (src, ext) = args
-        self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)
+        if _needs_build(obj):
+            self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)


The issue vs. #9035 is probably that _needs_build should also check if any compilation flags differ from the previous compilation.

Could well be. I need to delete the SciPy build directory in order to run tests that show the problem if it was working in the previous build.

Deleting might not help, because the same file is built two times with different options during the same build.

Just referring to my local build. Another problem I ran into was that in order to cythonize the source I needed to do a Python2 build first. I have cython installed for Python 3 but it goes under the name cython3.

hm I though I tested scipy with this, apparently I forgot to run the tests

checking the arguments is simple enough, would be nicer to add the extension name to the dependency filename instead but it is probably clunky to get that from the compiler object.

rgommers added the component: numpy.distutils label Apr 5, 2017

eric-wieser reviewed Apr 6, 2017

View reviewed changes

juliantaylor force-pushed the no-rebuild branch from 2b66b68 to e7ad056 Compare April 6, 2017 11:34

rgommers reviewed Apr 10, 2017

View reviewed changes

juliantaylor added 2 commits April 10, 2017 12:05

MAINT: don't rewrite api headers unnecessarily

ae06915

This would mess up automatic compile dependency determination.

MAINT: remove hashlib fallback for python < 2.5

38e152a

juliantaylor force-pushed the no-rebuild branch from ed97950 to cf02cc8 Compare April 10, 2017 10:46

juliantaylor force-pushed the no-rebuild branch 3 times, most recently from 1226555 to 8f400b5 Compare April 10, 2017 11:25

juliantaylor added 2 commits April 10, 2017 17:35

ENH: automatically determine compile dependencies

a448495

Use these dependencies to avoid unnecessary recompilations of unchanged files.

MAINT: update .gitignore

fa00be8

juliantaylor force-pushed the no-rebuild branch from 8f400b5 to fa00be8 Compare April 10, 2017 15:36

rgommers merged commit 3b2a7a7 into numpy:master Apr 19, 2017

rgommers added this to the 1.13.0 release milestone Apr 19, 2017

person142 mentioned this pull request Apr 20, 2017

Failures with the master branch of numpy scipy/scipy#7321

Closed

charris mentioned this pull request May 1, 2017

SciPy testing broken by #8897 #9035

Closed

charris added a commit to charris/numpy that referenced this pull request May 1, 2017

BUG: Revert "Merge pull request numpy#8897 from juliantaylor/no-rebuild"

807ac7d

This reverts commit 3b2a7a7, it was causing errors in Scipy testing. Closes numpy#9035.

pv reviewed May 1, 2017

View reviewed changes

charris mentioned this pull request May 1, 2017

BUG: Revert "Merge pull request #8897 from juliantaylor/no-rebuild" #9036

Closed

juliantaylor deleted the no-rebuild branch May 1, 2017 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: don't rebuild unchanged files #8897

ENH: don't rebuild unchanged files #8897

juliantaylor commented Apr 5, 2017

juliantaylor commented Apr 5, 2017

rgommers commented Apr 5, 2017

juliantaylor commented Apr 5, 2017

rgommers commented Apr 5, 2017

eric-wieser Apr 6, 2017

eric-wieser Apr 6, 2017

juliantaylor Apr 6, 2017

eric-wieser Apr 6, 2017

rgommers commented Apr 6, 2017

juliantaylor commented Apr 6, 2017 •

edited

juliantaylor commented Apr 6, 2017

juliantaylor commented Apr 6, 2017

rgommers commented Apr 10, 2017

rgommers commented Apr 10, 2017

rgommers Apr 10, 2017

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 19, 2017

rgommers commented Apr 19, 2017

charris commented May 1, 2017

pv May 1, 2017

charris May 1, 2017

pv May 1, 2017 •

edited

charris May 1, 2017

juliantaylor May 1, 2017

ENH: don't rebuild unchanged files #8897

ENH: don't rebuild unchanged files #8897

Conversation

juliantaylor commented Apr 5, 2017

juliantaylor commented Apr 5, 2017

rgommers commented Apr 5, 2017

juliantaylor commented Apr 5, 2017

rgommers commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgommers commented Apr 6, 2017

juliantaylor commented Apr 6, 2017 • edited

juliantaylor commented Apr 6, 2017

juliantaylor commented Apr 6, 2017

rgommers commented Apr 10, 2017

rgommers commented Apr 10, 2017

Choose a reason for hiding this comment

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 10, 2017

juliantaylor commented Apr 10, 2017

rgommers commented Apr 19, 2017

rgommers commented Apr 19, 2017

charris commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pv May 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliantaylor commented Apr 6, 2017 •

edited

pv May 1, 2017 •

edited