Opened 7 years ago
Last modified 4 years ago
#12659 needs_work enhancement
build the sage library in place
Reported by:  mhansen  Owned by:  GeorgSWeber 

Priority:  major  Milestone:  sage6.4 
Component:  build  Keywords:  
Cc:  wstein, kini  Merged in:  
Authors:  Mike Hansen  Reviewers:  William Stein 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  #13363  Stopgaps: 
Description (last modified by )
Use distutils to build the Sage library inplace to save on space / make it clearer which files are actually being used in runtime.
See the discussion at http://groups.google.com/group/sagedevel/browse_thread/thread/d5aef15acc4d178b
Attachments (2)
Change History (55)
comment:1 Changed 7 years ago by
 Cc wstein added
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Description modified (diff)
comment:3 Changed 7 years ago by
comment:4 Changed 7 years ago by
I removed it since we don't need to copy those files anymore  they're already in the right place.
comment:5 Changed 7 years ago by
 Cc kini added
comment:6 Changed 7 years ago by
Mike  that makes sense. However, why do we have this code still:
422 if not self.inplace: 423 relative_ext_dir = os.path.split(relative_ext_filename)[0] 424 prefixes = ['', self.build_lib, self.build_temp] 425 for prefix in prefixes: 426 path = os.path.join(prefix, relative_ext_dir) 427 try: 428 os.makedirs(path) 429 except OSError, e: 430 assert e.errno==errno.EEXIST, 'Cannot create %s.' % path
Wouldn't the above only be run if we are not building in place? It seems to me that it would be best to either keep the code discussed in the comment above, or should replace all the code above by
422 if not self.inplace: 423 ERROR MESSAGE
I'm in favor of the latter, unless I'm misunderstanding things.
Changed 7 years ago by
comment:7 Changed 7 years ago by
Agreed  I've put an updated patch.
comment:8 Changed 7 years ago by
Regarding everything else about this patch, it seems to work perfectly (though I'm running a few tests), and in fact is REALLY, REALLY AWESOME. This must go into Sage ASAP! It's wonderful.
comment:9 Changed 7 years ago by
Thanks!
comment:10 Changed 7 years ago by
It seems to me that if this get's merged then we should undo the changes at #11235
comment:11 Changed 7 years ago by
 Status changed from needs_review to positive_review
I'm happy with this code.
And indeed the work at #11235 seems not necessary anymore.
comment:12 Changed 7 years ago by
 Reviewers set to William Stein
comment:13 Changed 7 years ago by
Could this have caused
sage t "devel/sagemain/sage/graphs/graph_decompositions/rankwidth.pyx" ********************************************************************** File "/tmp/jdemeyer/merger/sage5.0.beta9/devel/sagemain/sage/graphs/graph_decompositions/rankwidth.pyx", line 47: sage: rw, tree = g.rank_decomposition() Exception raised: Traceback (most recent call last): File "/tmp/jdemeyer/merger/sage5.0.beta9/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/tmp/jdemeyer/merger/sage5.0.beta9/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/tmp/jdemeyer/merger/sage5.0.beta9/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_0[3]>", line 1, in <module> rw, tree = g.rank_decomposition()###line 47: sage: rw, tree = g.rank_decomposition() File "/tmp/jdemeyer/merger/sage5.0.beta9/local/lib/python/sitepackages/sage/graphs/graph.py", line 2959, in rank_decomposition from sage.graphs.graph_decompositions.rankwidth import rank_decomposition ImportError: cannot import name rank_decomposition **********************************************************************
comment:14 Changed 7 years ago by
 Status changed from positive_review to needs_work
Indeed, it is. I have no explanation for the fact that only the import of this particular module fails. It was recently added in #11754, but how should that be relevant?
comment:15 followup: ↓ 17 Changed 7 years ago by
How did you get that particular error? How did you merge in the patch here?
comment:16 followup: ↓ 18 Changed 7 years ago by
Dumbest question ever, I'm sure, but I'll ask it  will this affect upgrades at all?
comment:17 in reply to: ↑ 15 Changed 7 years ago by
Replying to mhansen:
How did you get that particular error? How did you merge in the patch here?
Apply the patch, sdist, build Sage from scratch.
comment:18 in reply to: ↑ 16 Changed 7 years ago by
Replying to kcrisman:
Dumbest question ever, I'm sure, but I'll ask it  will this affect upgrades at all?
I certainly don't think this is a dumb question, in fact it is a very good question. I don't know the answer though.
comment:19 followup: ↓ 20 Changed 7 years ago by
Upgrades should be fine  when sage b
gets run, the sitepackages link will update to the new location, and the new extension modules should be built in place.
comment:20 in reply to: ↑ 19 Changed 7 years ago by
Upgrades should be fine  when
sage b
gets run, the sitepackages link will update to the new location, and the new extension modules should be built in place.
So the first time an upgrade happens, the entire Sage library will have to rebuild its extensions and then put them in the new location, correct?
comment:21 Changed 7 years ago by
Yes  it does not try to copy existing extension modules over.
comment:22 Changed 7 years ago by
By the way, I had the same experience as Jeroen: I applied the patches, did ./sage b
and ./sage sdist ...
. Then I built Sage from the resulting tar file, and got doctest failures:
sage t long force_lib devel/sage/sage/graphs/graph.py # 4 doctests failed sage t long force_lib devel/sage/sage/graphs/graph_decompositions/rankwidth.pyx # 11 doctests failed
Is the issue the C code in the directory sage/graphs/graph_decompositions/rankwidth/
? Somehow that code is now not being found by the Python and Cython code that uses it?
comment:23 Changed 7 years ago by
Looking at #11754, I think the issue is that there is a extension module sage.graphs.graph_decompositions.rankwidth as well as a package (with __init__.py
) sage/graphs/decompositions/rankwidth/ . I think that should really be fixed as it's ambiguous which should be used.
comment:24 Changed 7 years ago by
 Dependencies set to #12684
