Opened 11 years ago

Last modified 10 years ago

#12016 closed enhancement

parallelism in Sage: just use value of 'MAKE' — at Version 20

Reported by: jhpalmieri Owned by: GeorgSWeber
Priority: critical Milestone: sage-4.8
Component: build Keywords:
Cc: jdemeyer, leif Merged in:
Authors: John Palmieri, Jeroen Demeyer Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11969, #12096, #12098 Stopgaps:

Status badges

Description (last modified by jdemeyer)

With the attached patches, along with the changes from #11959, the various parallel aspects of Sage should be controlled by setting the -j flag in MAKE. That is, if MAKE='make -j16', then

  • running make will build spkg's in parallel, using 16 processes (this was done in #11959). This is standard make behaviour, no patches are needed.
  • running make ptestlong or sage -tp 0 <files> will doctest in parallel using 16 threads. If the -j flag in MAKE is not set, then determine the number of threads as before: min(8, cpu_count()).
  • running ./sage -b will build the Sage library using 16 threads. If the -j flag in MAKE is not set, then use only 1 thread.

In #6495, we should implement the same behavior for doc building.

Apply:

  1. 12016-root.patch to the SAGE_ROOT repository.
  2. 12016-base.patch to spkg/base.
  3. 12016-scripts.patch to the SCRIPTS repository.
  4. 12016-sage.patch to the Sage library.

Notes: With the patches applied, building spkgs in parallel works well, except for a "jobserver unavailable" warning in:

  • ntl
  • singular
  • rubiks

Change History (23)

comment:1 Changed 11 years ago by jdemeyer

We should remove NUM_THREADS from the top-level Makefile.

comment:2 Changed 11 years ago by jdemeyer

  • Authors changed from John Palmieri to John Palmieri, Jeroen Demeyer
  • Description modified (diff)

comment:3 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:6 follow-up: Changed 11 years ago by jdemeyer

  • Description modified (diff)

John, with your solution there is a lot of code duplication (determining the number of threads is done in 3 places, potentially in 3 different ways). How about having code in sage-sage or sage-env to determine the number of threads and saving it in an environment variable SAGE_NUM_PROCESSES (which the user could set by hand; if not set, the value comes from MAKE or MAKEFLAGS; if no -j option is given, set to 1).

comment:7 in reply to: ↑ 6 ; follow-up: Changed 11 years ago by jhpalmieri

Replying to jdemeyer:

John, with your solution there is a lot of code duplication (determining the number of threads is done in 3 places, potentially in 3 different ways). How about having code in sage-sage or sage-env to determine the number of threads and saving it in an environment variable SAGE_NUM_PROCESSES

Sounds okay.

(which the user could set by hand; if not set, the value comes from MAKE or MAKEFLAGS; if no -j option is given, set to 1).

If you run "sage -tp <files>", should you use 1 process or more than 1? The "-tp" option means "parallel", so perhaps the default should be more than 1 in this case. In other cases (like docbuilding, for example), the default should be 1.

comment:8 follow-up: Changed 11 years ago by jhpalmieri

For something like make -j16 ptestlong, how do we recover the number 16? If I execute this command (with MAKE unset), I see

MAKEFLAGS= --jobserver-fds=3,4 -j
MFLAGS=- --jobserver-fds=3,4 -j

but I don't see "16" anywhere in the listing of the environment variables.

comment:9 in reply to: ↑ 7 Changed 11 years ago by jdemeyer

Replying to jhpalmieri:

Replying to jdemeyer:

John, with your solution there is a lot of code duplication (determining the number of threads is done in 3 places, potentially in 3 different ways). How about having code in sage-sage or sage-env to determine the number of threads and saving it in an environment variable SAGE_NUM_PROCESSES

Sounds okay.

(which the user could set by hand; if not set, the value comes from MAKE or MAKEFLAGS; if no -j option is given, set to 1).

If you run "sage -tp <files>", should you use 1 process or more than 1? The "-tp" option means "parallel", so perhaps the default should be more than 1 in this case. In other cases (like docbuilding, for example), the default should be 1.

Sure, that is what I meant. We should compute the value once, but in sage -tp we can still decide to use the number of processes.

comment:10 in reply to: ↑ 8 Changed 11 years ago by jdemeyer

Replying to jhpalmieri:

For something like make -j16 ptestlong, how do we recover the number 16? If I execute this command (with MAKE unset), I see

MAKEFLAGS= --jobserver-fds=3,4 -j
MFLAGS=- --jobserver-fds=3,4 -j

but I don't see "16" anywhere in the listing of the environment variables.

You are right. I had not tried this before. So let's scrap that idea.

Changed 11 years ago by jhpalmieri

Changed 11 years ago by jhpalmieri

comment:11 Changed 11 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from new to needs_review

Here are new patches. These use SAGE_NUM_THREADS if it is set, and otherwise try to extract a number from MAKE. (My method for doing this is probably not ideal, but the options This is done in sage-env. Running sage -b should use this setting now, also.

I don't know how to get the number of threads from

make -j16 ptestlong

so I removed that from the "to do" list in the ticket description.

In the file sage-ptest, I removed the "FIXME" comment in

    try:
        # FIXME: Nice, but <NUMTHREADS> should immediately follow '-tp' etc.,
        #        i.e., be the next argument. We might have file or directory
        #        names that properly convert to an int...
        numthreads = int(argv[1])
        infiles = argv[2:]
    except ValueError: # can't convert first arg to an integer: arg was probably omitted
        numthreads = 1

The script sage-ptest doesn't get a "tp" argument; it is instead called by sage-sage, and the way it is called, the first argument to sage-ptest is precisely what ever came after "-tp". So I don't think anything needs fixing. If we ever rewrite sage-sage (#21) to properly parse arguments, we can make sure that "-tp" has a default numerical argument of zero.

Changed 11 years ago by jhpalmieri

comment:12 follow-up: Changed 11 years ago by jdemeyer

  • Priority changed from major to critical
  • Status changed from needs_review to needs_work

1) If you are going to use the string "auto" for automatic, you might as well use "infinite" for infinite, instead of zero.

1b) Alternatively: use 0 for automatic (as is sage -tp 0) and 999999 for unlimited. This would mean less special-case code, since a value like 999999 is more than what a user would normally specify (for the forseeable future).

2) In sage-ptest, unlimited really should be unlimited. Not max(8, # of cpus).

3) We should also do the following long-needed fix here: setting MAKE to make -j16 is very standard in Sage circles, but not actually the prefered way according to the GNU make folks. One really should use MAKEFLAGS instead (similar to the distinction between CC and CFLAGS). This is why you often see an error like "make -jN forced in sub-make. Disabling job server mode" (freely quoted from my mind). So, when MAKEFLAGS exists, assume that make understands the flags and do not pass flags in MAKE.

4) Why did you change

sage-build "$@" || exit $?

to

sage-build "$@"

in the sage_build() function in sage-sage?

5) You reverted a lot of changes that I made to doc/en/developer/doctesting.rst. Why? I actually tried all the examples in the documentation and pasted the exact output I got (on sage.math.washington.edu). Surely, this is better than keeping the outdated (and in many cases totally wrong) output.

I am planning to work further on this, so don't change any code yet. But please give your opinion.

comment:13 in reply to: ↑ 12 ; follow-ups: Changed 11 years ago by jhpalmieri

Replying to jdemeyer:

1) If you are going to use the string "auto" for automatic, you might as well use "infinite" for infinite, instead of zero.

1b) Alternatively: use 0 for automatic (as is sage -tp 0) and 999999 for unlimited. This would mean less special-case code, since a value like 999999 is more than what a user would normally specify (for the forseeable future).

Sounds good to me.

2) In sage-ptest, unlimited really should be unlimited. Not max(8, # of cpus).

Okay.

3) We should also do the following long-needed fix here: setting MAKE to make -j16 is very standard in Sage circles, but not actually the prefered way according to the GNU make folks. One really should use MAKEFLAGS instead (similar to the distinction between CC and CFLAGS). This is why you often see an error like "make -jN forced in sub-make. Disabling job server mode" (freely quoted from my mind). So, when MAKEFLAGS exists, assume that make understands the flags and do not pass flags in MAKE.

I'm willing to try that, especially if you write the patch instead of me :)

4) Why did you change

sage-build "$@" || exit $?

to

sage-build "$@"

in the sage_build() function in sage-sage?

That was a mistake.

5) You reverted a lot of changes that I made to doc/en/developer/doctesting.rst. Why? I actually tried all the examples in the documentation and pasted the exact output I got (on sage.math.washington.edu). Surely, this is better than keeping the outdated (and in many cases totally wrong) output.

Some of them I disagreed with, like the complete removal of the section "Beyond the Sage library". So I started from scratch, at which point I just put in the changes that I felt were relevant to the ticket or easy for me to change. Probably I should have started with your patch and added the section (with modifications) back in.

It looks like #9739 broke doctesting of .sage files. We should fix that (not on this ticket).

comment:14 in reply to: ↑ 13 Changed 11 years ago by jhpalmieri

Replying to jhpalmieri:

It looks like #9739 broke doctesting of .sage files. We should fix that (not on this ticket).

See #12069.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 11 years ago by jdemeyer

Replying to jhpalmieri:

Some of them I disagreed with, like the complete removal of the section "Beyond the Sage library".

I removed that because it totally didn't work. But this is probably #12069. How about we leave the last section of the documentation alone in this ticket but then change the documentation in #12069?

comment:16 in reply to: ↑ 15 Changed 11 years ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

Some of them I disagreed with, like the complete removal of the section "Beyond the Sage library".

I removed that because it totally didn't work. But this is probably #12069. How about we leave the last section of the documentation alone in this ticket but then change the documentation in #12069?

Okay, sounds fine to me.

comment:17 Changed 11 years ago by jdemeyer

  • Dependencies changed from #11969 to #11969, #12096
  • Description modified (diff)

The essence of the patch should be there. I'm not claiming it works, I still need to test many things.

comment:18 Changed 11 years ago by jdemeyer

  • Dependencies changed from #11969, #12096 to #11969, #12096, #12098

comment:19 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:20 Changed 11 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.