Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the necessity to set SAGE_PARALLEL_SPKG_BUILD #11959

Closed
jhpalmieri opened this issue Oct 27, 2011 · 53 comments
Closed

Remove the necessity to set SAGE_PARALLEL_SPKG_BUILD #11959

jhpalmieri opened this issue Oct 27, 2011 · 53 comments

Comments

@jhpalmieri
Copy link
Member

The patches on this ticket remove necessity to set the environment variable SAGE_PARALLEL_SPKG_BUILD to turn parallel spkg building on. Thus it will be on by default, but note that "parallel spkg building" only has an effect if MAKE is set to, for example, "make -j16".

In the current implementation, parallel building can be disabled by either setting the variable to "no" or by running "make build-serial".

This change was discussed in sage-devel.

Apply:

Depends on #11926

CC: @nexttime @kcrisman

Component: build

Keywords: SAGE_PARALLEL_SPKG_BUILD MAKE -j --jobs

Author: John Palmieri

Reviewer: Leif Leonhardy

Merged: sage-4.8.alpha0

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

@jhpalmieri

This comment has been minimized.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 27, 2011

Changed keywords from none to SAGE_PARALLEL_SPKG_BUILD MAKE -j --jobs

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 27, 2011

comment:2

Objections to renaming the ticket to "Remove the necessity to set SAGE_PARALLEL_SPKG_BUILD"? (It's yours, so I didn't change its title. ;-) )

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 27, 2011

comment:3

John, you confused the names of the patches. :)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 27, 2011

comment:4

My alternate patch to $SAGE_ROOT/spkg/install has become a bit longer:

diff --git a/spkg/install b/spkg/install
--- a/spkg/install
+++ b/spkg/install
@@ -430,12 +430,10 @@
 
 
 # Skip the rest if nothing to do (i.e., to [re]build):
-# Note that we should use $MAKE here, but we currently don't use
-# it further below either unless SAGE_PARALLEL_SPKG_BUILD=yes, which
-# I consider a "bug". -- Leif Leonhardy
-# If "make" doesn't understand the -q option, it should exit with a
-# non-zero status which is not a problem.
-if make -q -f standard/deps $1; then
+# If "make" doesn't understand the -q option (although we require
+# GNU make, which supports it), it should exit with a non-zero status
+# which is not a problem.
+if ${MAKE:-make} -q -f standard/deps $1; then
     echo "Nothing to (re)build / all up-to-date."
     exit 0
 fi
@@ -443,48 +441,57 @@
 
 # Dump environment for debugging purposes:
 echo "*** ALL ENVIRONMENT VARIABLES BEFORE BUILD: ***"
-env
+env | sort
 echo "***********************************************"
 
 
 ###############################################################################
 # NOW do the actual build:
 ###############################################################################
-if [ "$SAGE_PARALLEL_SPKG_BUILD" = "yes" ] && [ -n "$MAKE" ]; then
-    time $MAKE -f standard/deps $1
-else
-    time make -f standard/deps $1
-fi
+time ${MAKE:-make} -f standard/deps $1
 
-# added by Burcin Erocal, see trac #6295.
+# Added by Burcin Erocal, see trac #6295:
 if [ $? -ne 0 ]; then
-    echo "Error building Sage."
+    echo >&2 "Error building Sage."
     exit 1
 fi
 
+# Rename makefile to Makefile (see #10156):
 cd "$SAGE_ROOT"
-
-# Rename makefile to Makefile (see #10156)
 if [ -f makefile ]; then
-    mv makefile Makefile
-    if [ $? -ne 0 ]; then
-        echo "Error renaming 'makefile' to 'Makefile'."
-        exit 1
+    if [ ! -f Makefile ]; then
+        if ! mv makefile Makefile; then
+            echo >&2 "Error renaming 'makefile' to 'Makefile'."
+            exit 1
+        fi
+    else
+        # Both 'makefile' and 'Makefile' exist.
+        echo >&2 "Warning: Deleting old 'makefile'; 'Makefile' remains."
+        if ! rm -f makefile; then
+            echo >&2 "Error deleting old 'makefile'."
+            exit 1
+        fi
     fi
 fi
 
