sage --clone: do not rebuild the entire Sage library, and do not rebuild the docs #13245

jhpalmieri · 2012-07-12T23:38:22Z

With the attached patches, sage --clone

does no longer rebuild the reference manual,
does no longer unnecessarily rebuild the whole Sage library (just because of a missing Cython version file).

In e.g. the sage-combinat script, there are usually so many patches applied in the queue, it makes more sense to (re)build the manual after applying the patches, not before.

The unintended rebuild of all extension modules upon cloning is a bug introduced by #13031.

Apply attachment: trac_13245-clone-nodocbuild.patch and attachment: trac_13245-clone-cython.patch.

Depends on #14721

CC: @nthiery @saliola @nexttime @hivert @jdemeyer

Component: scripts

Author: John Palmieri, Leif Leonhardy

Reviewer: John Palmieri, Travis Scrimshaw, Leif Leonhardy

Merged: sage-5.10.rc2

Issue created by migration from https://trac.sagemath.org/ticket/13245

The text was updated successfully, but these errors were encountered:

jhpalmieri · 2012-07-12T23:39:08Z

scripts repo

jhpalmieri · 2012-07-12T23:39:54Z

comment:1

Attachment: trac_13245-clone-docbuild.patch.gz

jhpalmieri · 2013-03-11T17:55:39Z

comment:2

Attachment: trac_13245-clone-nodocbuild.patch.gz

Here is a different patch; this one just turns off docbuilding in sage-clone altogether.

nexttime · 2013-05-15T12:53:27Z

comment:3

Ping.

Not directly related, but meanwhile sage-clone also regenerates all Cython-generated files, because we don't copy .cython_version, which is fixed by:

diff --git a/sage-clone b/sage-clone
--- a/sage-clone
+++ b/sage-clone
@@ -53,6 +53,10 @@

 cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))

+if os.path.isfile('sage/.cython_version'):
+    print "Copying over hidden Cython version file..."
+    os.link('sage/.cython_version', branch+'/.cython_version')
+
 def copy_dtree(src_dir, dest_dir):
     src_root = os.path.abspath(src_dir)
     dest_root = os.path.abspath(dest_dir)

