build the sage library in place

[mwhansen]

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/sage-devel/browse_thread/thread/d5aef15acc4d178b

Depends on #13363

CC: @williamstein @kini @fchapoton @jhpalmieri

Component: build

Reviewer: John Palmieri

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

[williamstein]

comment:3

Hey Mike, could you explain briefly why you delete (not move) all this code? Just curious.

747	 	        # if cython worked, copy the file to the build directory 
748	 	        pyx_inst_file = '%s/%s'%(SITE_PACKAGES, f) 
749	 	        retval = os.system('cp %s %s 2>/dev/null'%(f, pyx_inst_file)) 
750	 	        # we could do this more elegantly -- load the files, use 
751	 	        # os.path.exists to check that they exist, etc. ... but the 
752	 	        # *vast* majority of the time, the copy just works. so this is 
753	 	        # just specializing for the most common use case. 
754	 	        if retval: 
755	 	            dirname, filename = os.path.split(pyx_inst_file) 
756	 	            try: 
757	 	                os.makedirs(dirname) 
758	 	            except OSError, e: 
759	 	                assert e.errno==errno.EEXIST, 'Cannot create %s.' % dirname 
760	 	            retval = os.system('cp %s %s 2>/dev/null'%(f, pyx_inst_file)) 
761	 	            if retval: 
762	 	                raise OSError, "cannot copy %s to %s"%(f,pyx_inst_file) 
763	 	        print "%s --> %s"%(f, pyx_inst_file) 
764	 	         
743	            return r         

[mwhansen]

comment:4

I removed it since we don't need to copy those files anymore -- they're already in the right place.

[williamstein]

comment:6

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.

[mwhansen]

comment:7

Attachment: trac_12659.patch.gz

Agreed -- I've put an updated patch.

[williamstein]

comment:8

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.

[mwhansen]

comment:9

Thanks!

[koffie]

comment:10

It seems to me that if this get's merged then we should undo the changes at #11235

[williamstein]

comment:11

I'm happy with this code.

And indeed the work at #11235 seems not necessary anymore.

[jdemeyer]

Reviewer: William Stein

[jdemeyer]

comment:13

Could this have caused

sage -t  "devel/sage-main/sage/graphs/graph_decompositions/rankwidth.pyx"
**********************************************************************
File "/tmp/jdemeyer/merger/sage-5.0.beta9/devel/sage-main/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/sage-5.0.beta9/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/tmp/jdemeyer/merger/sage-5.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/sage-5.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/sage-5.0.beta9/local/lib/python/site-packages/sage/graphs/graph.py", line 2959, in rank_decomposition
        from sage.graphs.graph_decompositions.rankwidth import rank_decomposition
    ImportError: cannot import name rank_decomposition
**********************************************************************

[jdemeyer]

comment:14

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?

[mwhansen]

comment:15

How did you get that particular error? How did you merge in the patch here?

[kcrisman]

comment:16

Dumbest question ever, I'm sure, but I'll ask it - will this affect upgrades at all?

[jdemeyer]

comment:17

Replying to @mwhansen:

How did you get that particular error? How did you merge in the patch here?

Apply the patch, sdist, build Sage from scratch.

[jdemeyer]

comment:18

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.

[mwhansen]

comment:19

Upgrades should be fine -- when sage -b gets run, the site-packages link will update to the new location, and the new extension modules should be built in place.

[kcrisman]

comment:20

Upgrades should be fine -- when sage -b gets run, the site-packages 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?

[mwhansen]

comment:21

Yes -- it does not try to copy existing extension modules over.

[jhpalmieri]

comment:22

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?

[nthiery]

comment:41

As I mentionned on sage-devel, 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:

https://github.com/sagemath/sagetrac-mirror/commits/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

[williamstein]

comment:43

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.

[kcrisman]

comment:44

+1

[jdemeyer]

comment:45

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.

[dimpase]

comment:46

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...

[jdemeyer]

comment:47

Replying to @dimpase:

Why was copying chosen in the 1st place, I wonder...

Portability?

[williamstein]

comment:48

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

[jdemeyer]

comment:49

Replying to @williamstein:

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.

[dimpase]

comment:50

Replying to @williamstein:

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 .so-free 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.

[nthiery]

comment:51

Replying to @williamstein:

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

[nthiery]

comment:52

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 [comment:41]; it seemed pretty close from working. But of course the devil might be in the last bits to be polished.

[jdemeyer]

comment:53

I still don't understand what the problem is that this ticket is trying to fix.

[mkoeppe]

Changed reviewer from William Stein to none

[mkoeppe]

Changed author from Mike Hansen to none

[mkoeppe]

comment:54

Let's close this ticket as outdated. For a new approach to in-place builds / editable installs, see #30371.

[jhpalmieri]

Reviewer: John Palmieri

[jhpalmieri]

comment:55

Okay.