summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMassimiliano Culpo <massimiliano.culpo@gmail.com>2019-05-24 20:45:22 +0200
committerTodd Gamblin <tgamblin@llnl.gov>2019-05-24 11:45:22 -0700
commitc291866b9ac1c6b36022af4b8e9f362669e5f84f (patch)
tree62bed0c148d735b0c346da64f28e3bb3d0a70b6b
parent1bd4521f723f7ac1d901423829ca965aeb00f9de (diff)
downloadspack-c291866b9ac1c6b36022af4b8e9f362669e5f84f.tar.gz
spack-c291866b9ac1c6b36022af4b8e9f362669e5f84f.tar.bz2
spack-c291866b9ac1c6b36022af4b8e9f362669e5f84f.tar.xz
spack-c291866b9ac1c6b36022af4b8e9f362669e5f84f.zip
build env: simplify handling of parallel jobs (#11524)
This PR implements several refactors requested in #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`)
-rw-r--r--lib/spack/spack/build_environment.py9
-rw-r--r--lib/spack/spack/cmd/bootstrap.py5
-rw-r--r--lib/spack/spack/cmd/common/arguments.py37
-rw-r--r--lib/spack/spack/cmd/diy.py4
-rw-r--r--lib/spack/spack/cmd/install.py5
-rw-r--r--lib/spack/spack/package.py9
-rw-r--r--lib/spack/spack/test/build_environment.py17
-rw-r--r--lib/spack/spack/test/cmd/common/arguments.py40
-rw-r--r--lib/spack/spack/test/conftest.py6
-rw-r--r--var/spack/repos/builtin.mock/packages/a/package.py2
-rw-r--r--var/spack/repos/builtin/packages/bazel/package.py7
11 files changed, 104 insertions, 37 deletions
diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 90b02cc99f..1abc9ebeb7 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -404,12 +404,9 @@ def set_build_environment_variables(pkg, env, dirty):
def _set_variables_for_single_module(pkg, module):
"""Helper function to set module variables for single module."""
- # number of jobs spack will build with.
- jobs = spack.config.get('config:build_jobs') or multiprocessing.cpu_count()
- if not pkg.parallel:
- jobs = 1
- elif pkg.make_jobs:
- jobs = pkg.make_jobs
+
+ jobs = spack.config.get('config:build_jobs') if pkg.parallel else 1
+ assert jobs is not None, "no default set for config:build_jobs"
m = module
m.make_jobs = jobs
diff --git a/lib/spack/spack/cmd/bootstrap.py b/lib/spack/spack/cmd/bootstrap.py
index 593b581c30..bf0002630b 100644
--- a/lib/spack/spack/cmd/bootstrap.py
+++ b/lib/spack/spack/cmd/bootstrap.py
@@ -15,9 +15,7 @@ level = "long"
def setup_parser(subparser):
- subparser.add_argument(
- '-j', '--jobs', action='store', type=int,
- help="explicitly set number of make jobs (default: #cpus)")
+ arguments.add_common_arguments(subparser, ['jobs'])
subparser.add_argument(
'--keep-prefix', action='store_true', dest='keep_prefix',
help="don't remove the install prefix if installation fails")
@@ -38,7 +36,6 @@ def bootstrap(parser, args, **kwargs):
'keep_prefix': args.keep_prefix,
'keep_stage': args.keep_stage,
'install_deps': 'dependencies',
- 'make_jobs': args.jobs,
'verbose': args.verbose,
'dirty': args.dirty
})
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 25619f2aa8..b228c16071 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -72,6 +72,35 @@ class ConstraintAction(argparse.Action):
return sorted(specs.values())
+class SetParallelJobs(argparse.Action):
+ """Sets the correct value for parallel build jobs.
+
+ The value is is set in the command line configuration scope so that
+ it can be retrieved using the spack.config API.
+ """
+ def __call__(self, parser, namespace, jobs, option_string):
+ # Jobs is a single integer, type conversion is already applied
+ # see https://docs.python.org/3/library/argparse.html#action-classes
+ if jobs < 1:
+ msg = 'invalid value for argument "{0}" '\
+ '[expected a positive integer, got "{1}"]'
+ raise ValueError(msg.format(option_string, jobs))
+
+ spack.config.set('config:build_jobs', jobs, scope='command_line')
+
+ setattr(namespace, 'jobs', jobs)
+
+ @property
+ def default(self):
+ # This default is coded as a property so that look-up
+ # of this value is done only on demand
+ return spack.config.get('config:build_jobs')
+
+ @default.setter
+ def default(self, value):
+ pass
+
+
_arguments['constraint'] = Args(
'constraint', nargs=argparse.REMAINDER, action=ConstraintAction,
help='constraint to select a subset of installed packages')
@@ -111,17 +140,13 @@ _arguments['very_long'] = Args(
'-L', '--very-long', action='store_true',
help='show full dependency hashes as well as versions')
-_arguments['jobs'] = Args(
- '-j', '--jobs', action='store', type=int, dest='jobs',
- help="explicitely set number of make jobs. default is #cpus")
-
_arguments['tags'] = Args(
'-t', '--tags', action='append',
help='filter a package query by tags')
_arguments['jobs'] = Args(
- '-j', '--jobs', action='store', type=int, dest="jobs",
- help="explicitly set number of make jobs, default is #cpus.")
+ '-j', '--jobs', action=SetParallelJobs, type=int, dest='jobs',
+ help='explicitly set number of parallel jobs')
_arguments['install_status'] = Args(
'-I', '--install-status', action='store_true', default=False,
diff --git a/lib/spack/spack/cmd/diy.py b/lib/spack/spack/cmd/diy.py
index 1027f80e82..a78a3fc80b 100644
--- a/lib/spack/spack/cmd/diy.py
+++ b/lib/spack/spack/cmd/diy.py
@@ -50,10 +50,6 @@ def diy(self, args):
if not args.spec:
tty.die("spack diy requires a package spec argument.")
- if args.jobs is not None:
- if args.jobs <= 0:
- tty.die("the -j option must be a positive integer")
-
specs = spack.cmd.parse_specs(args.spec)
if len(specs) > 1:
tty.die("spack diy only takes one spec.")
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index 1570309a58..91618fcfd9 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -35,7 +35,6 @@ def update_kwargs_from_args(args, kwargs):
'keep_stage': args.keep_stage,
'restage': not args.dont_restage,
'install_source': args.install_source,
- 'make_jobs': args.jobs,
'verbose': args.verbose,
'fake': args.fake,
'dirty': args.dirty,
@@ -232,10 +231,6 @@ def install(parser, args, **kwargs):
else:
tty.die("install requires a package argument or a spack.yaml file")
- if args.jobs is not None:
- if args.jobs <= 0:
- tty.die("The -j option must be a positive integer!")
-
if args.no_checksum:
spack.config.set('config:checksum', False, scope='command_line')
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 4b81afe84e..4af53e00d0 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -396,9 +396,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
#: By default we build in parallel. Subclasses can override this.
parallel = True
- #: # jobs to use for parallel make. If set, overrides default of ncpus.
- make_jobs = None
-
#: By default do not run tests within package's install()
run_tests = False
@@ -1369,8 +1366,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
skip_patch (bool): Skip patch stage of build if True.
verbose (bool): Display verbose build output (by default,
suppresses it)
- make_jobs (int): Number of make jobs to use for install. Default
- is ncpus
fake (bool): Don't really build; install fake stub files instead.
explicit (bool): True if package was explicitly installed, False
if package was implicitly installed (as a dependency).
@@ -1393,7 +1388,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
install_deps = kwargs.get('install_deps', True)
skip_patch = kwargs.get('skip_patch', False)
verbose = kwargs.get('verbose', False)
- make_jobs = kwargs.get('make_jobs', None)
fake = kwargs.get('fake', False)
explicit = kwargs.get('explicit', False)
tests = kwargs.get('tests', False)
@@ -1469,9 +1463,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
self.run_tests = (tests is True or
tests and self.name in tests)
- # Set parallelism before starting build.
- self.make_jobs = make_jobs
-
# Then install the package itself.
def build_process():
"""This implements the process forked for each build.
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index 1927a76b38..1146073db5 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/lib/spack/spack/test/build_environment.py
@@ -279,3 +279,20 @@ def test_set_build_environment_variables(
finally:
delattr(dep_pkg, 'libs')
+
+
+def test_parallel_false_is_not_propagating(config, mock_packages):
+ class AttributeHolder(object):
+ pass
+
+ # Package A has parallel = False and depends on B which instead
+ # can be built in parallel
+ s = spack.spec.Spec('a foobar=bar')
+ s.concretize()
+
+ for spec in s.traverse():
+ expected_jobs = spack.config.get('config:build_jobs') \
+ if s.package.parallel else 1
+ m = AttributeHolder()
+ spack.build_environment._set_variables_for_single_module(s.package, m)
+ assert m.make_jobs == expected_jobs
diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py
new file mode 100644
index 0000000000..f7ab281dac
--- /dev/null
+++ b/lib/spack/spack/test/cmd/common/arguments.py
@@ -0,0 +1,40 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import argparse
+import multiprocessing
+
+import pytest
+
+
+import spack.cmd.common.arguments as arguments
+import spack.config
+
+
+@pytest.fixture()
+def parser():
+ p = argparse.ArgumentParser()
+ arguments.add_common_arguments(p, ['jobs'])
+ yield p
+ # Cleanup the command line scope if it was set during tests
+ if 'command_line' in spack.config.config.scopes:
+ spack.config.config.remove_scope('command_line')
+
+
+@pytest.mark.parametrize('cli_args,expected', [
+ (['-j', '24'], 24),
+ ([], multiprocessing.cpu_count())
+])
+def test_setting_parallel_jobs(parser, cli_args, expected):
+ namespace = parser.parse_args(cli_args)
+ assert namespace.jobs == expected
+ assert spack.config.get('config:build_jobs') == expected
+
+
+def test_negative_integers_not_allowed_for_parallel_jobs(parser):
+ with pytest.raises(ValueError) as exc_info:
+ parser.parse_args(['-j', '-2'])
+
+ assert 'expected a positive integer' in str(exc_info.value)
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 9991b53f30..c628d7e3ae 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -288,7 +288,11 @@ def config(configuration_dir):
real_configuration = spack.config.config
- test_scopes = [
+ defaults = spack.config.InternalConfigScope(
+ '_builtin', spack.config.config_defaults
+ )
+ test_scopes = [defaults]
+ test_scopes += [
spack.config.ConfigScope(name, str(configuration_dir.join(name)))
for name in ['site', 'system', 'user']]
test_scopes.append(spack.config.InternalConfigScope('command_line'))
diff --git a/var/spack/repos/builtin.mock/packages/a/package.py b/var/spack/repos/builtin.mock/packages/a/package.py
index 7e5a6f72e6..951e633d78 100644
--- a/var/spack/repos/builtin.mock/packages/a/package.py
+++ b/var/spack/repos/builtin.mock/packages/a/package.py
@@ -32,6 +32,8 @@ class A(AutotoolsPackage):
depends_on('b', when='foobar=bar')
+ parallel = False
+
def with_or_without_fee(self, activated):
if not activated:
return '--no-fee'
diff --git a/var/spack/repos/builtin/packages/bazel/package.py b/var/spack/repos/builtin/packages/bazel/package.py
index 0c218f6641..51e4e51050 100644
--- a/var/spack/repos/builtin/packages/bazel/package.py
+++ b/var/spack/repos/builtin/packages/bazel/package.py
@@ -3,6 +3,8 @@
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import inspect
+
from spack import *
from multiprocessing import cpu_count
from spack.util.environment import env_flag
@@ -90,8 +92,9 @@ class Bazel(Package):
return super(BazelExecutable, self).__call__(*args, **kwargs)
jobs = cpu_count()
+ dependent_module = inspect.getmodule(dependent_spec.package)
if not dependent_spec.package.parallel:
jobs = 1
- elif dependent_spec.package.make_jobs:
- jobs = dependent_spec.package.make_jobs
+ elif dependent_module.make_jobs:
+ jobs = dependent_module.make_jobs
module.bazel = BazelExecutable('bazel', 'build', jobs)