-# build succeeded
-if [ "$1" = "all" ]; then
-    echo "To install gap, gp, singular, etc., scripts"
-    echo "in a standard bin directory, start sage and"
-    echo "type something like"
-    echo "   sage: install_scripts('/usr/local/bin')"
-    echo "at the Sage command prompt."
-    echo ""
-    echo "To build the documentation, run"
-    echo "   make doc"
-    echo ""
-fi
-
+# Build succeeded.
 echo "Sage build/upgrade complete!"
 
+if [ "$1" = "all" ]; then
+    echo
+    echo "To install small scripts to directly run Sage's versions of GAP,"
+    echo "the PARI/GP interpreter, Maxima, or Singular etc. (by typing e.g."
+    echo "just 'gap' or 'gp') into a standard 'bin' directory, start Sage"
+    echo "by typing 'sage' (or './sage') and enter something like"
+    echo
+    echo "    install_scripts('/usr/local/bin')"
+    echo
+    echo "at the Sage command prompt ('sage:')."
+    echo
+    echo "To (re)build the HTML documentation, run 'make doc' (if you haven't"
+    echo "issued 'make' but just 'make build' or e.g. 'sage --upgrade'; other-"
+    echo "wise it will be built automatically right now)."
+    echo
+    echo "To build the PDF version of the documentation, run 'make doc-pdf'."
+    echo
+fi

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:5

W.r.t. README.txt:

I'd quote make ('make'), and directly refer to the GNU make (online) manual, i.e., its Options summary section (which IMHO explains -j and -l etc. quite good):

http://www.gnu.org/software/make/manual/make.html#Options-Summary

s/so for example if you set/so for example if you do/ (or ... type)