The code here should work fine when #12684 is applied.
comment:25 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:26 Changed 7 years ago by
 Status changed from needs_review to needs_work
The code at #12684 fixes the problems with the graph files. For me, though, the banner is not getting updated when I run sage sdist ...
. The file devel/sage/sage/version.py
is correct, but the file devel/sage/build/sage/version.py
is not. (This is after applying the patches here, running ./sage b
, then ./sage sdist ...
.)
comment:27 Changed 7 years ago by
I took a fresh Sage tarball, applied the patch here to the sage spkg, and then built Sage. Then I ran ./sage sdist ...
. I got this error:
running build_ext Traceback (most recent call last): File "setup.py", line 983, in <module> include_dirs = include_dirs) File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/core.py", line 152, in setup dist.run_commands() File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/dist.py", line 953, in run_commands self.run_command(cmd) File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/dist.py", line 972, in run_command cmd_obj.run() File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/command/install.py", line 563, in run self.run_command('build') File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/cmd.py", line 326, in run_command self.distribution.run_command(command) File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/dist.py", line 972, in run_command cmd_obj.run() File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/command/build.py", line 127, in run self.run_command(cmd_name) File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/cmd.py", line 326, in run_command self.distribution.run_command(command) File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/dist.py", line 972, in run_command cmd_obj.run() File "/scratch/palmieri/12659/sage5.0.beta12gcc/local/lib/python/distutils/command/build_ext.py", line 340, in run self.build_extensions() File "setup.py", line 318, in build_extensions raise RuntimeError, "only inplace builds are currently supported" RuntimeError: only inplace builds are currently supported
This doesn't stop the build, but do we need to worry about it? If not, we should suppress it.
Also, as mentioned above, I took an existing Sage installation, applied the patch to devel/sage, and ran sage sdist ...
. I got the same error message. Also, in the existing Sage installation, while devel/sage/sage/version.py
was correct, devel/sage/build/sage/version.py
was not. local/lib/python/sitepackages/sage
linked to build/sage/
, and this is what led to the incorrect sagebanner that I mentioned earlier. Should the installation instructions include forcibly relinking local/lib/.../sage
to the correct thing, if applying to an existing Sage installation?
comment:28 Changed 7 years ago by
I think the scripts patch should be this instead (that is, removing the old link in a different part of the script):

