Simplified handling of parallel jobs during builds by alalazo · Pull Request #11524 · spack/spack

alalazo · 2019-05-21T11:53:30Z

This PR implements the refactoring requests in #11373 (comment), in particular:

Config scopes are used to handle builtin defaults, command line overrides and package overrides (parallel=False)
Package.make_jobs attribute has been removed
The use of the argument -j has been rationalized across commands
Add unit tests to check that setting parallel jobs works as expected
Fix packages that used Package.make_jobs (i.e. bazel)

`spack log-parse` still has its own argument, as it seems to have peculiar ways to set the default value

Apparently the config system has a number of well defined config scopes that can be used. To fix an error during smoke builds on Travis the 'package' and 'builtin' scope have both been changed to '_builtin'

citibeth

I like this! A couple of notes:

Looking over the code briefly, I'm not 100% sure how the level of parallelism is set, overridden, determined, etc; but it looks well thought out and should be a big improvement compared to what currently feels like a hack. I'm assuming docs will be added before merge?
(Out of scope): it would be great if someday there could be a way to set config parameters directly from the command line. Because right now it seems that command lines and config scopes are two parallel-but-different ways to change Spack's behavior.

lib/spack/spack/build_environment.py

adamjstewart

A lot of packages rely on Package.make_jobs:

$ grep make_jobs */package.py
bazel/package.py:        elif dependent_spec.package.make_jobs:
bazel/package.py:            jobs = dependent_spec.package.make_jobs
boost/package.py:        jobs = make_jobs
cbench/package.py:        build_env.set('JOBS', str(make_jobs))
charmpp/package.py:            "-j%d" % make_jobs,
cmake/package.py:            '--parallel={0}'.format(make_jobs)
foam-extend/package.py:            os.environ['WM_NCOMPPROCS'] = str(make_jobs)
intel-xed/package.py:        args = ['-j', str(make_jobs),
kahip/package.py:            filter_file('NCORES=.*', 'NCORES={0}'.format(make_jobs), f)
ninja-fortran/package.py:        ninja('-j{0}'.format(make_jobs), 'ninja_test')
ninja/package.py:        ninja('-j{0}'.format(make_jobs), 'ninja_test')
of-adios-write/package.py:            os.environ['WM_NCOMPPROCS'] = str(make_jobs)
of-precice/package.py:            os.environ['WM_NCOMPPROCS'] = str(make_jobs)
openfoam-com/package.py:            args.append('-j{0}'.format(make_jobs))
openfoam-org/package.py:            os.environ['WM_NCOMPPROCS'] = str(make_jobs)
petsc/package.py:        make('MAKE_NP=%s' % make_jobs, parallel=False)
py-numpy/package.py:                args = ['-j', str(make_jobs)]
py-pyside/package.py:        return ['--jobs={0}'.format(make_jobs)]
py-pytorch/package.py:        build_env.set('MAX_JOBS', make_jobs)
py-scipy/package.py:            args.extend(['-j', str(make_jobs)])
py-shiboken/package.py:        return ['--jobs={0}'.format(make_jobs)]
qt/package.py:        spack_env.set('MAKEFLAGS', '-j{0}'.format(make_jobs))
r/package.py:        # Use the number of make_jobs set in spack. The make program will
r/package.py:        spack_env.set('MAKEFLAGS', '-j{0}'.format(make_jobs))
slepc/package.py:        make('MAKE_NP=%s' % make_jobs, parallel=False)
xios/package.py:                   '--job', str(make_jobs)]

I just added another in #11494. I'm fine with removing it as long as there is some other way to access this info inside of a package.

alalazo · 2019-05-21T16:05:00Z

@adamjstewart

A lot of packages rely on Package.make_jobs: [...]

Apparently I missed only bazel. All the others rely on make_jobs defined at the module level, which is set here:

spack/lib/spack/spack/build_environment.py

Line 418 in 75607eb

m.make_jobs = jobs

module scope != package scope

lib/spack/spack/cmd/common/arguments.py

lib/spack/spack/build_environment.py

adamjstewart · 2019-05-21T20:56:16Z

This doesn't necessarily need to be in the same PR, but something I've always wanted to do is replace MakeExecutable with ParallelExecutable and replace make_jobs with jobs. There are a lot of things in Spack that accept -j16 aside from just make.

codecov · 2019-05-21T21:50:43Z

Codecov Report

Merging #11524 into develop will increase coverage by 0.03%.
The diff coverage is 31.25%.

@@             Coverage Diff             @@
##           develop   #11524      +/-   ##
===========================================
+ Coverage    74.96%      75%   +0.03%     
===========================================
  Files          213      213              
  Lines        23351    23352       +1     
  Branches      4578     4576       -2     
===========================================
+ Hits         17505    17514       +9     
  Misses        4726     4726              
+ Partials      1120     1112       -8