Perhaps also add that it's usually ok (or advantageous) to use more jobs than the machine has CPU cores (minus already running processes, or the current system load), especially if disk access isn't very fast [provided the machine isn't low of available memory or actually RAM].

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:6

W.r.t. the Sage Installation Guide:

export MAKE='make -jNUM' is not "a common setting of MAKE", but a shell command to set MAKE; 'make -jNUM' is the setting. (This has been in before.)

s/won't need to use/won't need to set/ (dito)

s/can speed up the process/can significantly speed up the process/

I'd omit "(Not all packages support this, but the ones for which this could be problematic should automatically disable it.)" since this is pretty irrelevant (or confusing); or rephrase it to say that a few packages do not support building them with multiple jobs and hence are built sequentially, although in parallel with other packages.

In the warning, substitute one-processor by single-core CPU or single-core machine. I'd also mention the platform (or operating system) to which this apparently applies; in my experience it is a non-issue on Linux. Attempting to build Sage with probably too many jobs should in general not hurt; one can simply reduce the number of jobs upon build errors and restart, or more precisely continue, the build afterwards by again typing make.

Similar what I've said w.r.t. README.txt applies to the Installation Guide.

Also we should somehow mention potential problems with ATLAS there, and perhaps what I said about self-tuning packages in general (on sage-devel). (Again, if ATLAS failed to build -- often just due to too high system load -- one can simply type make again, perhaps multiple times, since there are usually less packages left to build when make finally exits. If we haven't already, we should also mention that CPU throttling aka power-saving mode should be turned off; "on-demand" in contrast should be ok, i.e., provided it works properly. [I don't know whether that's in the README.txt; I think it is important enough that it should be there, too.])

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:7

P.S.:

I think (e.g.) MAKE="make -j3 -l10.0" or MAKE="make -j 3 -l 10.0" (or MAKE="make --jobs=3 --load-average=10.0") is more natural.

(Although AFAIK older versions of GNU make interpreted make -j 3 as "build target(!) '3' with infinitely many(!) jobs", so the first and last version may be safer.)

@williamstein
Copy link
Contributor

comment:8

I'm against this as is, but I think something very close is OK.

I think SAGE_PARALLEL_SPKG_BUILD="no" should be supported, but otherwise everything should be like in this proposal. The reason is that it can be confusing trying to deal with build issues when the build log mixes up output from multiple packages being built at once. Also there can be weird build issues resulting from building multiple packages at once. However, using "make -j16" (say) is very useful in all cases on a machine with lots of processors so you don't have to wait so long for individual packages to build.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:9

Replying to @williamstein:

I'm against this as is, but I think something very close is OK.

I think SAGE_PARALLEL_SPKG_BUILD="no" should be supported, but otherwise everything should be like in this proposal. The reason is that it can be confusing trying to deal with build issues when the build log mixes up output from multiple packages being built at once.

Therefore we have (in addition) individual logs for each spkg in $SAGE_ROOT/spkg/logs.

Also there can be weird build issues resulting from building multiple packages at once. However, using "make -j16" (say) is very useful in all cases on a machine with lots of processors so you don't have to wait so long for individual packages to build.

Well, building multiple spkgs in parallel is IMHO quite mature.

If you only need an equivalent of SAGE_PARALLEL_SPKG_BUILD=no (still building each spkg in parallel if it supports that, and of course MAKE is set to make -jN) for debugging,

$ echo ".NOTPARALLEL" >> $SAGE_ROOT/spkg/standard/deps

does the job.

We could of course document that, perhaps better in the Developer's Guide.

(Or provide some [other] way to enable and disable that feature -- by an environment variable or an easier command; the only odd thing is that deps is under revision control and hence you get uncommitted changes in the Sage root repository :-) whenever that file is modified. As an alternative, we could include some other "temporary" or volatile [make]file which is in .hgignore, and is either empty or contains just .NOTPARALLEL.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:10

Replying to @nexttime:

(Or provide some [other] way to enable and disable that feature -- by an environment variable or an easier command; the only odd thing is that deps is under revision control and hence you get uncommitted changes in the Sage root repository :-) whenever that file is modified. As an alternative, we could include some other "temporary" or volatile [make]file which is in .hgignore, and is either empty or contains just .NOTPARALLEL.)

We could easily achieve that by changing spkg/install to

...

if [[ ${SAGE_PARALLEL_SPKG_BUILD:-yes} = no ]]; then
    # Disable just building multiple packages at the same time; individual
    # spkgs still get built in parallel if they support it and '--jobs'
    # in $MAKE is greater than one (and at all specified):
    echo ".NOTPARALLEL:" > "$SAGE_ROOT"/spkg/standard/parallel_make.cfg
else
    # Create an (almost) empty file, s.t. including it from spkg/standard/deps
    # doesn't raise an error, thereby also invalidating any previous setting:
    echo > "$SAGE_ROOT"/spkg/standard/parallel_make.cfg
fi

time ${MAKE:-make} -f standard/deps $1

...

adding spkg/standard/parallel_make.cfg to $SAGE_ROOT/.hgignore, and adding the line

include parallel_make.cfg

to spkg/standard/deps.

@jhpalmieri
Copy link
Member Author

comment:11

How about a "make serial" target instead of the environment variable?

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 28, 2011

comment:12

Replying to @jhpalmieri:

How about a "make serial" target instead of the environment variable?

I'd say make build-serial, which just sets SAGE_PARALLEL_SPKG_BUILD=no and then does the usual thing, namely build.

Just like make rebuild sets SAGE_UPGRADING=yes and then "calls" build. ;-)

You can of course define some aliases for convenience, e.g. serial-build or build-macosx... XD

@jhpalmieri
Copy link
Member Author

comment:13

Building in parallel on Mac OS X works just fine in my experience. :p

Anyway, here are new versions of the patches.

@jhpalmieri jhpalmieri changed the title build spkgs in parallel by default Remove the necessity to set SAGE_PARALLEL_SPKG_BUILD Oct 28, 2011
@jhpalmieri
Copy link
Member Author

comment:15

Some comments about the new versions:

  • I've added a "build-serial" target in the Makefile.
  • spkg/install writes "parallel-make.cfg" in the directory "spkg", since that seems to be the current directory when the makefile "standard/deps" is executed.
  • Leif's proposed change to spkg/install regarding moving "makefile" to "Makefile" didn't seem to work on OS X, probably because of the stupid way OS X deals with upper and lower case in their standard file system. Or maybe I did something else wrong. Anyway, I didn't change that part of the script.
  • I haven't put everything possible into the top-level README file: the installation guide is supposed to have all of the details. I did add a comment about the spkg/logs/ files. See also "make" should run Sage once #11926, which adds a message about spkg/logs/PKG.log to the error message printed by sage-spkg.
  • in the installation guide, I added a few comments about problems building ATLAS.