sagebuild
diff git a/sagebuild b/sagebuild
a b build() { 15 15 echo "sage: Building and installing modified Sage library files." 16 16 echo "" 17 17 18 # Remove this link. It will be recreated automatically; removing 19 # it now allows for the transition to build Sage in place (trac 20 # #12659), because it will replace a link to 21 # devel/sage/build/sage with a link to devel/sage/sage. 22 rm f "$SAGE_LOCAL/lib/python/sitepackages/sage" 18 23 # install c_lib 19 24 # we're doing this here instead of setup.py for historical 20 25 # reasons:
This doesn't help with the error message in my previous comment, but it might take care of the other issue.
comment:29 Changed 7 years ago by
 Dependencies #12684 deleted
 Status changed from needs_work to needs_review
I've updated trac_12659script.patch which should fix all of the outstanding problems mentioned on this ticket.
comment:30 Changed 7 years ago by
 Dependencies set to #13363
I was wrong  one doctest fails (due to a warning being shown) without the patch at #13363 added.
comment:31 Changed 7 years ago by
Just for esthetics: could you fix the spacing in trac_12659script.patch?
The patches look okay to me, and it seems to work, but it would be nice if someone who knew the build process (in particular, setup.py for the Sage library) better took a look at this.
comment:32 Changed 7 years ago by
Fixed up the spacing.
Changed 7 years ago by
comment:33 Changed 7 years ago by
I am testing this ticket (and a bunch of others) on the buildbot. Upgrading fails: everything builds but Sage doesn't start up:
Testing that Sage starts... [20120905 05:12:26] Traceback (most recent call last): File "/release/buildbot/sage/sage1/sage_upgrade_4.5/build/sage5.4.beta1/local/bin/sageeval", line 4, in <module> from sage.all import * File "/release/buildbot/sage/sage1/sage_upgrade_4.5/build/sage5.4.beta1/local/lib/python2.7/sitepackages/sage/all.py", line 73, in <module> from sage.matrix.all import * File "/release/buildbot/sage/sage1/sage_upgrade_4.5/build/sage5.4.beta1/local/lib/python2.7/sitepackages/sage/matrix/all.py", line 1, in <module> from matrix_space import MatrixSpace, is_MatrixSpace File "/release/buildbot/sage/sage1/sage_upgrade_4.5/build/sage5.4.beta1/local/lib/python2.7/sitepackages/sage/matrix/matrix_space.py", line 33, in <module> import matrix File "matrix.pyx", line 1, in init sage.matrix.matrix (sage/matrix/matrix.c:2076) File "matrix2.pyx", line 1, in init sage.matrix.matrix2 (sage/matrix/matrix2.c:70164) File "matrix1.pyx", line 1, in init sage.matrix.matrix1 (sage/matrix/matrix1.c:13377) File "mutability.pxd", line 15, in init sage.matrix.matrix0 (sage/matrix/matrix0.c:29424) ValueError: PyCapsule_GetPointer called with invalid PyCapsule object Sage failed to start up.
Running ./sage baforce
solves the problem.
comment:34 Changed 7 years ago by
 Status changed from needs_review to needs_work
 Work issues set to upgrading
comment:35 Changed 6 years ago by
 Milestone changed from sage5.10 to sageduplicate/invalid/wontfix
 Reviewers changed from William Stein to Jeroen Demeyer
 Status changed from needs_work to positive_review
 Work issues upgrading deleted
Wontfix because of #14570 I assume?
comment:36 Changed 6 years ago by
I think that #14570 is orthogonal to this.
comment:37 Changed 6 years ago by
 Milestone changed from sageduplicate/invalid/wontfix to sage5.11
 Reviewers changed from Jeroen Demeyer to William Stein
 Status changed from positive_review to needs_work
comment:38 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:39 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:40 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:41 Changed 5 years ago by
As I mentionned on sagedevel, I recently noticed some preliminary work in src/setup.py toward compiling the Sage library in place, instead of putting the produced .so and copies of the .py files in src/build.
Compiling in place means saving 15s of sage b
each time I just
modify Python code, and I do that all the time; so we are speaking of
dozen of minutes per day. So I have been dreaming of it for
years. Besides, it would also remove one of the regular source of
confusions for our new contributors.
So I explored this further, and with very minimal further changes , this apparently seems to work smoothly: I haven't yet run all tests, but Sage starts, the documentation compiles, etc.
See branch u/nthiery/compile_library_inplace:
http://git.sagemath.org/sage.git/commit/?h=u/nthiery/compile_library_inplace
I can't wait until I can use this for real Sage development. But for this, I basically need to get this feature into Sage. It would be adventurous to enable this feature by default for all users at this point. On the other hand, we should allow the adventurous of us to use this extensively to chase out the bugs.
What would be the right approach to make this feature configurable when one compiles Sage for the first time? Using just an environment variable set by the user is not good, since a given user might have Sage installations configured differently. Some piece of information must be stored somewhere at configuration time.
I am happy to use a different ticket for this if this is deemed preferable.
Cheers,
Nicolas
comment:42 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:43 followup: ↓ 51 Changed 4 years ago by
This isn't *just* an issue of efficiency. Based on my experience, probably hundreds of potential sage developers have been confused by precisely the thing that just confused you. I remember Robert Miller running around at one Sage days going crazy because it seemed like everybody was massively confused by the fact there are multiple copies of arith.py (etc.). If I had any employees who could work on Sage, this would definitely be a good thing to put in a top 5 list of ticket priorities.
comment:44 Changed 4 years ago by
+1
comment:45 followup: ↓ 46 Changed 4 years ago by
I wouldn't like the clutter of .pyc
and .so
files in the source directory. If the source directory would be kept as clean as it currently is, +1. Otherwise, 1.
comment:46 in reply to: ↑ 45 ; followup: ↓ 47 Changed 4 years ago by
Replying to jdemeyer:
I wouldn't like the clutter of
.pyc
and.so
files in the source directory. If the source directory would be kept as clean as it currently is, +1. Otherwise, 1
How about making links to .py files in SAGELOCAL rather than copying them into SAGELOCAL ?
Why was copying chosen in the 1st place, I wonder...
comment:47 in reply to: ↑ 46 Changed 4 years ago by
comment:48 followups: ↓ 49 ↓ 50 Changed 4 years ago by
Copying wasn't chosen by us but by Python  it's just how setup.py install works.
The so and pyc files would definitely be in tree for this ticket and I believe this change would avoid tons of confusion. Being in tree is the entire point of being able to work on and run the source directly rather than copying It to a target location before build. Maybe jereon not wanting to run the source files directly is why this Ticket never got in? I don't know.
 sent from a phone
comment:49 in reply to: ↑ 48 ; followup: ↓ 52 Changed 4 years ago by
Replying to was:
The so and pyc files would definitely be in tree for this ticket and I believe this change would avoid tons of confusion.
What kind of confusion are you talking about? Perhaps the confusion can be fixed in a different way?
Maybe jereon not wanting to run the source files directly is why this Ticket never got in? I don't know.
No, it never got in Sage because it never actually worked.
comment:50 in reply to: ↑ 48 Changed 4 years ago by
Replying to was:
Copying wasn't chosen by us but by Python  it's just how setup.py install works.
well, what does prevent us to add an extra step of replacing copies with links? (This might not work well on native Windows, but we don't need to support this...)
This would combine the advantage of not having multiple copies of py files lying around (and not needing to sage b
every time you modify a .py file in Sage library), and
retaining .pyc and .sofree source directory.
The so and pyc files would definitely be in tree for this ticket and I believe this change would avoid tons of confusion.
comment:51 in reply to: ↑ 43 Changed 4 years ago by
Replying to was:
If I had any employees who could work on Sage, this would definitely be a good thing to put in a top 5 list of ticket priorities.
We will definitely consider this in ODK. Actually the main reason we did not put an explicit task about this is that I was (and still am) hoping it would be finished before that. Writing the grant distracted me away of working on it ...
Cheers,
Nicolas
comment:52 in reply to: ↑ 49 Changed 4 years ago by
Replying to jdemeyer:
Maybe jereon not wanting to run the source files directly is why this Ticket never got in? I don't know.
No, it never got in Sage because it never actually worked.
Well, see 41; it seemed pretty close from working. But of course the devil might be in the last bits to be polished.
comment:53 Changed 4 years ago by
I still don't understand what the problem is that this ticket is trying to fix.
Hey Mike, could you explain briefly why you delete (not move) all this code? Just curious.