Flag	Coverage Δ
#unitlinux	`74.67% <31.25%> (ø)`	⬆️
#unitosx	`73.94% <31.25%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
lib/spack/spack/cmd/diy.py	`50% <ø> (+2.94%)`	⬆️
lib/spack/spack/cmd/install.py	`80.76% <ø> (+1.52%)`	⬆️
lib/spack/spack/package.py	`62.79% <ø> (-0.11%)`	⬇️
lib/spack/spack/cmd/bootstrap.py	`57.14% <100%> (ø)`	⬆️
lib/spack/spack/cmd/common/arguments.py	`77.96% <23.07%> (-15.66%)`	⬇️
lib/spack/spack/build_environment.py	`71.8% <50%> (+0.31%)`	⬆️
lib/spack/llnl/util/tty/__init__.py	`50.23% <0%> (+0.93%)`	⬆️
lib/spack/spack/util/log_parse.py	`87.27% <0%> (+1.81%)`	⬆️
lib/spack/llnl/util/tty/colify.py	`67.52% <0%> (+5.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f828ce...0f99333. Read the comment docs.

alalazo · 2019-05-22T09:05:05Z

This doesn't necessarily need to be in the same PR, but something I've always wanted to do is replace MakeExecutable with ParallelExecutable and replace make_jobs with jobs. There are a lot of things in Spack that accept -j16 aside from just make.

Completely agree with that. @tgamblin Let me know if you want me to do the refactoring above here, or if we should leave it for another PR.

Also: I left spack log-parse untouched as it had some specific logic to decide the default value for the parallelism, but if you think it's worth it we can also try to standardize that use of -j.

lib/spack/spack/cmd/common/arguments.py

tgamblin

looks great to me, modulo the two comments above! Thanks!

If jobs gets passed to the action it's already a valid integer. Once we ruled out that the number is not less than 1 we are ensured it's a good number to set in command line scope

alalazo · 2019-05-24T18:21:50Z

Modifications done let me know if anything else is needed.

tgamblin · 2019-05-24T18:48:59Z

Thanks @alalazo!

This PR implements several refactors requested in spack#11373, specifically: - Config scopes are used to handle builtin defaults, command line overrides and package overrides (`parallel=False`) - `Package.make_jobs` attribute has been removed; `make_jobs` remains as a module-scope variable in the build environment. - The use of the argument `-j` has been rationalized across commands - move '-j'/'--jobs' argument into `spack.cmd.common.arguments` - Add unit tests to check that setting parallel jobs works as expected - add new test to ensure that build job setting is isolated to each build - Fix packages that used `Package.make_jobs` (i.e. `bazel`)

alalazo added 5 commits May 21, 2019 10:24

Use config scopes to handle the parallel=False attribute in packages

b1f4bd3

Created a custom argparse action to set the number of parallel jobs

7b5e72f

Removed Package.make_jobs and make_jobs argument of do_install

f526e92

Removed duplicate instance of the argument 'jobs'

1e9a485

Rationalized the use of the '-j' argument across commands

54db478

`spack log-parse` still has its own argument, as it seems to have peculiar ways to set the default value

alalazo added build-environment refactoring labels May 21, 2019

alalazo requested review from becker33 and tgamblin May 21, 2019 11:53

alalazo mentioned this pull request May 21, 2019

Cap the maximum number of build jobs #11373

Merged

Fixed the name of config scopes

75607eb

Apparently the config system has a number of well defined config scopes that can be used. To fix an error during smoke builds on Travis the 'package' and 'builtin' scope have both been changed to '_builtin'

citibeth reviewed May 21, 2019

View reviewed changes

lib/spack/spack/build_environment.py Outdated Show resolved Hide resolved

adamjstewart previously requested changes May 21, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

tgamblin requested changes May 21, 2019

View reviewed changes

Simplified handling of builtin values + added unit tests

0f99333

alalazo added 5 commits May 22, 2019 06:55

Simplified tests to check the setting of parallel jobs

d9db721

Renamed the 'values' argument of SetParallelJobs to 'jobs'

0701b17

Removed reference to Package.make_jobs in bazel

ff83a3c

Added '_builtin' to config.yaml fixture + simplified computation of jobs

d33e03c

Added test that verifies parallel=False is not propagating in the DAG

cad37ea

tgamblin reviewed May 22, 2019

View reviewed changes

lib/spack/spack/cmd/common/arguments.py Outdated Show resolved Hide resolved

Fixed out of date comment

4c4d0ab

tgamblin reviewed May 22, 2019

View reviewed changes

lib/spack/spack/cmd/common/arguments.py Show resolved Hide resolved

tgamblin approved these changes May 22, 2019

View reviewed changes

Further simplified SetParallelJobs by removing unneeded conditional

9d2caaa

If jobs gets passed to the action it's already a valid integer. Once we ruled out that the number is not less than 1 we are ensured it's a good number to set in command line scope

tgamblin merged commit c291866 into spack:develop May 24, 2019

alalazo deleted the refactoring/build_environment branch May 24, 2019 18:45

obreitwi mentioned this pull request Jun 26, 2019

PR #11524 breaks packages depending on bazel directly #11850

Closed

Conversation

alalazo commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

citibeth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo commented May 21, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart commented May 21, 2019

Uh oh!

codecov bot commented May 21, 2019

Codecov Report

Uh oh!

alalazo commented May 22, 2019

Uh oh!

Uh oh!

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo commented May 24, 2019

Uh oh!

tgamblin commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo commented May 21, 2019 •

edited

Loading