By the way, is there anything wrong with this kind of change:

diff --git a/spkg/install b/spkg/install
--- a/spkg/install
+++ b/spkg/install
@@ -483,19 +483,19 @@ fi
 echo "Sage build/upgrade complete!"
 
 if [ "$1" = "all" ]; then
-    echo
-    echo "To install small scripts to directly run Sage's versions of GAP,"
-    echo "the PARI/GP interpreter, Maxima, or Singular etc. (by typing e.g."
-    echo "just 'gap' or 'gp') into a standard 'bin' directory, start Sage"
-    echo "by typing 'sage' (or './sage') and enter something like"
-    echo
-    echo "    install_scripts('/usr/local/bin')"
-    echo
-    echo "at the Sage command prompt ('sage:')."
-    echo
-    echo "If you issued 'make', 'make all', or a similar command, then the"
-    echo "HTML version of the documentation will be built right now."
-    echo "Otherwise, if you want to (re)build the HTML documentation,"
-    echo "run 'make doc'.  To build the PDF version, run 'make doc-pdf'."
-    echo
+    echo "
+To install small scripts to directly run Sage's versions of GAP,
+the PARI/GP interpreter, Maxima, or Singular etc. (by typing e.g.
+just 'gap' or 'gp') into a standard 'bin' directory, start Sage
+by typing 'sage' (or './sage') and enter something like
+
+    install_scripts('/usr/local/bin')
+
+at the Sage command prompt ('sage:').
+
+If you issued 'make', 'make all', or a similar command, then the
+HTML version of the documentation will be built right now.
+Otherwise, if you want to (re)build the HTML documentation,
+run 'make doc'.  To build the PDF version, run 'make doc-pdf'.
+"
 fi

Are multiline strings not a good idea for "echo"? If this sort of thing is okay, it makes it much easier to edit the message.

@jhpalmieri
Copy link
Member Author

Attachment: trac_11959-root.patch.gz

root repo

@jhpalmieri

This comment has been minimized.

@jhpalmieri
Copy link
Member Author

Sage library

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

comment:17

Attachment: trac_11959-sage.patch.gz

Replying to @jhpalmieri:

Building in parallel on Mac OS X works just fine in my experience. :p

Incidentally I ran into "random" (non-reproducible) parallel build errors on bsd.math just yesterday, when it was btw. otherwise idling; with MAKE="make -j8".

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

comment:18

Replying to @jhpalmieri:

Are multiline strings not a good idea for "echo"? If this sort of thing is okay, it makes it much easier to edit the message.

echo has no problem with that, nor any reasonable shell. For larger texts, you might get problems with the size of the environment on some systems though. Note that

$ echo "
one line
"

outputs three lines, the first and last being empty.

I'd prefer cat and "here" documents though (if at all). If you want to focus on the code rather than the text, it's better to use a chain of (properly indented) echo commands.

bash also supports indentation of "here" documents (that doesn't appear in the output);

$ cat <<-EOF
...
EOF

strips leading tabs from the text.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

Reviewer: Leif Leonhardy

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

comment:19

make doesn't export "its" variables by default. So unless SAGE_PARALLEL_SPKG_BUILD was already defined (and exported) when make was invoked, setting it in the Makefile has no effect on programs (including shells) invoked by make. Without using further GNU make specifics, you'd have to do e.g.

build: $(PIPE) 
        cd spkg && \
        "../$(PIPE)" \
                 "env SAGE_PARALLEL_SPKG_BUILD='$(SAGE_PARALLEL_SPKG_BUILD)' ./install all 2>&1" \
                 "tee -a ../install.log" 

I <3 lengthy env var names...

It would be better (i.e., safer) to use an absolute filename in the include directive, e.g. $(SAGE_ROOT)/spkg/parallel_make.cfg.

I'd perhaps add to the comment there that .NOTPARALLEL has no effect on sub-makes.

While you're at it, the commit messages are no longer current.

@nexttime nexttime mannequin removed the s: needs review label Oct 29, 2011
@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

comment:28

I'm happy with the v2 patches as they are, so in principle positive review. Just have to take a closer look at the whole and make sure the patches apply... ;-)

Feel free to add or change further things if you like.

[IRC distracted me; that was a while ago, but I hadn't yet grabbed the patches... :-) ]

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 29, 2011

comment:29

"If you have a machine with 4 processors..."

Ask Intel what the differences between a uni-processor, a dual-processor and a multi-processor machine are... ;-) (These are separate Xeon series; AMD has Athlon/Phenom vs. Opteron. The Sage cluster machines boxen, geom, mod, redhawk and sage are all 4-processor machines, each processor a hexacore.)

(The cheap desktop and notebook ones are all single-processor multi-core machines, and I think the notion of cores is meanwhile quite common to everyone.)

If you happen to edit README.txt again, perhaps also mention sage-release, which is IMHO better suited for build errors. The lengthy standard error message of sage-spkg already mentions sage-devel (and currently only that).

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 30, 2011

comment:30

Small typo in deps:

"# only determines whether more than one spkg may be built at a time."

Or I should just try what it builds when it builds more than spkgs... :-)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 30, 2011

