From 2f1ed3eb33a3035fdad23c08a863b41e532e2e66 Mon Sep 17 00:00:00 2001 From: Lin Guo Date: Mon, 20 Jan 2025 01:34:40 -0800 Subject: [PATCH] Clean up use of mutable defaults under lib/ramble/ramble Danger of mutable defaults: https://pylint.pycqa.org/en/latest/user_guide/messages/warning/dangerous-default-value.html. The cleanup list comes from: ``` pylint --disable=all --enable=W0102 lib/ramble/ramble/* ``` There's also a flake8 plugin (bug-bear) that provides this check (along with other more opinionated checks.) Can be a follow-up to see if we should incorporate this into the CI checks. --- lib/ramble/ramble/application.py | 11 ++++-- lib/ramble/ramble/cmd/commands.py | 8 +++-- lib/ramble/ramble/cmd/mirror.py | 2 +- lib/ramble/ramble/expander.py | 12 +++++-- lib/ramble/ramble/filters.py | 4 +-- lib/ramble/ramble/graphs.py | 18 ++++++---- lib/ramble/ramble/keywords.py | 4 ++- .../ramble/language/application_language.py | 6 ++-- lib/ramble/ramble/language/shared_language.py | 35 ++++++++++++------- lib/ramble/ramble/mirror.py | 2 +- lib/ramble/ramble/modifier.py | 5 ++- lib/ramble/ramble/pipeline.py | 4 +-- lib/ramble/ramble/software_environments.py | 4 +-- lib/ramble/ramble/test/cmd/config.py | 4 ++- lib/ramble/ramble/test/stage.py | 4 ++- lib/ramble/ramble/util/executable.py | 7 ++-- lib/ramble/ramble/util/logger.py | 4 ++- lib/ramble/ramble/util/path.py | 6 ++-- lib/ramble/ramble/workload.py | 17 ++++++--- lib/ramble/ramble/workspace/workspace.py | 12 +++++-- 20 files changed, 116 insertions(+), 53 deletions(-) diff --git a/lib/ramble/ramble/application.py b/lib/ramble/ramble/application.py index 43e6f8744..0f569b30f 100644 --- a/lib/ramble/ramble/application.py +++ b/lib/ramble/ramble/application.py @@ -420,7 +420,10 @@ def experiment_log_file(self, logs_dir): """Returns an experiment log file path for the given logs directory""" return os.path.join(logs_dir, self.expander.experiment_namespace) + ".out" - def get_pipeline_phases(self, pipeline, phase_filters=["*"]): + def get_pipeline_phases(self, pipeline, phase_filters=None): + if phase_filters is None: + phase_filters = ["*"] + self.build_modifier_instances() self.build_phase_order() @@ -626,14 +629,16 @@ def run_phase(self, pipeline, phase, workspace): phase_func(workspace, app_inst=self) self._phase_times[phase] = time.time() - start_time - def print_phase_times(self, pipeline, phase_filters=["*"]): + def print_phase_times(self, pipeline, phase_filters=None): """Print phase execution times by pipeline phase order Args: pipeline (str): Name of pipeline to print timing information for - phase_filters (list(str)): Filters to limit phases to print + phase_filters (list(str) | None): Filters to limit phases to print """ logger.msg("Phase timing statistics:") + if phase_filters is None: + phase_filters = ["*"] for phase in self.get_pipeline_phases(pipeline, phase_filters=phase_filters): # Set default time to 0.0 s, to prevent KeyError from skipped phases if phase not in self._phase_times: diff --git a/lib/ramble/ramble/cmd/commands.py b/lib/ramble/ramble/cmd/commands.py index 287886bee..0602e189d 100644 --- a/lib/ramble/ramble/cmd/commands.py +++ b/lib/ramble/ramble/cmd/commands.py @@ -97,9 +97,13 @@ def __init__( prog, out=None, aliases=False, - documented_commands=[], - rst_levels=["-", "-", "^", "~", ":", "`"], + documented_commands=None, + rst_levels=None, ): + if documented_commands is None: + documented_commands = [] + if rst_levels is None: + rst_levels = ["-", "-", "^", "~", ":", "`"] out = sys.stdout if out is None else out super().__init__(prog, out, aliases, rst_levels) self.documented = documented_commands diff --git a/lib/ramble/ramble/cmd/mirror.py b/lib/ramble/ramble/cmd/mirror.py index 254f5c30c..822bf671f 100644 --- a/lib/ramble/ramble/cmd/mirror.py +++ b/lib/ramble/ramble/cmd/mirror.py @@ -104,7 +104,7 @@ def setup_parser(subparser): def mirror_add(args): """Add a mirror to Ramble.""" url = url_util.format(args.url) - ramble.mirror.add(args.name, url, args.scope, args) + ramble.mirror.add(args.name, url, args.scope) def mirror_remove(args): diff --git a/lib/ramble/ramble/expander.py b/lib/ramble/ramble/expander.py index d6d68f2ea..5de34a8bb 100644 --- a/lib/ramble/ramble/expander.py +++ b/lib/ramble/ramble/expander.py @@ -148,8 +148,8 @@ def define_value( allow_passthrough=True, expansion_func=str, evaluation_func=eval, - no_expand_vars=set(), - used_vars=set(), + no_expand_vars=None, + used_vars=None, ): """Define the value for this node. @@ -170,6 +170,10 @@ def define_value( evaluation_func (func): function to use for evaluating math of strings no_expand_vars (set): set of variable names that should never be expanded """ + if no_expand_vars is None: + no_expand_vars = set() + if used_vars is None: + used_vars = set() if self.contents is not None: parts = [] last_idx = 0 @@ -336,7 +340,9 @@ class Expander: _ast_dbg_prefix = "EXPANDER AST:" - def __init__(self, variables, experiment_set, no_expand_vars=set()): + def __init__(self, variables, experiment_set, no_expand_vars=None): + if no_expand_vars is None: + no_expand_vars = set() self._keywords = ramble.keywords.keywords diff --git a/lib/ramble/ramble/filters.py b/lib/ramble/ramble/filters.py index 2129ad1e4..bf718a3b3 100644 --- a/lib/ramble/ramble/filters.py +++ b/lib/ramble/ramble/filters.py @@ -16,14 +16,14 @@ class Filters: def __init__( self, - phase_filters=ALL_PHASES, + phase_filters=None, include_where_filters=None, exclude_where_filters=None, tags=None, ): """Create a new filter instance""" - self.phases = phase_filters + self.phases = ALL_PHASES if phase_filters is None else phase_filters self.include_where = None self.exclude_where = None self.tags = set() diff --git a/lib/ramble/ramble/graphs.py b/lib/ramble/ramble/graphs.py index 1c75810aa..154085aca 100644 --- a/lib/ramble/ramble/graphs.py +++ b/lib/ramble/ramble/graphs.py @@ -35,7 +35,7 @@ def _make_editable(self): self._sorted = None self._prepared = False - def update_graph(self, node, dep_nodes=[], internal_order=False): + def update_graph(self, node, dep_nodes=None, internal_order=False): """Update the graph with a new node and / or new dependencies. Given a node, and list of dependencies, define new edges in the @@ -43,12 +43,14 @@ def update_graph(self, node, dep_nodes=[], internal_order=False): Args: node (GraphNode): Node to inject or modify - dep_nodes (list(GraphNode)): List of nodes that are dependencies + dep_nodes (list(GraphNode) | None): List of nodes that are dependencies internal_order (Boolean): True to process internal dependencies, False to skip """ + if dep_nodes is None: + dep_nodes = [] self._make_editable() self.add_node(node) self.define_edges(node, dep_nodes, internal_order=internal_order) @@ -68,7 +70,7 @@ def add_node(self, node): if node not in self.adj_list: self.adj_list[node] = set() - def define_edges(self, node, dep_nodes=[], internal_order=False): + def define_edges(self, node, dep_nodes=None, internal_order=False): """Define graph edges Process dependencies, and internal orderings (inside the node object) @@ -76,11 +78,13 @@ def define_edges(self, node, dep_nodes=[], internal_order=False): Args: node (GraphNode): Node to inject or modify - dep_nodes (list(GraphNode)): List of nodes that are dependencies + dep_nodes (list(GraphNode) | None): List of nodes that are dependencies internal_order (Boolean): True to process internal dependencies, False to skip """ + if dep_nodes is None: + dep_nodes = [] for dep in dep_nodes: if dep.key not in self.node_definitions: self.node_definitions[dep.key] = dep @@ -198,7 +202,7 @@ def add_node(self, node, obj_inst=None): super().add_node(node) - def update_graph(self, phase_name, dependencies=[], internal_order=False, obj_inst=None): + def update_graph(self, phase_name, dependencies=None, internal_order=False, obj_inst=None): """Update the graph with a new phase and / or new dependencies. Given a phase name, and list of dependencies, define new edges in the @@ -206,11 +210,13 @@ def update_graph(self, phase_name, dependencies=[], internal_order=False, obj_in Args: phase_name (str): Name of the phase to inject or modify - dependencies (list(str)): List of phase names to inject dependencies on + dependencies (list(str) | None): List of phase names to inject dependencies on internal_order (Boolean): True to process internal dependencies, False to skip obj_inst (object): Application or modifier instance to extract phase function from """ + if dependencies is None: + dependencies = [] if self._prepared: del self._sorted self._sorted = None diff --git a/lib/ramble/ramble/keywords.py b/lib/ramble/ramble/keywords.py index 366fb560e..61bc3a350 100644 --- a/lib/ramble/ramble/keywords.py +++ b/lib/ramble/ramble/keywords.py @@ -80,9 +80,11 @@ class Keywords: specific inputs to further configure the experiment. """ - def __init__(self, extra_keys={}): + def __init__(self, extra_keys=None): # Merge in additional Keys: self.keys = default_keys.copy() + if extra_keys is None: + extra_keys = {} self.update_keys(extra_keys) def copy(self): diff --git a/lib/ramble/ramble/language/application_language.py b/lib/ramble/ramble/language/application_language.py index 75e7750e9..ff8c5b589 100644 --- a/lib/ramble/ramble/language/application_language.py +++ b/lib/ramble/ramble/language/application_language.py @@ -85,7 +85,7 @@ def _execute_workload(app): @application_directive("workload_groups") -def workload_group(name, workloads=[], mode=None, **kwargs): +def workload_group(name, workloads=None, mode=None, **kwargs): """Adds a workload group to this application Defines a new workload group that can be used within the context of its @@ -93,8 +93,10 @@ def workload_group(name, workloads=[], mode=None, **kwargs): Args: name (str): The name of the group - workloads (list(str)): A list of workloads to be grouped + workloads (list(str) | None): A list of workloads to be grouped """ + if workloads is None: + workloads = [] def _execute_workload_groups(app): if mode == "append": diff --git a/lib/ramble/ramble/language/shared_language.py b/lib/ramble/ramble/language/shared_language.py index 4b96ed82b..d0e5db760 100644 --- a/lib/ramble/ramble/language/shared_language.py +++ b/lib/ramble/ramble/language/shared_language.py @@ -87,7 +87,7 @@ def figure_of_merit( group_name, log_file="{log_file}", units="", - contexts=[], + contexts=None, fom_type: FomType = FomType.UNDEFINED, ): """Adds a figure of merit to track for this object @@ -100,9 +100,9 @@ def figure_of_merit( fom_regex (str): A regular expression using named groups to extract the FOM group_name (str): The name of the group that the FOM should be pulled from units (str): The units associated with the FOM - contexts (list(str)): List of contexts (defined by - figure_of_merit_context) this figure of merit - should exist in. + contexts (list(str) | None): List of contexts (defined by + figure_of_merit_context) this figure of merit + should exist in. fom_type: The type of figure of merit """ @@ -112,7 +112,7 @@ def _execute_figure_of_merit(obj): "regex": fom_regex, "group_name": group_name, "units": units, - "contexts": contexts, + "contexts": [] if contexts is None else contexts, "fom_type": fom_type, } @@ -276,7 +276,7 @@ def _execute_success_criteria(obj): @shared_directive("builtins") def register_builtin( - name, required=True, injection_method="prepend", depends_on=[], dependents=[] + name, required=True, injection_method="prepend", depends_on=None, dependents=None ): """Register a builtin @@ -326,11 +326,16 @@ def example_builtin(self): required (boolean): Whether the builtin will be auto-injected or not injection_method (str): The method of injecting the builtin. Can be 'prepend' or 'append' - depends_on (list(str)): The names of builtins this builtin depends on - (and must execute after). - dependents (list(str)): The names of builtins that should come - after the current one. + depends_on (list(str) | None): The names of builtins this builtin depends on + (and must execute after). + dependents (list(str) | None): The names of builtins that should come + after the current one. """ + if depends_on is None: + depends_on = [] + if dependents is None: + dependents = [] + supported_injection_methods = ["prepend", "append"] def _store_builtin(obj): @@ -355,7 +360,7 @@ def _store_builtin(obj): @shared_directive("phase_definitions") -def register_phase(name, pipeline=None, run_before=[], run_after=[]): +def register_phase(name, pipeline=None, run_before=None, run_after=None): """Register a phase Phases are portions of a pipeline that will execute when @@ -370,9 +375,13 @@ def register_phase(name, pipeline=None, run_before=[], run_after=[]): Args: name (str): The name of the phase. Phases are functions named '_'. pipeline (str): The name of the pipeline this phase should be registered into. - run_before (list(str)): A list of phase names this phase should run before - run_after (list(str)): A list of phase names this phase should run after + run_before (list(str) | None): A list of phase names this phase should run before + run_after (list(str) | None): A list of phase names this phase should run after """ + if run_before is None: + run_before = [] + if run_after is None: + run_after = [] def _execute_register_phase(obj): import ramble.util.graph diff --git a/lib/ramble/ramble/mirror.py b/lib/ramble/ramble/mirror.py index 893608f53..d094c9b29 100644 --- a/lib/ramble/ramble/mirror.py +++ b/lib/ramble/ramble/mirror.py @@ -420,7 +420,7 @@ def create(path, workspace): return mirror_stats.stats() -def add(name, url, scope, args={}): +def add(name, url, scope): """Add a named mirror in the given scope""" mirrors = ramble.config.get("mirrors", scope=scope) if not mirrors: diff --git a/lib/ramble/ramble/modifier.py b/lib/ramble/ramble/modifier.py index 319f1f3d4..090cecf54 100644 --- a/lib/ramble/ramble/modifier.py +++ b/lib/ramble/ramble/modifier.py @@ -148,9 +148,12 @@ def format_doc(self, **kwargs): results.write((" " * indent) + line + "\n") return results.getvalue() - def modded_variables(self, app, extra_vars={}): + def modded_variables(self, app, extra_vars=None): mods = {} + if extra_vars is None: + extra_vars = {} + if self._usage_mode not in self.variable_modifications: return mods diff --git a/lib/ramble/ramble/pipeline.py b/lib/ramble/ramble/pipeline.py index 5c1dac7c9..7c0a2c174 100644 --- a/lib/ramble/ramble/pipeline.py +++ b/lib/ramble/ramble/pipeline.py @@ -240,7 +240,7 @@ def __init__( self, workspace, filters, - output_formats=["text"], + output_formats=None, upload=False, print_results=False, summary_only=False, @@ -251,7 +251,7 @@ def __init__( super().__init__(workspace, filters) self.action_string = "Analyzing" - self.output_formats = output_formats + self.output_formats = ["text"] if output_formats is None else output_formats self.require_inventory = True self.upload_results = upload self.print_results = print_results diff --git a/lib/ramble/ramble/software_environments.py b/lib/ramble/ramble/software_environments.py index c4cf336d0..e0c4dfadc 100644 --- a/lib/ramble/ramble/software_environments.py +++ b/lib/ramble/ramble/software_environments.py @@ -69,7 +69,7 @@ def is_used(self): """ return self._used - def spec_str(self, all_packages: dict = {}, compiler=False): + def spec_str(self, all_packages=None, compiler=False): """Return a spec string for this software package Args: @@ -170,7 +170,7 @@ def __init__( self.compiler_spec = compiler_spec self._package_type = "Rendered" - def spec_str(self, all_packages: dict = {}, compiler=False): + def spec_str(self, all_packages=None, compiler=False): """Return a spec string for this software package Args: diff --git a/lib/ramble/ramble/test/cmd/config.py b/lib/ramble/ramble/test/cmd/config.py index fb567d1d9..802b5f196 100644 --- a/lib/ramble/ramble/test/cmd/config.py +++ b/lib/ramble/ramble/test/cmd/config.py @@ -23,7 +23,9 @@ workspace = ramble.main.RambleCommand("workspace") -def _create_config(scope=None, data={}, section="repos"): +def _create_config(scope=None, data=None, section="repos"): + if data is None: + data = {} scope = scope or ramble.config.default_modify_scope() cfg_file = ramble.config.config.get_config_filename(scope, section) with open(cfg_file, "w") as f: diff --git a/lib/ramble/ramble/test/stage.py b/lib/ramble/ramble/test/stage.py index e022b362b..5ca422ae8 100644 --- a/lib/ramble/ramble/test/stage.py +++ b/lib/ramble/ramble/test/stage.py @@ -194,7 +194,9 @@ def mock_stage_archive(tmp_build_stage_dir): # <_hidden_fn> Optional hidden file (contains _hidden_contents) # <_archive_fn> archive_url = file:///path/to/<_archive_fn> # - def create_stage_archive(expected_file_list=[_include_readme]): + def create_stage_archive(expected_file_list=None): + if expected_file_list is None: + expected_file_list = [_include_readme] tmpdir, test_stage_path = tmp_build_stage_dir mkdirp(test_stage_path) diff --git a/lib/ramble/ramble/util/executable.py b/lib/ramble/ramble/util/executable.py index 91f7e7adc..f6ac13fef 100644 --- a/lib/ramble/ramble/util/executable.py +++ b/lib/ramble/ramble/util/executable.py @@ -71,7 +71,7 @@ def __init__( template, use_mpi=False, mpi=False, - variables={}, + variables=None, redirect="{log_file}", output_capture=OUTPUT_CAPTURE.DEFAULT, run_in_background=False, @@ -85,12 +85,15 @@ def __init__( - use_mpi: Boolean value for if MPI should be applied to each portion of this executable's template - mpi: Same as use_mpi - - variables (dict): dictionary of variable definitions to use for this executable only + - variables (dict | None): dictionary of variable definitions + to use for this executable only - redirect: File to redirect output of template into - output_capture: Operator to use when capturing output - run_in_background: If true, run the command in background """ + if variables is None: + variables = {} if isinstance(template, str): self.template = [template] elif isinstance(template, list): diff --git a/lib/ramble/ramble/util/logger.py b/lib/ramble/ramble/util/logger.py index 9c722dbbf..11a4b675d 100644 --- a/lib/ramble/ramble/util/logger.py +++ b/lib/ramble/ramble/util/logger.py @@ -78,7 +78,7 @@ def active_stream(self): return self.log_stack[-1][1] return None - def _stream_kwargs(self, default_kwargs={}, index=None): + def _stream_kwargs(self, default_kwargs=None, index=None): """Construct keyword arguments for a stream Build keyword arguments of the form: {'stream': } to allow @@ -97,6 +97,8 @@ def _stream_kwargs(self, default_kwargs={}, index=None): Returns: kwargs: Constructed kwargs with defaults applied """ + if default_kwargs is None: + default_kwargs = {} kwargs = {} stream_index = None if index is not None: diff --git a/lib/ramble/ramble/util/path.py b/lib/ramble/ramble/util/path.py index 1066b634f..d7d968a2a 100644 --- a/lib/ramble/ramble/util/path.py +++ b/lib/ramble/ramble/util/path.py @@ -62,7 +62,7 @@ def get_system_path_max(): return sys_max_path_length -def substitute_config_variables(path, local_replacements={}): +def substitute_config_variables(path, local_replacements): """Substitute placeholders into paths. Ramble allows paths in configs to have some placeholders, as follows: @@ -86,8 +86,10 @@ def repl(match): return re.sub(r"(\$\w+\b|\$\{\w+\})", repl, path) -def substitute_path_variables(path, local_replacements={}): +def substitute_path_variables(path, local_replacements=None): """Substitute config vars, expand environment vars, expand user home.""" + if local_replacements is None: + local_replacements = {} path = substitute_config_variables(path, local_replacements) path = os.path.expandvars(path) path = os.path.expanduser(path) diff --git a/lib/ramble/ramble/workload.py b/lib/ramble/ramble/workload.py index e1dd5e987..c453f5b25 100644 --- a/lib/ramble/ramble/workload.py +++ b/lib/ramble/ramble/workload.py @@ -6,7 +6,7 @@ # option. This file may not be copied, modified, or distributed # except according to those terms. -from typing import List +from typing import List, Optional import copy import ramble.util.colors as rucolor @@ -121,16 +121,25 @@ class Workload: """Class representing a single workload""" def __init__( - self, name: str, executables: List[str], inputs: List[str] = [], tags: List[str] = [] + self, + name: str, + executables: List[str], + inputs: Optional[List[str]] = None, + tags: Optional[List[str]] = None, ): """Constructor for a workload Args: name (str): Name of this workload executables (list(str)): List of executable names for this workload - inputs (list(str)): List of input names for this workload - tags (list(str)): List of tags for this workload + inputs (list(str) | None): List of input names for this workload + tags (list(str) | None): List of tags for this workload """ + if inputs is None: + inputs = [] + if tags is None: + tags = [] + self.name = name self.variables = {} self.environment_variables = {} diff --git a/lib/ramble/ramble/workspace/workspace.py b/lib/ramble/ramble/workspace/workspace.py index a14c241f5..422df0130 100644 --- a/lib/ramble/ramble/workspace/workspace.py +++ b/lib/ramble/ramble/workspace/workspace.py @@ -1074,7 +1074,7 @@ def add_experiments( variable_definitions, experiment_name, package_manager=None, - zips=[], + zips=None, matrix=None, overwrite=False, ): @@ -1096,7 +1096,7 @@ def add_experiments( within generated experiments experiment_name (str): The name of the experiments to add package_manager (str): Name of package manager to use for the generated experiments - zips (list(str)): List of strings representing zips to define, in the + zips (list(str) | None): List of strings representing zips to define, in the format zipname=[var1,var2,var3] matrix (str): String representing a matrix to define within the experiment in the format of var1,var2,var3. @@ -1104,6 +1104,9 @@ def add_experiments( collide with new definitions or not. """ + if zips is None: + zips = [] + def yaml_add_comment_before_key( base, key, comment, column=None, clear=False, start_char="#" ): @@ -1507,7 +1510,7 @@ def symlink_result(self, out_file, latest_file): create_symlink(out_file, latest_file) - def dump_results(self, output_formats=["text"], print_results=False, summary_only=False): + def dump_results(self, output_formats=None, print_results=False, summary_only=False): """ Write out result file in desired format @@ -1517,6 +1520,9 @@ def dump_results(self, output_formats=["text"], print_results=False, summary_onl """ + if output_formats is None: + output_formats = ["text"] + if not self.results: self.results = {}