But there are other issues as well (e.g. rebuilding the fast_callable interpreters, which IMHO isn't necessary either).

nexttime · 2013-05-15T13:08:37Z

comment:4

Replying to @nexttime:

But there are other issues as well (e.g. rebuilding the fast_callable interpreters, which IMHO isn't necessary either).

Oh, forgot: Cython-generated header files (*.h) don't contain a special comment on that on the first line (which is probably an upstream Cython bug), so the following currently doesn't work as expected either:

            if ext in ['.h', '.c', '.cpp']:
                if 'Cython' in open(src + '/' + F).readline():
                    os.link(src + '/' + F, dest + '/' +F)
                    os.utime(dest + '/' +F, None)

(For the moment, we could look for ^#ifndef __PYX_HAVE__sage__ in those, probably even without the sage__.)

jhpalmieri · 2013-05-15T22:10:54Z

comment:5

Okay, here is a patch to deal with the .h files and .cython_version. Rebuilding the fast_callable interpreters doesn't take too much time, so I'm not dealing with it right now. Leif, would you like to be listed as an author?

jhpalmieri · 2013-05-15T22:11:12Z

Attachment: trac_13245-clone-cython.patch.gz

nexttime · 2013-05-16T13:33:23Z

comment:6

Replying to @jhpalmieri:

Rebuilding the fast_callable interpreters doesn't take too much time, so I'm not dealing with it right now.

Yes, but it also triggers the rebuild of (currently) at least one other Cython module (besides the newly generated five sage.ext.interpreters.wrapper_*.pyx):

...
Building interpreters for fast_callable
Updating Cython code....
Compiling sage/plot/plot3d/parametric_surface.pyx because it depends on ./sage/ext/interpreters/wrapper_rdf.pxd.
...
building 'sage.plot.plot3d.parametric_surface' extension
...

(I think we could just copy over all files in sage/ext/interpreters/, which is in .hgignore.)

Note sure what or whether doc-rebuilding is triggered (upon next sage --docbuild ...) by rebuilding the interpreters though.

Could you explain why we "touch" almost all manually copied (more precisely, linked) files?

Leif, would you like to be listed as an author?

Does the patch introduce new environment variables? :P

[Haven't tried the patches here yet.]

nexttime · 2013-05-16T13:53:33Z

comment:7

Replying to @nexttime:

Could you explain why we "touch" almost all manually copied (more precisely, linked) files?

Ok, seems like Mercurial is to blame. We could use hg clone -U though instead, and create the working copy by ourselves. (Not tested.)

nexttime · 2013-05-16T14:10:04Z

comment:8

Replying to @nexttime:

Replying to @nexttime:

Could you explain why we "touch" almost all manually copied (more precisely, linked) files?

Ok, seems like Mercurial is to blame. We could use hg clone -U though instead, and create the working copy by ourselves. (Not tested.)

Or change back the modification times of the files (of the working copy) Mercurial created... ;-)

OMG.

nexttime · 2013-05-16T14:43:56Z

comment:9

Replying to @nexttime:

Replying to @nexttime:

Ok, seems like Mercurial is to blame. We could use hg clone -U though instead, and create the working copy by ourselves. (Not tested.)

Or change back the modification times of the files (of the working copy) Mercurial created... ;-)

OMG.

Thank you, nanny!

(How about making this optional?!?)

nexttime · 2013-05-16T15:44:32Z

comment:10

Since apparently hg archive (meanwhile?) doesn't preserve modification times either, we could use hg clone -U and some cp -pPR of hg manifest magic instead (of plain hg clone).

jhpalmieri · 2013-05-16T17:29:55Z

comment:11

Given the upcoming transition to git, I don't think it's worth trying to wrestle with Mercurial very much. I don't want to spend much more time on it, anyway.

nexttime · 2013-05-16T17:33:31Z

comment:12

Replying to @jhpalmieri:

Given the upcoming transition to git, I don't think it's worth trying to wrestle with Mercurial very much. I don't want to spend much more time on it, anyway.

Yes, hopefully.

Anyway, the title (and parts of the description) still advertise an option to disable docbuilding, but the currently listed patch just disables it unconditionally. Which way do we want to go?

jhpalmieri · 2013-05-16T19:39:41Z

comment:13

I think it makes more sense not to build the docs at all. But I don't use sage --clone myself, I only opened this in response to experiences of various sage-combinat users.

jhpalmieri · 2013-05-31T18:55:14Z

Reviewer: John Palmieri

jhpalmieri · 2013-05-31T18:55:14Z

comment:14

I think that rebuilding the files in sage/ext/interpreters/ is because of code in devel/sage/setup.py, and any changes to this should be on another ticket.

Meanwhile, the patch attachment: trac_13245-clone-cython.patch seems to fix the main cloning problem. I'm happy with that one.

jhpalmieri · 2013-05-31T18:55:14Z

Changed author from John Palmieri to John Palmieri, Leif Leonhardy

nexttime · 2013-06-06T08:20:51Z

comment:15

Adjusting priority to reflect the changed subject.

Will try to finally review this asap...

tscrim · 2013-06-10T15:41:41Z

comment:40

a) I only applied the two patches to the scripts repo, nothing to the main repo (I had just built sage, didn't even start it yet).

b) Not 100% but almost since there are far fewer extensions that get rebuilt after pushing/popping the entire combinat queue, and it seemed like the same number of modules after running -sync-build (which currently erases all cython files). I can't tell you the interesting lines because I didn't think I would need it (and I really don't want to go through rebuilding it all again) At least afterwards, switching between branches is fast. Nevertheless I would think changes in one branch's built extensions would not affect another's...

Did you run

sage --clone foo
sage -b foo

and then

sage -b main

?

nexttime · 2013-06-10T16:02:48Z

comment:41

Replying to @tscrim:

Did you run

sage --clone foo
sage -b foo

and then

sage -b main

?

Not explicitly, since sage-clone already runs sage -b in the new clone.

jhpalmieri · 2013-06-10T17:51:56Z

comment:42

Just for reference: with Sage 5.10.rc1 (not that the version should matter, as far as I know), I applied the two patches here and did

./sage -b   # to make sure everything was up to date
./sage --combinat install

and it only built 6 Cython files before applying the combinat queue:

Building interpreters for fast_callable
Updating Cython code....
Compiling sage/plot/plot3d/parametric_surface.pyx because it depends on ./sage/ext/interpreters/wrapper_rdf.pxd.
Compiling sage/ext/interpreters/wrapper_rdf.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_cdf.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_rr.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_py.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_el.pyx because it changed.
Cythonizing sage/ext/interpreters/wrapper_cdf.pyx
Cythonizing sage/ext/interpreters/wrapper_el.pyx
Cythonizing sage/ext/interpreters/wrapper_py.pyx
Cythonizing sage/ext/interpreters/wrapper_rdf.pyx
Cythonizing sage/ext/interpreters/wrapper_rr.pyx
Cythonizing sage/plot/plot3d/parametric_surface.pyx
Finished compiling Cython code (time = 17.591463089 seconds)
running install
running build
running build_py
running build_ext
warning: Replacing library search directory in linker command:
  "/Users/palmieri/Desktop/Sage_stuff/sage_builds/clean/sage-5.10.rc1/local/lib" -> "/Users/palmieri/Desktop/Sage_stuff/sage_builds/sage-5.10.rc1/local/lib"

building 'sage.plot.plot3d.parametric_surface' extension
building 'sage.ext.interpreters.wrapper_rdf' extension
building 'sage.ext.interpreters.wrapper_cdf' extension
building 'sage.ext.interpreters.wrapper_rr' extension
building 'sage.ext.interpreters.wrapper_py' extension
building 'sage.ext.interpreters.wrapper_el' extension
Executing 6 commands (using 2 threads)
...

tscrim · 2013-06-10T18:01:37Z

comment:43

Here's a snippet of what I get -- with no patches applied, double-checking the patches are applied in local/bin, and then running sage -b -- when I run

sage --clone foo
...
Building interpreters for fast_callable
Updating Cython code....
Compiling sage/plot/plot3d/parametric_surface.pyx because it depends on ./sage/ext/interpreters/wrapper_rdf.pxd.
Compiling sage/ext/interpreters/wrapper_rdf.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_cdf.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_rr.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_py.pyx because it changed.
Compiling sage/ext/interpreters/wrapper_el.pyx because it changed.
Cythonizing sage/ext/interpreters/wrapper_cdf.pyx
Cythonizing sage/ext/interpreters/wrapper_el.pyx
Cythonizing sage/ext/interpreters/wrapper_py.pyx
Cythonizing sage/ext/interpreters/wrapper_rdf.pyx
Cythonizing sage/ext/interpreters/wrapper_rr.pyx
Cythonizing sage/plot/plot3d/parametric_surface.pyx
Finished compiling Cython code (time = 70.2989528179 seconds)
running install
running build
running build_py
running build_ext
building 'sage.algebras.quatalg.quaternion_algebra_element' extension
building 'sage.algebras.letterplace.free_algebra_letterplace' extension
...
building 'sage.ext.interpreters.wrapper_py' extension
building 'sage.ext.interpreters.wrapper_el' extension
Executing 343 commands (using 1 thread)

I'll upload a full log once it's done. Could it be because I didn't run sage -b after first installing the patches the first time around? Either that or (more likely) it's something specific to me or my system...

tscrim · 2013-06-10T19:33:07Z

comment:44

Attachment: build_log.txt

Here's a log from calling sage -b main afterwards (because I stupidly overwrote the first log).

jhpalmieri · 2013-06-10T19:52:17Z

comment:45

When I called sage -b main afterwards, I got


----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
scons: `install' is up to date.
Updating Cython code....
Finished compiling Cython code (time = 11.5694179535 seconds)
running install
running build
running build_py
running build_ext
warning: Replacing library search directory in linker command:
  "/Users/palmieri/Desktop/Sage_stuff/sage_builds/clean/sage-5.10.rc1/local/lib" -> "/Users/palmieri/Desktop/Sage_stuff/sage_builds/sage-5.10.rc1/local/lib"

Executing 0 commands (using 1 thread)
Time to execute 0 commands: 0.0257179737091 seconds
Total time spent compiling C/C++ extensions:  0.278106927872 seconds.
running install_lib
running install_egg_info
Removing /Users/palmieri/Desktop/Sage_stuff/sage_builds/sage-5.10.rc1/local/lib/python2.7/site-packages/sage-0.0.0-py2.7.egg-info
Writing /Users/palmieri/Desktop/Sage_stuff/sage_builds/sage-5.10.rc1/local/lib/python2.7/site-packages/sage-0.0.0-py2.7.egg-info

real	0m17.365s
user	0m6.021s
sys	0m2.604s

So it looks like there is something odd with your system.

nexttime · 2013-06-10T19:56:33Z

comment:46

Replying to @nexttime:

Replying to @tscrim:
Did you run
sage --clone foo
sage -b foo
and then
sage -b main
?
Not explicitly, since sage-clone already runs sage -b in the new clone.

But even when I do, that doesn't make any difference, as expected.

nexttime · 2013-06-10T19:59:57Z

comment:47

P.S.: I also touched some pyx files in the clone, ran sage -b, then sage -b main. Not surprisingly, the latter didn't trigger any rebuilds either.

jhpalmieri · 2013-06-10T20:38:51Z

comment:48

Just to make sure, when you run ./sage --hg -R local/bin log, it shows that the two patches have been applied, right?

tscrim · 2013-06-10T20:57:08Z

comment:49

travis@travis-virtualbox:~/sage-5.10.rc1$ sage --hg -R local/bin log | head -n 21
changeset:   1953:64e1b84c2d39
tag:         qtip
tag:         tip
tag:         trac_13245-clone-nodocbuild.patch
user:        J. H. Palmieri <palmieri@math.washington.edu>
date:        Mon Mar 11 10:53:04 2013 -0700
summary:     Don't rebuild docs after cloning.

changeset:   1952:148e4ed48517
tag:         qbase
tag:         trac_13245-clone-cython.patch
user:        J. H. Palmieri <palmieri@math.washington.edu>
date:        Mon Mar 11 10:53:04 2013 -0700
summary:     sage-clone: copy over autogenerated .h files, and also .cython_version

changeset:   1951:0eb3c4df6d10
tag:         qparent
user:        Jeroen Demeyer <jdemeyer@cage.ugent.be>
date:        Wed Jun 05 22:11:28 2013 +0000
summary:     5.10.rc1

So it's probably just something I messed up with my install (my guess by applying the patches and then running -combinat install before (rebuilding and) starting sage) and/or system.

nexttime · 2013-06-10T21:03:50Z

comment:50

I think^TM I know what the problem might be, namely the order in which subdirectories of devel/sage/build/ are copied.

Since the odd addition of build_dir in setup.py, which already breaks sage-sync-build, the Cython-generated .c and .cpp files also end up below build/.

Now if these get copied (and touched!) after their corresponding .o and .so files, the latter will get rebuilt.

Travis, could you switch back to the main branch, remove the line setting build_dir from setup.py (or change build_dir='build/cythonized' to build_dir=None), do sage -b to get a clean new main repo, and retry your test? B)

tscrim · 2013-06-10T21:28:44Z

comment:51

Doing so right now, although the change seems to have triggered a recompile/recythonizing of the cython files (it might have been a side effect of one of my other tests). However I need to sleep, so I'll finish the test in the morning. Thanks.

nexttime · 2013-06-10T22:22:28Z

comment:52

Replying to @nexttime:

I think^TM I know what the problem might be, namely the order in which subdirectories of devel/sage/build/ are copied.

Since the odd addition of build_dir in setup.py, which already breaks sage-sync-build, the Cython-generated .c and .cpp files also end up below build/.

Now if these get copied (and touched!) after their corresponding .o and .so files, the latter will get rebuilt.

With

def copy_dtree(src_dir, dest_dir):
    src_root = os.path.abspath(src_dir)
    dest_root = os.path.abspath(dest_dir)

    for root, dirs, files in os.walk(src_root):
        nroot = dest_root + root[len(src_root):]
        for d in dirs:
            os.makedirs(nroot+'/'+d)
        for f in files:
            if os.path.splitext(f)[1] in [".c",".cpp",".o",".so"]: # ADDED
                print("  %s" % f) # ADDED
            os.link(root+'/'+f,nroot+'/'+f)
            os.utime(nroot+'/'+f, None)

sys.stdout.flush(); sys.stderr.flush() # ADDED
print "Copying over build directory..."
copy_dtree('sage/build', branch + '/build')
sys.stdout.flush(); sys.stderr.flush() # ADDED

I can at least confirm that for me all .o and .so files get copied after all .c and .cpp files have been copied -- rather incidentally I think.

tscrim · 2013-06-11T07:25:52Z

comment:53

My .o files get copied, then my .c and .cpp, then my .so (with the changes to setup.py).

tscrim · 2013-06-11T07:34:22Z

comment:54

Also I only get the rebuilding of the 7 extensions and switch back to main is trivial.

jdemeyer · 2013-06-11T13:51:45Z

comment:55

Any progress? Does this still deserve to be a Sage 5.10 blocker?

nexttime · 2013-06-11T16:19:24Z

comment:56

Replying to @jdemeyer:

Any progress? Does this still deserve to be a Sage 5.10 blocker?

At least the .cython_version part here.

And we should really disable (postpone) Cython-generated files "out-of-tree", since this breaks both sage-clone and sage-sync-build, i.e., set build_dir to None in setup.py.

([Even] with that, upgrading from beta4+ will still trigger the rebuild of the whole Sage library, but only once.)

nexttime · 2013-06-11T16:25:06Z

comment:57

Replying to @nexttime:

And we should really disable (postpone) Cython-generated files "out-of-tree", since this breaks both sage-clone and sage-sync-build, i.e., set build_dir to None in setup.py.

Inline patch, worth its own (blocker) ticket:

diff --git a/setup.py b/setup.py
--- a/setup.py
+++ b/setup.py
@@ -537,7 +537,8 @@
     ext_modules = cythonize(
         ext_modules,
         nthreads = int(os.environ.get('SAGE_NUM_THREADS', 0)),
-        build_dir = 'build/cythonized',
+        build_dir = None, # Don't "cythonize out-of-tree" (cf. #14570) until
+                          # sage-clone and sage-sync-build can deal with that.
         force=force)
 
     open(version_file, 'w').write(Cython.__version__)

EDIT: Added ticket reference to the patch.

nexttime · 2013-06-11T16:31:14Z

comment:58

Replying to @jdemeyer:

Any progress?

I was (more or less reluctantly) going to give this positive review, but then Travis discovered that using build_dir can also break sage-clone.

So I'm happy provided we make this ticket depend on the above patch / its ticket.

nthiery · 2013-06-11T16:39:24Z

comment:59

Replying to @jdemeyer:

Any progress? Does this still deserve to be a Sage 5.10 blocker?

It would be great indeed to have 5.10 ready for the Sage-Combinat days next week! There is a lot of good stuff there.

nexttime · 2013-06-11T16:49:04Z

comment:60

Replying to @nexttime:

Replying to @jdemeyer:

Any progress?

I was (more or less reluctantly) going to give this positive review, but then Travis discovered that using build_dir can also break sage-clone.

So I'm happy provided we make this ticket depend on the above patch / its ticket.

I've now created #14721.

nexttime · 2013-06-11T16:49:04Z

Changed reviewer from John Palmieri to John Palmieri, Travis Scrimshaw, Leif Leonhardy

nexttime · 2013-06-11T16:49:04Z

Dependencies: #14721

nexttime · 2013-06-11T17:12:00Z

comment:61

Replying to @nexttime:

Replying to @nexttime:

So I'm happy provided we make this ticket depend on the above patch / its ticket.

I've now created #14721.

Needs review... :P

jdemeyer · 2013-06-13T08:21:11Z

Merged: sage-5.10.rc2

jhpalmieri added this to the sage-5.10 milestone Jul 12, 2012

jhpalmieri added c: scripts labels Jul 12, 2012

jhpalmieri assigned nexttime Jul 12, 2012

jhpalmieri added the s: needs review label Jul 12, 2012

This comment has been minimized.

Sign in to view

jhpalmieri changed the title ~~sage --clone: give option to not rebuild the docs~~ sage --clone: do not rebuild the entire Sage library, and do not rebuild the docs May 31, 2013

nexttime mannequin added p: blocker / 1 and removed p: minor / 4 labels Jun 6, 2013

nexttime mannequin added s: positive review and removed s: needs review labels Jun 11, 2013

jdemeyer removed the s: positive review label Jun 13, 2013

jdemeyer closed this as completed Jun 13, 2013

nexttime mannequin mentioned this issue Jun 13, 2013

Disable "cythonizing out-of-tree" until sage-sync-build and sage-clone can deal with that #14721

Closed

sage --clone: do not rebuild the entire Sage library, and do not rebuild the docs #13245

sage --clone: do not rebuild the entire Sage library, and do not rebuild the docs #13245

Comments

jhpalmieri commented Jul 12, 2012

jhpalmieri commented Jul 12, 2012

jhpalmieri commented Jul 12, 2012

jhpalmieri commented Mar 11, 2013

This comment has been minimized.

nexttime mannequin commented May 15, 2013

nexttime mannequin commented May 15, 2013

This comment has been minimized.

jhpalmieri commented May 15, 2013

jhpalmieri commented May 15, 2013

nexttime mannequin commented May 16, 2013

nexttime mannequin commented May 16, 2013

nexttime mannequin commented May 16, 2013

nexttime mannequin commented May 16, 2013

nexttime mannequin commented May 16, 2013

jhpalmieri commented May 16, 2013

nexttime mannequin commented May 16, 2013

jhpalmieri commented May 16, 2013

jhpalmieri commented May 31, 2013

jhpalmieri commented May 31, 2013

jhpalmieri commented May 31, 2013

nexttime mannequin commented Jun 6, 2013

tscrim commented Jun 10, 2013

nexttime mannequin commented Jun 10, 2013

jhpalmieri commented Jun 10, 2013

tscrim commented Jun 10, 2013

tscrim commented Jun 10, 2013

jhpalmieri commented Jun 10, 2013

nexttime mannequin commented Jun 10, 2013

nexttime mannequin commented Jun 10, 2013

jhpalmieri commented Jun 10, 2013

tscrim commented Jun 10, 2013

nexttime mannequin commented Jun 10, 2013

tscrim commented Jun 10, 2013

nexttime mannequin commented Jun 10, 2013

tscrim commented Jun 11, 2013

tscrim commented Jun 11, 2013

jdemeyer commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nthiery commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

nexttime mannequin commented Jun 11, 2013

jdemeyer commented Jun 13, 2013