comment:31

Replying to @nexttime:

If you happen to edit README.txt again [...]

You could also increase the number of developers in "Over 200 people have contributed code to Sage." to 300+.

@kcrisman
Copy link
Member

comment:32

Just in case this isn't written in stone, it would be nice to still have the full thing in install.log. One could add something about referring to the other logs, though, if it doesn't already do so, maybe especially in the case of an error in the build.

@jhpalmieri
Copy link
Member Author

comment:33

I'm curious as to how the full install.log is useful. Having it essentially doubles the amount of log space, since almost everything is in both the install.log and the individual log files. The individual log files aren't jumbled together the way they are in install.log (for parallel builds). If the whole log file just said:

  • building X
  • building Y
  • ...
  • done building Y (or "error building Y")
  • done building X
  • ...

with instructions about how to find the individual log files, doesn't that convey all of the necessary information?

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 30, 2011

comment:34

It's amazing how many typos are in the Installation Guide. But the documentation has really improved compared to how it was e.g. last year.

One could also add a link to the Environment Variables section (i.e., MAKE) when it comes to "make", since the steps for building from source there currently do not mention building in parallel (or make -j) at all.


As said, I'm happy with the patches as they are, and everything works as expected, so positive review.

@jhpalmieri
Copy link
Member Author

comment:35

FYI: I'm updating the root patch to fix the typo in deps.

@jhpalmieri
Copy link
Member Author

Attachment: trac_11959-root.v2.patch.gz

root repo

@kcrisman
Copy link
Member

comment:36

Replying to @jhpalmieri:

I'm curious as to how the full install.log is useful. Having it essentially doubles the amount of log space, since almost

Because I'm lazy and don't want to have to dive into spkg/ to open them. Won't revert the positive review on this, though, since it will be in 4.7.3, certainly makes sense in many ways, and if anyone else wants it they'll have plenty of alphas to complain :)

@jhpalmieri
Copy link
Member Author

comment:37

Replying to @kcrisman:

Replying to @jhpalmieri:

I'm curious as to how the full install.log is useful. Having it essentially doubles the amount of log space, since almost

Because I'm lazy and don't want to have to dive into spkg/ to open them. Won't revert the positive review on this

This ticket doesn't actually affect the contents of install.log, it was just a side discussion. The patches here do put a little more emphasis on spkg/logs/*, because I think they still aren't well known enough, but no log files were harmed by the patches for this ticket.

@jdemeyer
Copy link

jdemeyer commented Nov 1, 2011

Rebased on top of #11926

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Nov 1, 2011

Dependencies: #11926

@jdemeyer
Copy link

jdemeyer commented Nov 1, 2011

comment:38

Attachment: trac_11959-root.v2-rebased.patch.gz

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2011

Merged: sage-4.7.3.alpha0

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2011

Rebased on top of #11926

@jdemeyer
Copy link

jdemeyer commented Nov 3, 2011

Attachment: trac_11959-sage.v2-rebased.patch.gz

Milestone sage-4.7.3 deleted

@jdemeyer jdemeyer removed this from the sage-4.8 milestone Nov 3, 2011
@jdemeyer
Copy link

jdemeyer commented Nov 3, 2011

Changed merged from sage-4.7.3.alpha0 to sage-4.8.alpha0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants