From 33c3c3c700cf0acd458389526974945e62fe895e Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 30 Oct 2020 21:10:45 +0100 Subject: Config: cache results of get_config (#19605) `config.get_config` now caches the results and returns the same configuration if called multiple times with the same arguments (i.e. the same section and scope). As a consequence, it is expected that users will always call update methods provided in the `config` module after changing the configuration (even if manipulating it as a Python nested dictionary). The following two examples should cover most scenarios: * Most configuration update logic in the core (e.g. relating to adding new compiler) should call `Configuration.update_config` * Tests that need to change the global configuration should use the newly-provided `config.replace_config` function. (if neither of these methods apply, then the essential requirement is to use a method marked as `_config_mutator`) Failure to call such a function after modifying the configuration will lead to unexpected results (e.g. calling `get_config` after changing the configuration will not reflect the changes since the first call to get_config). --- lib/spack/spack/config.py | 51 ++++++++++++++++++++++++---- lib/spack/spack/test/cmd/common/arguments.py | 1 + lib/spack/spack/test/config.py | 4 +-- lib/spack/spack/test/conftest.py | 5 ++- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index 9c12454edb..393a2ebb31 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -2,7 +2,6 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) - """This module implements Spack's configuration file handling. This implements Spack's configuration system, which handles merging @@ -29,9 +28,9 @@ When read in, Spack validates configurations with jsonschemas. The schemas are in submodules of :py:mod:`spack.schema`. """ - import collections import copy +import functools import os import re import sys @@ -157,7 +156,7 @@ class ConfigScope(object): self.sections[section] = data return self.sections[section] - def write_section(self, section): + def _write_section(self, section): filename = self.get_section_filename(section) data = self.get_section(section) @@ -253,7 +252,7 @@ class SingleFileScope(ConfigScope): self.sections[section_key] = {section_key: data} return self.sections.get(section, None) - def write_section(self, section): + def _write_section(self, section): data_to_write = self._raw_data # If there is no existing data, this section SingleFileScope has never @@ -305,7 +304,7 @@ class ImmutableConfigScope(ConfigScope): This is used for ConfigScopes passed on the command line. """ - def write_section(self, section): + def _write_section(self, section): raise ConfigError("Cannot write to immutable scope %s" % self) def __repr__(self): @@ -341,7 +340,7 @@ class InternalConfigScope(ConfigScope): self.sections[section] = None return self.sections[section] - def write_section(self, section): + def _write_section(self, section): """This only validates, as the data is already in memory.""" data = self.get_section(section) if data is not None: @@ -371,6 +370,18 @@ class InternalConfigScope(ConfigScope): return result +def _config_mutator(method): + """Decorator to mark all the methods in the Configuration class + that mutate the underlying configuration. Used to clear the + memoization cache. + """ + @functools.wraps(method) + def _method(self, *args, **kwargs): + self._get_config_memoized.cache.clear() + return method(self, *args, **kwargs) + return _method + + class Configuration(object): """A full Spack configuration, from a hierarchy of config files. @@ -390,6 +401,7 @@ class Configuration(object): self.push_scope(scope) self.format_updates = collections.defaultdict(list) + @_config_mutator def push_scope(self, scope): """Add a higher precedence scope to the Configuration.""" cmd_line_scope = None @@ -404,11 +416,13 @@ class Configuration(object): if cmd_line_scope: self.scopes['command_line'] = cmd_line_scope + @_config_mutator def pop_scope(self): """Remove the highest precedence scope and return it.""" name, scope = self.scopes.popitem(last=True) return scope + @_config_mutator def remove_scope(self, scope_name): return self.scopes.pop(scope_name) @@ -472,6 +486,7 @@ class Configuration(object): scope = self._validate_scope(scope) return scope.get_section_filename(section) + @_config_mutator def clear_caches(self): """Clears the caches for configuration files, @@ -479,6 +494,7 @@ class Configuration(object): for scope in self.scopes.values(): scope.clear() + @_config_mutator def update_config(self, section, update_data, scope=None, force=False): """Update the configuration file for a particular scope. @@ -526,7 +542,7 @@ class Configuration(object): yaml.comments.Comment.attrib, comments) - scope.write_section(section) + scope._write_section(section) def get_config(self, section, scope=None): """Get configuration settings for a section. @@ -552,6 +568,10 @@ class Configuration(object): } """ + return self._get_config_memoized(section, scope) + + @llnl.util.lang.memoized + def _get_config_memoized(self, section, scope): _validate_section_name(section) if scope is None: @@ -619,6 +639,7 @@ class Configuration(object): return value + @_config_mutator def set(self, path, value, scope=None): """Convenience function for setting single values in config files. @@ -784,6 +805,22 @@ def _config(): config = llnl.util.lang.Singleton(_config) +def replace_config(configuration): + """Replace the current global configuration with the instance passed as + argument. + + Args: + configuration (Configuration): the new configuration to be used. + + Returns: + The old configuration that has been removed + """ + global config + config.clear_caches(), configuration.clear_caches() + old_config, config = config, configuration + return old_config + + def get(path, default=None, scope=None): """Module-level wrapper for ``Configuration.get()``.""" return config.get(path, default, scope) diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py index 5117607a2e..76e39bcad0 100644 --- a/lib/spack/spack/test/cmd/common/arguments.py +++ b/lib/spack/spack/test/cmd/common/arguments.py @@ -20,6 +20,7 @@ def parser(): arguments.add_common_arguments(p, ['jobs']) yield p # Cleanup the command line scope if it was set during tests + spack.config.config.clear_caches() if 'command_line' in spack.config.config.scopes: spack.config.config.scopes['command_line'].clear() diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index f43a571e73..b852f4777e 100644 --- a/lib/spack/spack/test/config.py +++ b/lib/spack/spack/test/config.py @@ -766,7 +766,7 @@ config: assert data['config']['install_tree'] == {'root': 'dummy_tree_value'} with pytest.raises(spack.config.ConfigError): - scope.write_section('config') + scope._write_section('config') def test_single_file_scope(tmpdir, config): @@ -839,7 +839,7 @@ def test_write_empty_single_file_scope(tmpdir): env_schema = spack.schema.env.schema scope = spack.config.SingleFileScope( 'test', str(tmpdir.ensure('config.yaml')), env_schema, ['spack']) - scope.write_section('config') + scope._write_section('config') # confirm we can write empty config assert not scope.get_section('config') diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 0a39d1c6c3..f43d6933ba 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -318,8 +318,7 @@ spack.config.config = spack.config._config() @contextlib.contextmanager def use_configuration(config): """Context manager to swap out the global Spack configuration.""" - saved = spack.config.config - spack.config.config = config + saved = spack.config.replace_config(config) # Avoid using real spack configuration that has been cached by other # tests, and avoid polluting the cache with spack test configuration @@ -329,7 +328,7 @@ def use_configuration(config): yield - spack.config.config = saved + spack.config.replace_config(saved) spack.compilers._cache_config_file = saved_compiler_cache -- cgit v1.2.3-60-g2f50