Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#4480 closed defect (fixed)

[with new patch, positive review] cython dependancy checking is too slow

Reported by: robertwb Owned by: craigcitro
Priority: blocker Milestone: sage-3.2
Component: build Keywords:
Cc: tornaria Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The attached patch builds the entire dependency tree from scratch in 0.6 seconds, and caches it to disk so subsequent dependency checking takes 0.05 seconds to verify.

Attachments (4)

4480-cython-deps.patch (14.8 KB) - added by robertwb 12 years ago.
trac-4480-rc0.patch (108.5 KB) - added by craigcitro 12 years ago.
trac-4480-cumulative.patch (108.7 KB) - added by craigcitro 12 years ago.
trac-4480-pt18.patch (3.0 KB) - added by craigcitro 12 years ago.
okay, maybe it only feels like part 18. :)

Download all attachments as: .zip

Change History (17)

Changed 12 years ago by robertwb

comment:1 Changed 12 years ago by craigcitro

  • Summary changed from [with patch, needs review] cython dependancy checking is too slow to [with patch, under review] cython dependancy checking is too slow

So Gonzalo and I are in the middle of writing the build process in setup.py. Luckily, the business of finding includes is the one piece of code we were reusing! So I'll review/merge this into our patch once we're done, which should be today.

Changed 12 years ago by craigcitro

comment:2 Changed 12 years ago by craigcitro

  • Cc tornaria added
  • Owner changed from mabshoff to craigcitro
  • Status changed from new to assigned
  • Summary changed from [with patch, under review] cython dependancy checking is too slow to [with new patch, needs review] cython dependancy checking is too slow

Here we go! So this patch applies on top of rc0 + the patch at #4500, because it was a needed fix. With this patch in, we've got it all: fast cached dependency checking, parallel calls to Cython, and dozens of small improvements to the build system thrown in, too. I also reorganized setup.py and created a module_list.py to have the list of extension modules.

Robert, I've looked at (probably every line of) your code which is now in here -- can you review the rest? I did make small changes to one or two of your functions; in particular, parse_deps, I think.

I've run it through some paces, but I'm happy to fix any bugs that pop up.

Credit should go to Rob, Gonzalo, and me.

comment:3 Changed 12 years ago by craigcitro

  • Milestone changed from sage-3.2.1 to sage-3.2
  • Priority changed from major to blocker

comment:4 Changed 12 years ago by mabshoff

I am giving a positive review to all the changes that move the extensions to the new file. But we need a formal review for Robert's changes and since Craig was involved here, too, I am not so sure who would review what portion of those.

I am testing a vanilla tree with the fixes here and #4500 applied, so I should be able to at least see if everything works as expected when combining all pieces into a virgin build.

Thoughts?

Cheers,

Michael

comment:5 Changed 12 years ago by robertwb

I'll look at this today. I only apply the last patch, right?

comment:6 Changed 12 years ago by mabshoff

Yes, but you should also apply

http://trac.sagemath.org/sage_trac/attachment/ticket/4500/trac-4500.patch

in this context. I found an issue when building from vanilla with both the last patch here and trac-4500.patch applied and there is an issue that I mentioned on the other ticket with a work around fix, but I am not sure if it is the correct fix. It does work, but that does not mean that it is either elegant or proper.

Cheers,

Michael

comment:7 Changed 12 years ago by robertwb

  • Summary changed from [with new patch, needs review] cython dependancy checking is too slow to [with new patch, positive review] cython dependancy checking is too slow

It looks good to me...this is a much needed cleanup!

I think mabshoff's fix proposed #4500 is good, and should also be applied.

comment:8 Changed 12 years ago by craigcitro

Ditto on mabshoff's fix at #4500.

comment:9 Changed 12 years ago by craigcitro

In addition to mabshoff's fix at #4500, I think we should add the following:

diff -r c543000d6447 setup.py
--- a/setup.py  Thu Nov 13 05:32:07 2008 -0800
+++ b/setup.py  Thu Nov 13 16:26:41 2008 -0800
@@ -13,11 +13,11 @@
 else:
     sdist = False
 
-# uncomment to turn warnings off
-# import distutils.sysconfig
-# NO_WARN = True
-# if NO_WARN and distutils.sysconfig.get_config_var('CC').startswith("gcc"):
-#     extra_compile_args = ['-w']
+# comment these four lines out to turn on warnings from gcc
+import distutils.sysconfig
+NO_WARN = True
+if NO_WARN and distutils.sysconfig.get_config_var('CC').startswith("gcc"):
+    extra_compile_args = ['-w']
 
 if not os.environ.has_key('SAGE_ROOT'):
     print "    ERROR: The environment variable SAGE_ROOT must be defined."

This just turns warnings back off -- as William points out, it's a lot of output for the unsuspecting. Michael and I had discussed this when we changed it, and it would be good to sit down and actually look at the warnings once to see if anything interesting is being turned up. In the interim, though, let's not spam. ;)

comment:10 Changed 12 years ago by robertwb

I'm OK with that.

comment:11 Changed 12 years ago by mabshoff

Can somebody post a cumulative patch here?

Cheers,

Michael

Changed 12 years ago by craigcitro

comment:12 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged trac-4480-cumulative.patch in Sage 3.2.rc1

Changed 12 years ago by craigcitro

okay, maybe it only feels like part 18. :)

comment:13 Changed 12 years ago by mabshoff

Merged trac-4480-pt18.patch in Sage 3.2.rc1.

Thanks Craig.

Cheers,

Michael

Note: See TracTickets for help on using tickets.