diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ac7348d8..08bac767c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The learn.py script in the openfoam_wf* examples will now create the missing Energy v Lidspeed plot - Fixed the flags associated with the `stop-workers` command (--spec, --queues, --workers) - Fixed the --step flag for the `run-workers` command +- Fixed most of the pylint errors that we're showing up when you ran `make check-style` + - Some errors have been disabled rather than fixed. These include: + - Any pylint errors in merlin_template.py since it's deprecated now + - A "duplicate code" instance between a function in `expansion.py` and a method in `study.py` + - The function is explicitly not creating a MerlinStudy object so the code *must* be duplicate here + - Invalid-name (C0103): These errors typically relate to the names of short variables (i.e. naming files something like f or errors e) + - Unused-argument (W0613): These have been disabled for celery-related functions since celery *does* use these arguments behind the scenes + - Broad-exception (W0718): Pylint wants a more specific exception but sometimes it's ok to have a broad exception + - Import-outside-toplevel (C0415): Sometimes it's necessary for us to import inside a function. Where this is the case, these errors are disabled + - Too-many-statements (R0915): This is disabled for the `setup_argparse` function in `main.py` since it's necessary to be big. It's disabled in `tasks.py` and `celeryadapter.py` too until we can get around to refactoring some code there + - No-else-return (R1705): These are disabled in `router.py` until we refactor the file + - Consider-using-with (R1732): Pylint wants us to consider using with for calls to subprocess.run or subprocess.Popen but it's not necessary + - Too-many-arguments (R0913): These are disabled for functions that I believe *need* to have several arguments + - Note: these could be fixed by using *args and **kwargs but it makes the code harder to follow so I'm opting to not do that + - Too-many-local-variables (R0914): These are disabled for functions that have a lot of variables + - It may be a good idea at some point to go through these and try to find ways to shorten the number of variables used or split the functions up + - Too-many-branches (R0912): These are disabled for certain functions that require a good amount of branching + - Might be able to fix this in the future if we split functions up more + - Too-few-public-methods (R0903): These are disabled for classes we may add to in the future or "wrapper" classes + - Attribute-defined-outside-init (W0201): These errors are only disabled in `specification.py` as they occur in class methods so init() won't be called ### Added - Now loads np.arrays of dtype='object', allowing mix-type sample npy @@ -27,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added the --disable-logs flag to the `run-workers` command - Merlin will now assign `default_worker` to any step not associated with a worker - Added `get_step_worker_map()` as a method in `specification.py` +- Added `tabulate_info()` function in `display.py` to help with table formatting ### Changed - Changed celery_regex to celery_slurm_regex in test_definitions.py @@ -42,6 +63,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Modified the `merlinspec.json` file: - the minimum `gpus per task` is now 0 instead of 1 - variables defined in the `env` block of a spec file can now be arrays +- Refactored `batch.py`: + - Merged 4 functions (`check_for_slurm`, `check_for_lsf`, `check_for_flux`, and `check_for_pbs`) into 1 function named `check_for_scheduler` + - Modified `get_batch_type` to accommodate this change + - Added a function `parse_batch_block` to handle all the logic of reading in the batch block and storing it in one dict + - Added a function `get_flux_launch` to help decrease the amount of logic taking place in `batch_worker_launch` + - Modified `batch_worker_launch` to use the new `parse_batch_block` function + - Added a function `construct_scheduler_legend` to build a dict that keeps as much information as we need about each scheduler stored in one place + - Cleaned up the `construct_worker_launch_command` function to utilize the newly added functions and decrease the amount of repeated code + ## [1.9.1] ### Fixed diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 2d4a7c9f5..145fe0e02 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -6,4 +6,4 @@ Joe Koning Jeremy White Aidan Keogh Ryan Lee -Brian Gunnarson \ No newline at end of file +Brian Gunnarson diff --git a/docs/source/merlin_commands.rst b/docs/source/merlin_commands.rst index 2a767797f..316d675fe 100644 --- a/docs/source/merlin_commands.rst +++ b/docs/source/merlin_commands.rst @@ -378,7 +378,7 @@ To send out a stop signal to some or all connected workers, use: $ merlin stop-workers [--spec ] [--queues ] [--workers ] [--task_server celery] -The default behavior will send a stop to all connected workers, +The default behavior will send a stop to all connected workers across all workflows, having them shutdown softly. The ``--spec`` option targets only workers named in the ``merlin`` block of the spec file. @@ -390,7 +390,7 @@ The ``--queues`` option allows you to pass in the names of specific queues to st # Stop all workers on these queues, no matter their name $ merlin stop-workers --queues queue1 queue2 -The ``--workers`` option allows you to pass in a regular expression of names of queues to stop: +The ``--workers`` option allows you to pass in regular expressions of names of workers to stop: .. code:: bash diff --git a/docs/source/modules/port_your_application.rst b/docs/source/modules/port_your_application.rst index c9d89b06d..0dd501c2e 100644 --- a/docs/source/modules/port_your_application.rst +++ b/docs/source/modules/port_your_application.rst @@ -44,7 +44,7 @@ The scripts defined in the workflow steps are also written to the output directo .. where are the worker logs, and what might show up there that .out and .err won't see? -> these more developer focused output? -When a bug crops up in a running study with many parameters, there are a few other commands to make use of. Rather than trying to spam ``Ctrl-c`` to kill all the workers, you will want to instead use ``merlin stop-workers .yaml`` to stop the workers. This should then be followed up with ``merlin purge .yaml`` to clear out the task queue to prevent the same +When a bug crops up in a running study with many parameters, there are a few other commands to make use of. Rather than trying to spam ``Ctrl-c`` to kill all the workers, you will want to instead use ``merlin stop-workers --spec .yaml`` to stop the workers for that workflow. This should then be followed up with ``merlin purge .yaml`` to clear out the task queue to prevent the same buggy tasks from continuing to run the next time ``run-workers`` is invoked. .. last item from board: use merlin status to see if have workers ... is that 'dangling tasks' in the image? diff --git a/merlin/celery.py b/merlin/celery.py index 52d4e4589..9febf2523 100644 --- a/merlin/celery.py +++ b/merlin/celery.py @@ -44,12 +44,22 @@ from merlin.config import broker, celeryconfig, results_backend from merlin.config.configfile import CONFIG from merlin.config.utils import Priority, get_priority -from merlin.router import route_for_task from merlin.utils import nested_namespace_to_dicts LOG: logging.Logger = logging.getLogger(__name__) + +# This function has to have specific args/return values for celery so ignore pylint +def route_for_task(name, args, kwargs, options, task=None, **kw): # pylint: disable=W0613,R1710 + """ + Custom task router for queues + """ + if ":" in name: + queue, _ = name.split(":") + return {"queue": queue} + + merlin.common.security.encrypt_backend_traffic.set_backend_funcs() @@ -84,7 +94,7 @@ # set task priority defaults to prioritize workflow tasks over task-expansion tasks task_priority_defaults: Dict[str, Union[int, Priority]] = { "task_queue_max_priority": 10, - "task_default_priority": get_priority(Priority.mid), + "task_default_priority": get_priority(Priority.MID), } if CONFIG.broker.name.lower() == "redis": app.conf.broker_transport_options = { diff --git a/merlin/common/abstracts/enums/__init__.py b/merlin/common/abstracts/enums/__init__.py index f242b15fb..366692ac1 100644 --- a/merlin/common/abstracts/enums/__init__.py +++ b/merlin/common/abstracts/enums/__init__.py @@ -32,17 +32,7 @@ from enum import IntEnum -__all__ = ( - "ReturnCode", - "OK_VALUE", - "ERROR_VALUE", - "RESTART_VALUE", - "SOFT_FAIL_VALUE", - "HARD_FAIL_VALUE", - "DRY_OK_VALUE", - "RETRY_VALUE", - "STOP_WORKERS_VALUE", -) +__all__ = ("ReturnCode",) class ReturnCode(IntEnum): diff --git a/merlin/common/openfilelist.py b/merlin/common/openfilelist.py index 5e82abbc2..d7dbf0ff4 100644 --- a/merlin/common/openfilelist.py +++ b/merlin/common/openfilelist.py @@ -57,6 +57,9 @@ """ +# This file is not currently used so we don't care what pylint has to say +# pylint: skip-file + import copy diff --git a/merlin/common/opennpylib.py b/merlin/common/opennpylib.py index c2391ca6d..e91105bf8 100644 --- a/merlin/common/opennpylib.py +++ b/merlin/common/opennpylib.py @@ -81,6 +81,9 @@ print a.dtype # dtype of array """ +# This file is not currently used so we don't care what pylint has to say +# pylint: skip-file + from typing import List, Tuple import numpy as np diff --git a/merlin/common/sample_index.py b/merlin/common/sample_index.py index dd93cf5a7..0d786820a 100644 --- a/merlin/common/sample_index.py +++ b/merlin/common/sample_index.py @@ -69,7 +69,7 @@ class SampleIndex: # Class variable to indicate depth (mostly used for pretty printing). depth = -1 - def __init__(self, minid, maxid, children, name, leafid=-1, num_bundles=0, address=""): + def __init__(self, minid, maxid, children, name, leafid=-1, num_bundles=0, address=""): # pylint: disable=R0913 """The constructor.""" # The direct children of this node, generally also of type SampleIndex. @@ -251,14 +251,14 @@ def get_path_to_sample(self, sample_id): """ path = self.name for child_val in self.children.values(): - if sample_id >= child_val.min and sample_id < child_val.max: + if child_val.min <= sample_id < child_val.max: path = os.path.join(path, child_val.get_path_to_sample(sample_id)) return path def write_single_sample_index_file(self, path): """Writes the index file associated with this node.""" if not self.is_directory: - return + return None fname = os.path.join(path, self.name, "sample_index.txt") with open(fname, "w") as _file: diff --git a/merlin/common/sample_index_factory.py b/merlin/common/sample_index_factory.py index 45ddb2ed1..47e600d82 100644 --- a/merlin/common/sample_index_factory.py +++ b/merlin/common/sample_index_factory.py @@ -37,6 +37,11 @@ from merlin.utils import cd +# These pylint errors I've disabled are for "too many arguments" +# and "too many local variables". I think the functions are still clear +# pylint: disable=R0913,R0914 + + def create_hierarchy( num_samples, bundle_size, diff --git a/merlin/common/security/encrypt.py b/merlin/common/security/encrypt.py index 6a0766427..1c9bd342b 100644 --- a/merlin/common/security/encrypt.py +++ b/merlin/common/security/encrypt.py @@ -28,9 +28,7 @@ # SOFTWARE. ############################################################################### -""" -TODO -""" +"""This module handles encryption logic""" import logging import os @@ -40,6 +38,10 @@ from merlin.config.configfile import CONFIG +# This disables all the errors about short variable names (like using f to represent a file) +# pylint: disable=invalid-name + + LOG = logging.getLogger(__name__) @@ -52,8 +54,8 @@ def _get_key_path(): try: key_filepath = os.path.abspath(os.path.expanduser(key_filepath)) - except KeyError: - raise ValueError("Error! No password provided for RabbitMQ") + except KeyError as e: + raise ValueError("Error! No password provided for RabbitMQ") from e return key_filepath diff --git a/merlin/common/tasks.py b/merlin/common/tasks.py index b0eca1b6c..3a4077ac3 100644 --- a/merlin/common/tasks.py +++ b/merlin/common/tasks.py @@ -35,8 +35,10 @@ import os from typing import Any, Dict, Optional +# Need to disable an overwrite warning here since celery has an exception that we need that directly +# overwrites a python built-in exception from celery import chain, chord, group, shared_task, signature -from celery.exceptions import MaxRetriesExceededError, OperationalError, TimeoutError +from celery.exceptions import MaxRetriesExceededError, OperationalError, TimeoutError # pylint: disable=W0622 from merlin.common.abstracts.enums import ReturnCode from merlin.common.sample_index import uniform_directories @@ -62,14 +64,21 @@ STOP_COUNTDOWN = 60 +# TODO: most of the pylint errors that are disabled in this file are the ones listed below. +# We should refactor this file so that we use more functions to solve all of these errors +# R0912: too many branches +# R0913: too many arguments +# R0914: too many local variables +# R0915: too many statements + @shared_task( # noqa: C901 bind=True, autoretry_for=retry_exceptions, retry_backoff=True, - priority=get_priority(Priority.high), + priority=get_priority(Priority.HIGH), ) -def merlin_step(self, *args: Any, **kwargs: Any) -> Optional[ReturnCode]: # noqa: C901 +def merlin_step(self, *args: Any, **kwargs: Any) -> Optional[ReturnCode]: # noqa: C901 pylint: disable=R0912,R0915 """ Executes a Merlin Step :param args: The arguments, one of which should be an instance of Step @@ -123,7 +132,8 @@ def merlin_step(self, *args: Any, **kwargs: Any) -> Optional[ReturnCode]: # noq self.retry(countdown=step.retry_delay) except MaxRetriesExceededError: LOG.warning( - f"*** Step '{step_name}' in '{step_dir}' exited with a MERLIN_RESTART command, but has already reached its retry limit ({self.max_retries}). Continuing with workflow." + f"""*** Step '{step_name}' in '{step_dir}' exited with a MERLIN_RESTART command, + but has already reached its retry limit ({self.max_retries}). Continuing with workflow.""" ) result = ReturnCode.SOFT_FAIL elif result == ReturnCode.RETRY: @@ -135,7 +145,8 @@ def merlin_step(self, *args: Any, **kwargs: Any) -> Optional[ReturnCode]: # noq self.retry(countdown=step.retry_delay) except MaxRetriesExceededError: LOG.warning( - f"*** Step '{step_name}' in '{step_dir}' exited with a MERLIN_RETRY command, but has already reached its retry limit ({self.max_retries}). Continuing with workflow." + f"""*** Step '{step_name}' in '{step_dir}' exited with a MERLIN_RETRY command, + but has already reached its retry limit ({self.max_retries}). Continuing with workflow.""" ) result = ReturnCode.SOFT_FAIL elif result == ReturnCode.SOFT_FAIL: @@ -226,9 +237,9 @@ def prepare_chain_workspace(sample_index, chain_): bind=True, autoretry_for=retry_exceptions, retry_backoff=True, - priority=get_priority(Priority.low), + priority=get_priority(Priority.LOW), ) -def add_merlin_expanded_chain_to_chord( +def add_merlin_expanded_chain_to_chord( # pylint: disable=R0913,R0914 self, task_type, chain_, @@ -364,14 +375,14 @@ def add_chains_to_chord(self, all_chains): # generates a new task signature, so we need to make # sure we are modifying task signatures before adding them to the # kwargs. - for g in reversed(range(len(all_chains))): - if g < len(all_chains) - 1: + for j in reversed(range(len(all_chains))): + if j < len(all_chains) - 1: # fmt: off - new_kwargs = signature(all_chains[g][i]).kwargs.update( - {"next_in_chain": all_chains[g + 1][i]} + new_kwargs = signature(all_chains[j][i]).kwargs.update( + {"next_in_chain": all_chains[j + 1][i]} ) # fmt: on - all_chains[g][i] = all_chains[g][i].replace(kwargs=new_kwargs) + all_chains[j][i] = all_chains[j][i].replace(kwargs=new_kwargs) chain_steps.append(all_chains[0][i]) for sig in chain_steps: @@ -387,9 +398,9 @@ def add_chains_to_chord(self, all_chains): bind=True, autoretry_for=retry_exceptions, retry_backoff=True, - priority=get_priority(Priority.low), + priority=get_priority(Priority.LOW), ) -def expand_tasks_with_samples( +def expand_tasks_with_samples( # pylint: disable=R0913,R0914 self, dag, chain_, @@ -398,7 +409,6 @@ def expand_tasks_with_samples( task_type, adapter_config, level_max_dirs, - **kwargs, ): """ Generate a group of celery chains of tasks from a chain of task names, using merlin @@ -492,6 +502,7 @@ def expand_tasks_with_samples( LOG.debug("simple chain task queued") +# Pylint complains that "self" is unused but it's needed behind the scenes with celery @shared_task( bind=True, autoretry_for=retry_exceptions, @@ -499,9 +510,9 @@ def expand_tasks_with_samples( acks_late=False, reject_on_worker_lost=False, name="merlin:shutdown_workers", - priority=get_priority(Priority.high), + priority=get_priority(Priority.HIGH), ) -def shutdown_workers(self, shutdown_queues): +def shutdown_workers(self, shutdown_queues): # pylint: disable=W0613 """ This task issues a call to shutdown workers. @@ -518,13 +529,15 @@ def shutdown_workers(self, shutdown_queues): return stop_workers("celery", None, shutdown_queues, None) +# Pylint complains that these args are unused but celery passes args +# here behind the scenes and won't work if these aren't here @shared_task( autoretry_for=retry_exceptions, retry_backoff=True, name="merlin:chordfinisher", - priority=get_priority(Priority.low), + priority=get_priority(Priority.LOW), ) -def chordfinisher(*args, **kwargs): +def chordfinisher(*args, **kwargs): # pylint: disable=W0613 """. It turns out that chain(group,group) in celery does not execute one group after another, but executes the groups as if they were independent from @@ -539,7 +552,7 @@ def chordfinisher(*args, **kwargs): autoretry_for=retry_exceptions, retry_backoff=True, name="merlin:queue_merlin_study", - priority=get_priority(Priority.low), + priority=get_priority(Priority.LOW), ) def queue_merlin_study(study, adapter): """ diff --git a/merlin/config/__init__.py b/merlin/config/__init__.py index 08a0362ae..24e4ed5c1 100644 --- a/merlin/config/__init__.py +++ b/merlin/config/__init__.py @@ -38,7 +38,10 @@ from merlin.utils import nested_dict_to_namespaces -class Config: +# Pylint complains that there's too few methods here but this class might +# be useful if we ever need to do extra stuff with the configuration so we'll +# ignore it for now +class Config: # pylint: disable=R0903 """ The Config class, meant to store all Merlin config settings in one place. Regardless of the config data loading method, this class is meant to diff --git a/merlin/config/broker.py b/merlin/config/broker.py index ea26edc8f..0bcb3c42f 100644 --- a/merlin/config/broker.py +++ b/merlin/config/broker.py @@ -58,17 +58,16 @@ def read_file(filepath): "Safe file read from filepath" - with open(filepath, "r") as f: + with open(filepath, "r") as f: # pylint: disable=C0103 line = f.readline().strip() return quote(line, safe="") -def get_rabbit_connection(config_path, include_password, conn="amqps"): +def get_rabbit_connection(include_password, conn="amqps"): """ Given the path to the directory where the broker configurations are stored setup and return the RabbitMQ connection string. - :param config_path : The path for ssl certificates and passwords :param include_password : Format the connection for ouput by setting this True """ LOG.debug(f"Broker: connection = {conn}") @@ -86,13 +85,13 @@ def get_rabbit_connection(config_path, include_password, conn="amqps"): password_filepath = CONFIG.broker.password LOG.debug(f"Broker: password filepath = {password_filepath}") password_filepath = os.path.abspath(expanduser(password_filepath)) - except KeyError: - raise ValueError("Broker: No password provided for RabbitMQ") + except KeyError as e: # pylint: disable=C0103 + raise ValueError("Broker: No password provided for RabbitMQ") from e try: password = read_file(password_filepath) - except IOError: - raise ValueError(f"Broker: RabbitMQ password file {password_filepath} does not exist") + except IOError as e: # pylint: disable=C0103 + raise ValueError(f"Broker: RabbitMQ password file {password_filepath} does not exist") from e try: port = CONFIG.broker.port @@ -120,13 +119,10 @@ def get_rabbit_connection(config_path, include_password, conn="amqps"): return RABBITMQ_CONNECTION.format(**rabbitmq_config) -def get_redissock_connection(config_path, include_password): +def get_redissock_connection(): """ Given the path to the directory where the broker configurations are stored setup and return the redis+socket connection string. - - :param config_path : The path for ssl certificates and passwords - :param include_password : Format the connection for ouput by setting this True """ try: db_num = CONFIG.broker.db_num @@ -141,18 +137,17 @@ def get_redissock_connection(config_path, include_password): # flake8 complains this function is too complex, we don't gain much nesting any of this as a separate function, # however, cyclomatic complexity examination is off to get around this -def get_redis_connection(config_path, include_password, ssl=False): # noqa C901 +def get_redis_connection(include_password, use_ssl=False): # noqa C901 """ Return the redis or rediss specific connection - :param config_path : The path for ssl certificates and passwords :param include_password : Format the connection for ouput by setting this True - :param ssl : Flag to use rediss output + :param use_ssl : Flag to use rediss output """ server = CONFIG.broker.server LOG.debug(f"Broker: server = {server}") - urlbase = "rediss" if ssl else "redis" + urlbase = "rediss" if use_ssl else "redis" try: port = CONFIG.broker.port @@ -179,9 +174,9 @@ def get_redis_connection(config_path, include_password, ssl=False): # noqa C901 except IOError: password = CONFIG.broker.password if include_password: - spass = "%s:%s@" % (username, password) + spass = f"{username}:{password}@" else: - spass = "%s:%s@" % (username, "******") + spass = f"{username}:******@" except (AttributeError, KeyError): spass = "" LOG.debug(f"Broker: redis using default password = {spass}") @@ -218,25 +213,24 @@ def get_connection_string(include_password=True): if broker not in BROKERS: raise ValueError(f"Error: {broker} is not a supported broker.") - else: - return _sort_valid_broker(broker, config_path, include_password) + return _sort_valid_broker(broker, include_password) -def _sort_valid_broker(broker, config_path, include_password): - if broker == "rabbitmq" or broker == "amqps": - return get_rabbit_connection(config_path, include_password, conn="amqps") +def _sort_valid_broker(broker, include_password): + if broker in ("rabbitmq", "amqps"): + return get_rabbit_connection(include_password, conn="amqps") - elif broker == "amqp": - return get_rabbit_connection(config_path, include_password, conn="amqp") + if broker == "amqp": + return get_rabbit_connection(include_password, conn="amqp") - elif broker == "redis+socket": - return get_redissock_connection(config_path, include_password) + if broker == "redis+socket": + return get_redissock_connection() - elif broker == "redis": - return get_redis_connection(config_path, include_password) + if broker == "redis": + return get_redis_connection(include_password) # broker must be rediss - return get_redis_connection(config_path, include_password, ssl=True) + return get_redis_connection(include_password, use_ssl=True) def get_ssl_config() -> Union[bool, Dict[str, Union[str, ssl.VerifyMode]]]: @@ -276,7 +270,7 @@ def get_ssl_config() -> Union[bool, Dict[str, Union[str, ssl.VerifyMode]]]: if not broker_ssl: broker_ssl = True - if broker == "rabbitmq" or broker == "rediss" or broker == "amqps": + if broker in ("rabbitmq", "rediss", "amqps"): return broker_ssl return False diff --git a/merlin/config/configfile.py b/merlin/config/configfile.py index bb7f79875..fc4743b86 100644 --- a/merlin/config/configfile.py +++ b/merlin/config/configfile.py @@ -36,7 +36,7 @@ import logging import os import ssl -from typing import Dict, List, Optional, Union +from typing import Dict, Optional, Union from merlin.config import Config from merlin.utils import load_yaml @@ -60,9 +60,9 @@ def load_config(filepath): """ if not os.path.isfile(filepath): LOG.info(f"No app config file at {filepath}") - else: - LOG.info(f"Reading app config from file {filepath}") - return load_yaml(filepath) + return None + LOG.info(f"Reading app config from file {filepath}") + return load_yaml(filepath) def find_config_file(path=None): @@ -78,10 +78,9 @@ def find_config_file(path=None): if os.path.isfile(local_app): return local_app - elif os.path.isfile(path_app): + if os.path.isfile(path_app): return path_app - else: - return None + return None app_path = os.path.join(path, APP_FILENAME) if os.path.exists(app_path): @@ -133,6 +132,7 @@ def get_config(path: Optional[str]) -> Dict: def load_default_celery(config): + """Creates the celery default configuration""" try: config["celery"] except KeyError: @@ -152,6 +152,7 @@ def load_default_celery(config): def load_defaults(config): + """Loads default configuration values""" load_default_user_names(config) load_default_celery(config) @@ -247,7 +248,7 @@ def get_ssl_entries( except (AttributeError, KeyError): LOG.debug(f"{server_type}: ssl ssl_protocol not present") - if server_ssl and "cert_reqs" not in server_ssl.keys(): + if server_ssl and "cert_reqs" not in server_ssl: server_ssl["cert_reqs"] = ssl.CERT_REQUIRED ssl_map: Dict[str, str] = process_ssl_map(server_name) @@ -290,14 +291,12 @@ def merge_sslmap(server_ssl: Dict[str, Union[str, ssl.VerifyMode]], ssl_map: Dic : param ssl_map : the dict holding special key:value pairs for rediss and mysql """ new_server_ssl: Dict[str, Union[str, ssl.VerifyMode]] = {} - sk: List[str] = server_ssl.keys() - smk: List[str] = ssl_map.keys() - k: str - for k in sk: - if k in smk: - new_server_ssl[ssl_map[k]] = server_ssl[k] + + for key in server_ssl: + if key in ssl_map: + new_server_ssl[ssl_map[key]] = server_ssl[key] else: - new_server_ssl[k] = server_ssl[k] + new_server_ssl[key] = server_ssl[key] return new_server_ssl diff --git a/merlin/config/results_backend.py b/merlin/config/results_backend.py index 40ea19af7..3a03b3d79 100644 --- a/merlin/config/results_backend.py +++ b/merlin/config/results_backend.py @@ -100,7 +100,7 @@ def get_backend_password(password_file, certs_path=None): # The password was given instead of the filepath. password = password_file.strip() else: - with open(password_filepath, "r") as f: + with open(password_filepath, "r") as f: # pylint: disable=C0103 line = f.readline().strip() password = quote(line, safe="") @@ -151,9 +151,9 @@ def get_redis(certs_path=None, include_password=True, ssl=False): # noqa C901 password = CONFIG.results_backend.password if include_password: - spass = "%s:%s@" % (username, password) + spass = f"{username}:{password}@" else: - spass = "%s:%s@" % (username, "******") + spass = f"{username}:******@" except (KeyError, AttributeError): spass = "" LOG.debug(f"Results backend: redis using default password = {spass}") @@ -183,7 +183,7 @@ def get_mysql_config(certs_path, mysql_certs): certs = {} for key, filename in mysql_certs.items(): - for f in files: + for f in files: # pylint: disable=C0103 if not f == filename: continue @@ -215,7 +215,7 @@ def get_mysql(certs_path=None, mysql_certs=None, include_password=True): if not server: msg = f"Results backend: server {server} does not have a configuration" - raise Exception(msg) + raise TypeError(msg) # TypeError since server is None and not str password = get_backend_password(password_file, certs_path=certs_path) @@ -225,8 +225,9 @@ def get_mysql(certs_path=None, mysql_certs=None, include_password=True): mysql_config = get_mysql_config(certs_path, mysql_certs) if not mysql_config: - msg = f"The connection information for MySQL could not be set, cannot find:\n {mysql_certs}\ncheck the celery/certs path or set the ssl information in the app.yaml file." - raise Exception(msg) + msg = f"""The connection information for MySQL could not be set, cannot find:\n + {mysql_certs}\ncheck the celery/certs path or set the ssl information in the app.yaml file.""" + raise TypeError(msg) # TypeError since mysql_config is None when it shouldn't be mysql_config["user"] = CONFIG.results_backend.username if include_password: @@ -275,16 +276,16 @@ def _resolve_backend_string(backend, certs_path, include_password): if "mysql" in backend: return get_mysql(certs_path=certs_path, include_password=include_password) - elif "sqlite" in backend: + if "sqlite" in backend: return SQLITE_CONNECTION_STRING - elif backend == "redis": + if backend == "redis": return get_redis(certs_path=certs_path, include_password=include_password) - elif backend == "rediss": + if backend == "rediss": return get_redis(certs_path=certs_path, include_password=include_password, ssl=True) - else: - return None + + return None def get_ssl_config(celery_check=False): diff --git a/merlin/config/utils.py b/merlin/config/utils.py index 3cce32883..8d85ccf50 100644 --- a/merlin/config/utils.py +++ b/merlin/config/utils.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module contains priority handling""" import enum from typing import List @@ -35,34 +36,43 @@ class Priority(enum.Enum): - high = 1 - mid = 2 - low = 3 + """Enumerated Priorities""" + + HIGH = 1 + MID = 2 + LOW = 3 def is_rabbit_broker(broker: str) -> bool: + """Check if the broker is a rabbit server""" return broker in ["rabbitmq", "amqps", "amqp"] def is_redis_broker(broker: str) -> bool: + """Check if the broker is a redis server""" return broker in ["redis", "rediss", "redis+socket"] def get_priority(priority: Priority) -> int: + """ + Get the priority based on the broker. For a rabbit broker + a low priority is 1 and high is 10. For redis it's the opposite. + :returns: An int representing the priority level + """ broker: str = CONFIG.broker.name.lower() - priorities: List[Priority] = [Priority.high, Priority.mid, Priority.low] + priorities: List[Priority] = [Priority.HIGH, Priority.MID, Priority.LOW] if not isinstance(priority, Priority): raise TypeError(f"Unrecognized priority '{priority}'! Priority enum options: {[x.name for x in priorities]}") - if priority == Priority.mid: + if priority == Priority.MID: return 5 if is_rabbit_broker(broker): - if priority == Priority.low: + if priority == Priority.LOW: return 1 - if priority == Priority.high: + if priority == Priority.HIGH: return 10 if is_redis_broker(broker): - if priority == Priority.low: + if priority == Priority.LOW: return 10 - if priority == Priority.high: + if priority == Priority.HIGH: return 1 raise ValueError(f"Function get_priority has reached unknown state! Maybe unsupported broker {broker}?") diff --git a/merlin/display.py b/merlin/display.py index 0e0e11e66..cc05fd8ea 100644 --- a/merlin/display.py +++ b/merlin/display.py @@ -127,7 +127,7 @@ def _examine_connection(server, sconf, excpts): counter += 1 if counter > connect_timeout: conn_check.kill() - raise TimeoutError(f"Connection was killed due to timeout ({connect_timeout}server)") + raise TimeoutError(f"Connection was killed due to timeout ({connect_timeout}s)") conn.release() if conn_check.exception: error, _ = conn_check.exception @@ -195,6 +195,7 @@ def display_multiple_configs(files, configs): pprint.pprint(config) +# Might use args here in the future so we'll disable the pylint warning for now def print_info(args): # pylint: disable=W0613 """ Provide version and location information about python and pip to diff --git a/merlin/examples/dev_workflows/multiple_workers.yaml b/merlin/examples/dev_workflows/multiple_workers.yaml index f393f87d3..8785d9e9a 100644 --- a/merlin/examples/dev_workflows/multiple_workers.yaml +++ b/merlin/examples/dev_workflows/multiple_workers.yaml @@ -53,4 +53,4 @@ merlin: steps: [step_2] other_merlin_test_worker: args: -l INFO - steps: [step_3, step_4] \ No newline at end of file + steps: [step_3, step_4] diff --git a/merlin/examples/examples.py b/merlin/examples/examples.py index 7fbd502b1..bfd65a6a8 100644 --- a/merlin/examples/examples.py +++ b/merlin/examples/examples.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module contains example spec files with explanations of each block""" # Taken from https://lc.llnl.gov/mlsi/docs/merlin/merlin_config.html TEMPLATE_FILE_CONTENTS = """ diff --git a/merlin/examples/generator.py b/merlin/examples/generator.py index c97cc2757..3145b65e3 100644 --- a/merlin/examples/generator.py +++ b/merlin/examples/generator.py @@ -50,13 +50,15 @@ def gather_example_dirs(): + """Get all the example directories""" result = {} - for d in os.listdir(EXAMPLES_DIR): - result[d] = d + for directory in os.listdir(EXAMPLES_DIR): + result[directory] = directory return result def gather_all_examples(): + """Get all the example yaml files""" path = os.path.join(os.path.join(EXAMPLES_DIR, ""), os.path.join("*", "*.yaml")) return glob.glob(path) @@ -81,11 +83,11 @@ def list_examples(): directory = os.path.join(os.path.join(EXAMPLES_DIR, example_dir), "") specs = glob.glob(directory + "*.yaml") for spec in specs: - with open(spec) as f: + with open(spec) as f: # pylint: disable=C0103 try: spec_metadata = yaml.safe_load(f)["description"] except KeyError: - LOG.warn(f"{spec} lacks required section 'description'") + LOG.warning(f"{spec} lacks required section 'description'") continue except TypeError: continue @@ -103,6 +105,7 @@ def setup_example(name, outdir): """Setup the given example.""" example = None spec_paths = gather_all_examples() + spec_path = None for spec_path in spec_paths: spec = os.path.basename(os.path.normpath(spec_path)).replace(".yaml", "") if name == spec: diff --git a/merlin/exceptions/__init__.py b/merlin/exceptions/__init__.py index 225e69d28..ea376d1b5 100644 --- a/merlin/exceptions/__init__.py +++ b/merlin/exceptions/__init__.py @@ -32,6 +32,10 @@ Module of all Merlin-specific exception types. """ +# Pylint complains that these exceptions are no different from Exception +# but we don't care, we just need new names for exceptions here +# pylint: disable=W0246 + __all__ = ( "RetryException", "SoftFailException", @@ -48,7 +52,7 @@ class RetryException(Exception): """ def __init__(self): - super(RetryException, self).__init__() + super().__init__() class SoftFailException(Exception): @@ -58,7 +62,7 @@ class SoftFailException(Exception): """ def __init__(self): - super(SoftFailException, self).__init__() + super().__init__() class HardFailException(Exception): @@ -68,7 +72,7 @@ class HardFailException(Exception): """ def __init__(self): - super(HardFailException, self).__init__() + super().__init__() class InvalidChainException(Exception): @@ -77,7 +81,7 @@ class InvalidChainException(Exception): """ def __init__(self): - super(InvalidChainException, self).__init__() + super().__init__() class RestartException(Exception): @@ -87,4 +91,4 @@ class RestartException(Exception): """ def __init__(self): - super(RestartException, self).__init__() + super().__init__() diff --git a/merlin/main.py b/merlin/main.py index 723fadd22..3a30df135 100644 --- a/merlin/main.py +++ b/merlin/main.py @@ -366,7 +366,9 @@ def process_server(args: Namespace): config_server(args) -def setup_argparse() -> None: +# Pylint complains that there's too many statements here and wants us +# to split the function up but that wouldn't make much sense so we ignore it +def setup_argparse() -> None: # pylint: disable=R0915 """ Setup argparse and any CLI options we want available via the package. """ diff --git a/merlin/merlin_templates.py b/merlin/merlin_templates.py index a1af0298b..0db33d22e 100644 --- a/merlin/merlin_templates.py +++ b/merlin/merlin_templates.py @@ -42,12 +42,14 @@ LOG = logging.getLogger("merlin-templates") DEFAULT_LOG_LEVEL = "ERROR" +# We disable all pylint errors in this file since this is deprecated anyways -def process_templates(args): + +def process_templates(args): # pylint: disable=W0613,C0116 LOG.error("The command `merlin-templates` has been deprecated in favor of `merlin example`.") -def setup_argparse(): +def setup_argparse(): # pylint: disable=C0116 parser = argparse.ArgumentParser( prog="Merlin Examples", description=banner_small, @@ -57,14 +59,14 @@ def setup_argparse(): return parser -def main(): +def main(): # pylint: disable=C0116 try: parser = setup_argparse() args = parser.parse_args() setup_logging(logger=LOG, log_level=DEFAULT_LOG_LEVEL, colors=True) args.func(args) sys.exit() - except Exception as ex: + except Exception as ex: # pylint: disable=W0718 print(ex) sys.exit(1) diff --git a/merlin/router.py b/merlin/router.py index ab4b8e933..8e8c85bed 100644 --- a/merlin/router.py +++ b/merlin/router.py @@ -60,6 +60,10 @@ LOG = logging.getLogger(__name__) +# TODO go through this file and find a way to make a common return format to main.py +# Also, if that doesn't fix them, look into the pylint errors that have been disabled +# and try to resolve them + def run_task_server(study, run_mode=None): """ @@ -202,15 +206,6 @@ def stop_workers(task_server, spec_worker_names, queues, workers_regex): LOG.error("Celery is not specified as the task server!") -def route_for_task(name, args, kwargs, options, task=None, **kw): # pylint: disable=W0613,R1710 - """ - Custom task router for queues - """ - if ":" in name: - queue, _ = name.split(":") - return {"queue": queue} - - def create_config(task_server: str, config_dir: str, broker: str, test: str) -> None: """ Create a config for the given task server. diff --git a/merlin/server/server_commands.py b/merlin/server/server_commands.py index 045ef22e3..ecaa76b4e 100644 --- a/merlin/server/server_commands.py +++ b/merlin/server/server_commands.py @@ -70,7 +70,9 @@ def init_server() -> None: LOG.info("Merlin server initialization successful.") -def config_server(args: Namespace) -> None: +# Pylint complains that there's too many branches in this function but +# it looks clean to me so we'll ignore it +def config_server(args: Namespace) -> None: # pylint: disable=R0912 """ Process the merlin server config flags to make changes and edits to appropriate configurations based on the input passed in by the user. @@ -144,6 +146,8 @@ def config_server(args: Namespace) -> None: else: LOG.error(f"User '{args.remove_user}' doesn't exist within current users.") + return None + def status_server() -> None: """ @@ -162,20 +166,22 @@ def status_server() -> None: LOG.info("Merlin server is running.") -def start_server() -> bool: +def start_server() -> bool: # pylint: disable=R0911 """ Start a merlin server container using singularity. :return:: True if server was successful started and False if failed. """ current_status = get_server_status() - - if current_status == ServerStatus.NOT_INITALIZED or current_status == ServerStatus.MISSING_CONTAINER: - LOG.info("Merlin server has not been initialized. Please run 'merlin server init' first.") - return False - - if current_status == ServerStatus.RUNNING: - LOG.info("Merlin server already running.") - LOG.info("Stop current server with 'merlin server stop' before attempting to start a new server.") + uninitialized_err = "Merlin server has not been intitialized. Please run 'merlin server init' first." + status_errors = { + ServerStatus.NOT_INITALIZED: uninitialized_err, + ServerStatus.MISSING_CONTAINER: uninitialized_err, + ServerStatus.RUNNING: """Merlin server already running. + Stop current server with 'merlin server stop' before attempting to start a new server.""", + } + + if current_status in status_errors: + LOG.info(status_errors[current_status]) return False server_config = pull_server_config() @@ -184,16 +190,19 @@ def start_server() -> bool: return False image_path = server_config.container.get_image_path() - if not os.path.exists(image_path): - LOG.error("Unable to find image at " + image_path) - return False - config_path = server_config.container.get_config_path() - if not os.path.exists(config_path): - LOG.error("Unable to find config file at " + config_path) - return False - - process = subprocess.Popen( + path_errors = { + image_path: "image", + config_path: "config file", + } + + for path in (image_path, config_path): + if not os.path.exists(path): + LOG.error(f"Unable to find {path_errors[path]} at {path}") + return False + + # Pylint wants us to use with here but we don't need that + process = subprocess.Popen( # pylint: disable=R1732 server_config.container_format.get_run_command() .strip("\\") .format( @@ -237,9 +246,9 @@ def start_server() -> bool: redis_users.apply_to_redis(redis_config.get_ip_address(), redis_config.get_port(), redis_config.get_password()) new_app_yaml = os.path.join(server_config.container.get_config_dir(), "app.yaml") - ay = AppYaml() - ay.apply_server_config(server_config=server_config) - ay.write(new_app_yaml) + app_yaml = AppYaml() + app_yaml.apply_server_config(server_config=server_config) + app_yaml.write(new_app_yaml) LOG.info(f"New app.yaml written to {new_app_yaml}.") LOG.info("Replace app.yaml in ~/.merlin/app.yaml to use merlin server as main configuration.") LOG.info("To use for local runs, move app.yaml into the running directory.") diff --git a/merlin/server/server_config.py b/merlin/server/server_config.py index e62e12e4f..e433a4ebf 100644 --- a/merlin/server/server_config.py +++ b/merlin/server/server_config.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module represents everything that goes into server configuration""" import enum import logging @@ -52,7 +53,7 @@ try: - import importlib.resources as resources + from importlib import resources except ImportError: import importlib_resources as resources @@ -99,7 +100,7 @@ def generate_password(length, pass_command: str = None) -> str: random.shuffle(characters) password = [] - for i in range(length): + for _ in range(length): password.append(random.choice(characters)) random.shuffle(password) @@ -132,6 +133,8 @@ def parse_redis_output(redis_stdout: BufferedReader) -> Tuple[bool, str]: return False, line.decode("utf-8") line = redis_stdout.readline() + return False, "Reached end of redis output without seeing 'Ready to accept connections'" + def create_server_config() -> bool: """ @@ -142,7 +145,7 @@ def create_server_config() -> bool: :return:: True if success and False if fail """ if not os.path.exists(MERLIN_CONFIG_DIR): - LOG.error("Unable to find main merlin configuration directory at " + MERLIN_CONFIG_DIR) + LOG.error(f"Unable to find main merlin configuration directory at {MERLIN_CONFIG_DIR}") return False config_dir = os.path.join(MERLIN_CONFIG_DIR, MERLIN_SERVER_SUBDIR) @@ -172,7 +175,7 @@ def create_server_config() -> bool: # Load Merlin Server Configuration and apply it to app.yaml with resources.path("merlin.server", MERLIN_SERVER_CONFIG) as merlin_server_config: - with open(merlin_server_config) as f: + with open(merlin_server_config) as f: # pylint: disable=C0103 main_server_config = yaml.load(f, yaml.Loader) filename = LOCAL_APP_YAML if os.path.exists(LOCAL_APP_YAML) else AppYaml.default_filename merlin_app_yaml = AppYaml(filename) @@ -211,7 +214,7 @@ def config_merlin_server(): # else: password = generate_password(PASSWORD_LENGTH) - with open(pass_file, "w+") as f: + with open(pass_file, "w+") as f: # pylint: disable=C0103 f.write(password) LOG.info("Creating password file for merlin server container.") @@ -228,7 +231,9 @@ def config_merlin_server(): redis_users.write() redis_config.write() - LOG.info("User {} created in user file for merlin server container".format(os.environ.get("USER"))) + LOG.info(f"User {os.environ.get('USER')} created in user file for merlin server container") + + return None def pull_server_config() -> ServerConfig: @@ -251,7 +256,7 @@ def pull_server_config() -> ServerConfig: if "container" in server_config: if "format" in server_config["container"]: format_file = os.path.join(config_dir, server_config["container"]["format"] + ".yaml") - with open(format_file, "r") as ff: + with open(format_file, "r") as ff: # pylint: disable=C0103 format_data = yaml.load(ff, yaml.Loader) for key in format_needed_keys: if key not in format_data[server_config["container"]["format"]]: @@ -378,7 +383,7 @@ def pull_process_file(file_path: str) -> dict: if not returns None :return:: Data containing in process file. """ - with open(file_path, "r") as f: + with open(file_path, "r") as f: # pylint: disable=C0103 data = yaml.load(f, yaml.Loader) if check_process_file_format(data): return data @@ -392,6 +397,6 @@ def dump_process_file(data: dict, file_path: str): """ if not check_process_file_format(data): return False - with open(file_path, "w+") as f: + with open(file_path, "w+") as f: # pylint: disable=C0103 yaml.dump(data, f, yaml.Dumper) return True diff --git a/merlin/server/server_util.py b/merlin/server/server_util.py index 65f0b2abb..55c475f31 100644 --- a/merlin/server/server_util.py +++ b/merlin/server/server_util.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""Utils relating to merlin server""" import hashlib import logging @@ -47,7 +48,7 @@ MERLIN_SERVER_CONFIG = "merlin_server.yaml" -def valid_ipv4(ip: str) -> bool: +def valid_ipv4(ip: str) -> bool: # pylint: disable=C0103 """ Checks valid ip address """ @@ -69,12 +70,13 @@ def valid_port(port: int) -> bool: """ Checks valid network port """ - if port > 0 and port < 65536: + if 0 < port < 65536: return True return False -class ContainerConfig: +# Pylint complains about too many instance variables but it's necessary here so ignore +class ContainerConfig: # pylint: disable=R0902 """ ContainerConfig provides interface for parsing and interacting with the container value specified within the merlin_server.yaml configuration file. Dictionary of the config values should be passed when initialized @@ -120,50 +122,65 @@ def __init__(self, data: dict) -> None: self.user_file = data["user_file"] if "user_file" in data else self.USERS_FILE def get_format(self) -> str: + """Getter method to get the container format""" return self.format def get_image_type(self) -> str: + """Getter method to get the image type""" return self.image_type def get_image_name(self) -> str: + """Getter method to get the image name""" return self.image def get_image_url(self) -> str: + """Getter method to get the image url""" return self.url def get_image_path(self) -> str: + """Getter method to get the path to the image""" return os.path.join(self.config_dir, self.image) def get_config_name(self) -> str: + """Getter method to get the configuration file name""" return self.config def get_config_path(self) -> str: + """Getter method to get the configuration file path""" return os.path.join(self.config_dir, self.config) def get_config_dir(self) -> str: + """Getter method to get the configuration directory""" return self.config_dir def get_pfile_name(self) -> str: + """Getter method to get the process file name""" return self.pfile def get_pfile_path(self) -> str: + """Getter method to get the process file path""" return os.path.join(self.config_dir, self.pfile) def get_pass_file_name(self) -> str: + """Getter method to get the password file name""" return self.pass_file def get_pass_file_path(self) -> str: + """Getter method to get the password file path""" return os.path.join(self.config_dir, self.pass_file) def get_user_file_name(self) -> str: + """Getter method to get the user file name""" return self.user_file def get_user_file_path(self) -> str: + """Getter method to get the user file path""" return os.path.join(self.config_dir, self.user_file) def get_container_password(self) -> str: + """Getter method to get the container password""" password = None - with open(self.get_pass_file_path(), "r") as f: + with open(self.get_pass_file_path(), "r") as f: # pylint: disable=C0103 password = f.read() return password @@ -192,15 +209,19 @@ def __init__(self, data: dict) -> None: self.pull_command = data["pull_command"] if "pull_command" in data else self.PULL_COMMAND def get_command(self) -> str: + """Getter method to get the container command""" return self.command def get_run_command(self) -> str: + """Getter method to get the run command""" return self.run_command def get_stop_command(self) -> str: + """Getter method to get the stop command""" return self.stop_command def get_pull_command(self) -> str: + """Getter method to get the pull command""" return self.pull_command @@ -222,13 +243,17 @@ def __init__(self, data: dict) -> None: self.kill = data["kill"] if "kill" in data else self.KILL_COMMAND def get_status_command(self) -> str: + """Getter method to get the status command""" return self.status def get_kill_command(self) -> str: + """Getter method to get the kill command""" return self.kill -class ServerConfig: +# Pylint complains there's not enough methods here but this is essentially a wrapper for other +# classes so we can ignore it +class ServerConfig: # pylint: disable=R0903 """ ServerConfig is an interface for storing all the necessary configuration for merlin server. These configuration container things such as ContainerConfig, ProcessConfig, and ContainerFormatConfig. @@ -267,9 +292,10 @@ def __init__(self, filename) -> None: self.parse() def parse(self) -> None: + """Parses the redis configuration file""" self.entries = {} self.comments = {} - with open(self.filename, "r+") as f: + with open(self.filename, "r+") as f: # pylint: disable=C0103 file_contents = f.read() file_lines = file_contents.split("\n") comments = "" @@ -289,16 +315,19 @@ def parse(self) -> None: self.trailing_comments = comments[:-1] def write(self) -> None: - with open(self.filename, "w") as f: + """Writes to the redis configuration file""" + with open(self.filename, "w") as f: # pylint: disable=C0103 for entry in self.entry_order: f.write(self.comments[entry]) f.write(f"{entry} {self.entries[entry]}\n") f.write(self.trailing_comments) def set_filename(self, filename: str) -> None: + """Setter method to set the filename""" self.filename = filename def set_config_value(self, key: str, value: str) -> bool: + """Changes a configuration value""" if key not in self.entries: return False self.entries[key] = value @@ -306,17 +335,21 @@ def set_config_value(self, key: str, value: str) -> bool: return True def get_config_value(self, key: str) -> str: + """Given an entry in the config, get the value""" if key in self.entries: return self.entries[key] return None def changes_made(self) -> bool: + """Getter method to get the changes made""" return self.changed def get_ip_address(self) -> str: + """Getter method to get the ip from the redis config""" return self.get_config_value("bind") def set_ip_address(self, ipaddress: str) -> bool: + """Validates and sets a given ip address""" if ipaddress is None: return False # Check if ipaddress is valid @@ -332,9 +365,11 @@ def set_ip_address(self, ipaddress: str) -> bool: return True def get_port(self) -> str: + """Getter method to get the port from the redis config""" return self.get_config_value("port") def set_port(self, port: str) -> bool: + """Validates and sets a given port""" if port is None: return False # Check if port is valid @@ -350,6 +385,7 @@ def set_port(self, port: str) -> bool: return True def set_password(self, password: str) -> bool: + """Changes the password""" if password is None: return False self.set_config_value("requirepass", password) @@ -357,9 +393,14 @@ def set_password(self, password: str) -> bool: return True def get_password(self) -> str: + """Getter method to get the config password""" return self.get_config_value("requirepass") def set_directory(self, directory: str) -> bool: + """ + Sets the save directory in the redis config file. + Creates the directory if necessary. + """ if directory is None: return False if not os.path.exists(directory): @@ -378,6 +419,7 @@ def set_directory(self, directory: str) -> bool: return True def set_snapshot_seconds(self, seconds: int) -> bool: + """Sets the snapshot wait time""" if seconds is None: return False # Set the snapshot second in the redis config @@ -385,17 +427,19 @@ def set_snapshot_seconds(self, seconds: int) -> bool: if value is None: LOG.error("Unable to get exisiting parameter values for snapshot") return False - else: - value = value.split() - value[0] = str(seconds) - value = " ".join(value) - if not self.set_config_value("save", value): - LOG.error("Unable to set snapshot value seconds") - return False + + value = value.split() + value[0] = str(seconds) + value = " ".join(value) + if not self.set_config_value("save", value): + LOG.error("Unable to set snapshot value seconds") + return False + LOG.info(f"Snapshot wait time is set to {seconds} seconds") return True def set_snapshot_changes(self, changes: int) -> bool: + """Sets the snapshot threshold""" if changes is None: return False # Set the snapshot changes into the redis config @@ -403,17 +447,19 @@ def set_snapshot_changes(self, changes: int) -> bool: if value is None: LOG.error("Unable to get exisiting parameter values for snapshot") return False - else: - value = value.split() - value[1] = str(changes) - value = " ".join(value) - if not self.set_config_value("save", value): - LOG.error("Unable to set snapshot value seconds") - return False + + value = value.split() + value[1] = str(changes) + value = " ".join(value) + if not self.set_config_value("save", value): + LOG.error("Unable to set snapshot value seconds") + return False + LOG.info(f"Snapshot threshold is set to {changes} changes") return True def set_snapshot_file(self, file: str) -> bool: + """Sets the snapshot file""" if file is None: return False # Set the snapshot file in the redis config @@ -425,6 +471,7 @@ def set_snapshot_file(self, file: str) -> bool: return True def set_append_mode(self, mode: str) -> bool: + """Sets the append mode""" if mode is None: return False valid_modes = ["always", "everysec", "no"] @@ -443,6 +490,7 @@ def set_append_mode(self, mode: str) -> bool: return True def set_append_file(self, file: str) -> bool: + """Sets the append file""" if file is None: return False # Set the append file in the redis config @@ -461,13 +509,17 @@ class RedisUsers: """ class User: + """Embedded class to store user specific information""" + status = "on" hash_password = hashlib.sha256(b"password").hexdigest() keys = "*" channels = "*" commands = "@all" - def __init__(self, status="on", keys="*", channels="*", commands="@all", password=None) -> None: + def __init__( # pylint: disable=R0913 + self, status="on", keys="*", channels="*", commands="@all", password=None + ) -> None: self.status = status self.keys = keys self.channels = channels @@ -475,14 +527,20 @@ def __init__(self, status="on", keys="*", channels="*", commands="@all", passwor if password is not None: self.set_password(password) - def parse_dict(self, dict: dict) -> None: - self.status = dict["status"] - self.keys = dict["keys"] - self.channels = dict["channels"] - self.commands = dict["commands"] - self.hash_password = dict["hash_password"] + def parse_dict(self, dictionary: dict) -> None: + """ + Given a dict of user info, parse the dict and store + the values as class attributes. + :param `dictionary`: The dict to parse + """ + self.status = dictionary["status"] + self.keys = dictionary["keys"] + self.channels = dictionary["channels"] + self.commands = dictionary["commands"] + self.hash_password = dictionary["hash_password"] def get_user_dict(self) -> dict: + """Getter method to get the user info""" self.status = "on" return { "status": self.status, @@ -493,12 +551,15 @@ def get_user_dict(self) -> dict: } def __repr__(self) -> str: + """Repr magic method for User class""" return str(self.get_user_dict()) def __str__(self) -> str: + """Str magic method for User class""" return self.__repr__() def set_password(self, password: str) -> None: + """Setter method to set the user's hash password""" self.hash_password = hashlib.sha256(bytes(password, "utf-8")).hexdigest() filename = "" @@ -510,7 +571,8 @@ def __init__(self, filename) -> None: self.parse() def parse(self) -> None: - with open(self.filename, "r") as f: + """Parses the redis user configuration file""" + with open(self.filename, "r") as f: # pylint: disable=C0103 self.users = yaml.load(f, yaml.Loader) for user in self.users: new_user = self.User() @@ -518,36 +580,44 @@ def parse(self) -> None: self.users[user] = new_user def write(self) -> None: + """Writes to the redis user configuration file""" data = self.users.copy() for key in data: data[key] = self.users[key].get_user_dict() - with open(self.filename, "w") as f: + with open(self.filename, "w") as f: # pylint: disable=C0103 yaml.dump(data, f, yaml.Dumper) - def add_user(self, user, status="on", keys="*", channels="*", commands="@all", password=None) -> bool: + def add_user( # pylint: disable=R0913 + self, user, status="on", keys="*", channels="*", commands="@all", password=None + ) -> bool: + """Add a user to the dict of Redis users""" if user in self.users: return False self.users[user] = self.User(status, keys, channels, commands, password) return True def set_password(self, user: str, password: str): + """Set the password for a specific user""" if user not in self.users: return False self.users[user].set_password(password) + return True def remove_user(self, user) -> bool: + """Remove a user from the dict of users""" if user in self.users: del self.users[user] return True return False def apply_to_redis(self, host: str, port: int, password: str) -> None: - db = redis.Redis(host=host, port=port, password=password) - current_users = db.acl_users() + """Apply the changes to users to redis""" + database = redis.Redis(host=host, port=port, password=password) + current_users = database.acl_users() for user in self.users: if user not in current_users: data = self.users[user] - db.acl_setuser( + database.acl_setuser( username=user, hashed_passwords=[f"+{data.hash_password}"], enabled=(data.status == "on"), @@ -558,7 +628,7 @@ def apply_to_redis(self, host: str, port: int, password: str) -> None: for user in current_users: if user not in self.users: - db.acl_deluser(user) + database.acl_deluser(user) class AppYaml: @@ -579,29 +649,34 @@ def __init__(self, filename: str = default_filename) -> None: self.read(filename) def apply_server_config(self, server_config: ServerConfig): - rc = RedisConfig(server_config.container.get_config_path()) + """Store the redis configuration""" + redis_config = RedisConfig(server_config.container.get_config_path()) self.data[self.broker_name]["name"] = server_config.container.get_image_type() self.data[self.broker_name]["username"] = "default" self.data[self.broker_name]["password"] = server_config.container.get_pass_file_path() - self.data[self.broker_name]["server"] = rc.get_ip_address() - self.data[self.broker_name]["port"] = rc.get_port() + self.data[self.broker_name]["server"] = redis_config.get_ip_address() + self.data[self.broker_name]["port"] = redis_config.get_port() self.data[self.results_name]["name"] = server_config.container.get_image_type() self.data[self.results_name]["username"] = "default" self.data[self.results_name]["password"] = server_config.container.get_pass_file_path() - self.data[self.results_name]["server"] = rc.get_ip_address() - self.data[self.results_name]["port"] = rc.get_port() + self.data[self.results_name]["server"] = redis_config.get_ip_address() + self.data[self.results_name]["port"] = redis_config.get_port() def update_data(self, new_data: dict): + """Update the data dict with new entries""" self.data.update(new_data) def get_data(self): + """Getter method to obtain the data""" return self.data def read(self, filename: str = default_filename): + """Load in a yaml file and save it to the data attribute""" self.data = merlin.utils.load_yaml(filename) def write(self, filename: str = default_filename): - with open(filename, "w+") as f: + """Given a filename, dump the data to the file""" + with open(filename, "w+") as f: # pylint: disable=C0103 yaml.dump(self.data, f, yaml.Dumper) diff --git a/merlin/spec/all_keys.py b/merlin/spec/all_keys.py index ac0031823..5da1fed22 100644 --- a/merlin/spec/all_keys.py +++ b/merlin/spec/all_keys.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module defines all the keys possible in each block of a merlin spec file""" DESCRIPTION = {"description", "name"} diff --git a/merlin/spec/defaults.py b/merlin/spec/defaults.py index 7b54e5200..a1794bed2 100644 --- a/merlin/spec/defaults.py +++ b/merlin/spec/defaults.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module defines the default values of every block in the merlin spec""" DESCRIPTION = {"description": {}} diff --git a/merlin/spec/expansion.py b/merlin/spec/expansion.py index f4ea42e24..a76c56c16 100644 --- a/merlin/spec/expansion.py +++ b/merlin/spec/expansion.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module handles expanding variables in the merlin spec""" import logging from collections import ChainMap @@ -123,10 +124,10 @@ def recurse(section): if isinstance(section, str): return expandvars(expanduser(section)) if isinstance(section, dict): - for k, v in section.items(): - if k in ["cmd", "restart"]: + for key, val in section.items(): + if key in ["cmd", "restart"]: continue - section[k] = recurse(v) + section[key] = recurse(val) elif isinstance(section, list): for i, elem in enumerate(deepcopy(section)): section[i] = recurse(elem) @@ -164,10 +165,10 @@ def determine_user_variables(*user_var_dicts): raise ValueError(f"Cannot reassign value of reserved word '{key}'! Reserved words are: {RESERVED}.") new_val = str(val) if contains_token(new_val): - for determined_key in determined_results.keys(): + for determined_key, determined_val in determined_results.items(): var_determined_key = var_ref(determined_key) if var_determined_key in new_val: - new_val = new_val.replace(var_determined_key, determined_results[determined_key]) + new_val = new_val.replace(var_determined_key, determined_val) new_val = expandvars(expanduser(new_val)) determined_results[key.upper()] = new_val return determined_results @@ -217,6 +218,9 @@ def parameter_substitutions_for_cmd(glob_path, sample_paths): return substitutions +# There's similar code inside study.py but the whole point of this function is to not use +# the MerlinStudy object so we disable this pylint error +# pylint: disable=duplicate-code def expand_spec_no_study(filepath, override_vars=None): """ Get the expanded text of a spec without creating @@ -239,6 +243,9 @@ def expand_spec_no_study(filepath, override_vars=None): return expand_by_line(spec_text, evaluated_uvars) +# pylint: enable=duplicate-code + + def get_spec_with_expansion(filepath, override_vars=None): """ Return a MerlinSpec with overrides and expansion, without diff --git a/merlin/spec/override.py b/merlin/spec/override.py index 4c9291693..2c3194b50 100644 --- a/merlin/spec/override.py +++ b/merlin/spec/override.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module handles overriding variables in a spec file via the CLI""" import logging from copy import deepcopy @@ -49,6 +50,7 @@ def error_override_vars(override_vars, spec_filepath): def replace_override_vars(env, override_vars): + """Replace override variables in the environment block""" if override_vars is None: return env result = deepcopy(env) diff --git a/merlin/spec/specification.py b/merlin/spec/specification.py index 65326d54f..45d456067 100644 --- a/merlin/spec/specification.py +++ b/merlin/spec/specification.py @@ -37,6 +37,7 @@ import logging import os import shlex +from copy import deepcopy from io import StringIO import yaml @@ -48,6 +49,7 @@ LOG = logging.getLogger(__name__) +# Pylint complains we have too many instance attributes but it's fine class MerlinSpec(YAMLSpecification): # pylint: disable=R0902 """ This class represents the logic for parsing the Merlin yaml @@ -67,8 +69,9 @@ class MerlinSpec(YAMLSpecification): # pylint: disable=R0902 column_labels: [X0, X1] """ + # Pylint says this call to super is useless but we'll leave it in case we want to add to __init__ in the future def __init__(self): # pylint: disable=W0246 - super(MerlinSpec, self).__init__() # pylint: disable=R1725 + super().__init__() @property def yaml_sections(self): @@ -123,19 +126,19 @@ def __str__(self): return result @classmethod - def load_specification(cls, filepath, suppress_warning=True): # pylint: disable=W0237 + def load_specification(cls, path, suppress_warning=True): """ Load in a spec file and create a MerlinSpec object based on its' contents. :param `cls`: The class reference (like self) - :param `filepath`: A path to the spec file we're loading in + :param `path`: A path to the spec file we're loading in :param `suppress_warning`: A bool representing whether to warn the user about unrecognized keys :returns: A MerlinSpec object """ - LOG.info("Loading specification from path: %s", filepath) + LOG.info("Loading specification from path: %s", path) try: - # Load the YAML spec from the filepath - with open(filepath, "r") as data: + # Load the YAML spec from the path + with open(path, "r") as data: spec = cls.load_spec_from_string(data, needs_IO=False, needs_verification=True) except Exception as e: # pylint: disable=C0103 LOG.exception(e.args) @@ -143,7 +146,7 @@ def load_specification(cls, filepath, suppress_warning=True): # pylint: disable # Path not set in _populate_spec because loading spec with string # does not have a path so we set it here - spec.path = filepath + spec.path = path spec.specroot = os.path.dirname(spec.path) # pylint: disable=W0201 if not suppress_warning: @@ -306,24 +309,23 @@ def verify_batch_block(self, schema): YAMLSpecification.validate_schema("batch", self.batch, schema) # Additional Walltime checks in case the regex from the schema bypasses an error - if "walltime" in self.batch: # pylint: disable=R1702 - if self.batch["type"] == "lsf": - LOG.warning("The walltime argument is not available in lsf.") - else: - try: - err_msg = "Walltime must be of the form SS, MM:SS, or HH:MM:SS." - walltime = self.batch["walltime"] - if len(walltime) > 2: - # Walltime must have : if it's not of the form SS - if ":" not in walltime: + if self.batch["type"] == "lsf" and "walltime" in self.batch: + LOG.warning("The walltime argument is not available in lsf.") + elif "walltime" in self.batch: + try: + err_msg = "Walltime must be of the form SS, MM:SS, or HH:MM:SS." + walltime = self.batch["walltime"] + if len(walltime) > 2: + # Walltime must have : if it's not of the form SS + if ":" not in walltime: + raise ValueError(err_msg) + # Walltime must have exactly 2 chars between : + time = walltime.split(":") + for section in time: + if len(section) != 2: raise ValueError(err_msg) - # Walltime must have exactly 2 chars between : - time = walltime.split(":") - for section in time: - if len(section) != 2: - raise ValueError(err_msg) - except Exception: # pylint: disable=W0706 - raise + except Exception: # pylint: disable=W0706 + raise @staticmethod def load_merlin_block(stream): @@ -407,13 +409,13 @@ def fill_missing_defaults(object_to_update, default_dict): existing ones. """ - def recurse(result, defaults): # pylint: disable=W0621 - if not isinstance(defaults, dict): + def recurse(result, recurse_defaults): + if not isinstance(recurse_defaults, dict): return - for key, val in defaults.items(): + for key, val in recurse_defaults.items(): # fmt: off if (key not in result) or ( - (result[key] is None) and (defaults[key] is not None) + (result[key] is None) and (recurse_defaults[key] is not None) ): result[key] = val else: @@ -454,9 +456,9 @@ def warn_unrecognized_keys(self): # user block is not checked @staticmethod - def check_section(section_name, section, all_keys): # pylint: disable=W0621 + def check_section(section_name, section, known_keys): """Checks a section of the spec file to see if there are any unrecognized keys""" - diff = set(section.keys()).difference(all_keys) + diff = set(section.keys()).difference(known_keys) # TODO: Maybe add a check here for required keys @@ -474,7 +476,7 @@ def dump(self): try: yaml.safe_load(result) except Exception as e: # pylint: disable=C0103 - raise ValueError(f"Error parsing provenance spec:\n{e}") # pylint: disable=W0707 + raise ValueError(f"Error parsing provenance spec:\n{e}") from e return result def _dict_to_yaml(self, obj, string, key_stack, tab): @@ -490,9 +492,11 @@ def _dict_to_yaml(self, obj, string, key_stack, tab): return self._process_string(obj, lvl, tab) if isinstance(obj, bool): return str(obj).lower() - if not isinstance(obj, (list, dict)): - return obj - return self._process_dict_or_list(obj, string, key_stack, lvl, tab) + if isinstance(obj, list): + return self._process_list(obj, string, key_stack, lvl, tab) + if isinstance(obj, dict): + return self._process_dict(obj, string, key_stack, lvl, tab) + return obj def _process_string(self, obj, lvl, tab): """ @@ -503,50 +507,51 @@ def _process_string(self, obj, lvl, tab): obj = "|\n" + tab * (lvl + 1) + ("\n" + tab * (lvl + 1)).join(split) return obj - def _process_dict_or_list(self, obj, string, key_stack, lvl, tab): # pylint: disable=R0912,R0913 + def _process_list(self, obj, string, key_stack, lvl, tab): # pylint: disable=R0913 """ - Processes lists and dicts for _dict_to_yaml() in the dump() method. + Processes lists for _dict_to_yaml() in the dump() method. """ - from copy import deepcopy # pylint: disable=C0415 + num_entries = len(obj) + use_hyphens = key_stack[-1] in ["paths", "sources", "git", "study"] or key_stack[0] in ["user"] + if not use_hyphens: + string += "[" + else: + string += "\n" + for i, elem in enumerate(obj): + key_stack = deepcopy(key_stack) + key_stack.append("elem") + if use_hyphens: + string += (lvl + 1) * tab + "- " + str(self._dict_to_yaml(elem, "", key_stack, tab)) + "\n" + else: + string += str(self._dict_to_yaml(elem, "", key_stack, tab)) + if num_entries > 1 and i != len(obj) - 1: + string += ", " + key_stack.pop() + if not use_hyphens: + string += "]" + return string + def _process_dict(self, obj, string, key_stack, lvl, tab): # pylint: disable=R0913 + """ + Processes dicts for _dict_to_yaml() in the dump() method + """ list_offset = 2 * " " - if isinstance(obj, list): - num_entries = len(obj) - use_hyphens = key_stack[-1] in ["paths", "sources", "git", "study"] or key_stack[0] in ["user"] - if not use_hyphens: - string += "[" + if len(key_stack) > 0 and key_stack[-1] != "elem": + string += "\n" + i = 0 + for key, val in obj.items(): + key_stack = deepcopy(key_stack) + key_stack.append(key) + if len(key_stack) > 1 and key_stack[-2] == "elem" and i == 0: + # string += (tab * (lvl - 1)) + string += "" + elif "elem" in key_stack: + string += list_offset + (tab * lvl) else: - string += "\n" - for i, elem in enumerate(obj): - key_stack = deepcopy(key_stack) - key_stack.append("elem") - if use_hyphens: - string += (lvl + 1) * tab + "- " + str(self._dict_to_yaml(elem, "", key_stack, tab)) + "\n" - else: - string += str(self._dict_to_yaml(elem, "", key_stack, tab)) - if num_entries > 1 and i != len(obj) - 1: - string += ", " - key_stack.pop() - if not use_hyphens: - string += "]" - # must be dict - else: - if len(key_stack) > 0 and key_stack[-1] != "elem": - string += "\n" - i = 0 - for key, val in obj.items(): - key_stack = deepcopy(key_stack) - key_stack.append(key) - if len(key_stack) > 1 and key_stack[-2] == "elem" and i == 0: - # string += (tab * (lvl - 1)) - string += "" - elif "elem" in key_stack: - string += list_offset + (tab * lvl) - else: - string += tab * (lvl + 1) - string += str(key) + ": " + str(self._dict_to_yaml(val, "", key_stack, tab)) + "\n" - key_stack.pop() - i += 1 + string += tab * (lvl + 1) + string += str(key) + ": " + str(self._dict_to_yaml(val, "", key_stack, tab)) + "\n" + key_stack.pop() + i += 1 return string def get_step_worker_map(self): diff --git a/merlin/study/celeryadapter.py b/merlin/study/celeryadapter.py index 81f0762f8..8daaa1fe0 100644 --- a/merlin/study/celeryadapter.py +++ b/merlin/study/celeryadapter.py @@ -51,8 +51,10 @@ def run_celery(study, run_mode=None): configure Celery to run locally (without workers). """ # Only import celery stuff if we want celery in charge + # Pylint complains about circular import between merlin.common.tasks -> merlin.router -> merlin.study.celeryadapter + # For now I think this is still the best way to do this so we'll ignore it from merlin.celery import app # pylint: disable=C0415 - from merlin.common.tasks import queue_merlin_study # pylint: disable=C0415 + from merlin.common.tasks import queue_merlin_study # pylint: disable=C0415, R0401 adapter_config = study.get_adapter_config(override_type="local") @@ -468,7 +470,7 @@ def purge_celery_tasks(queues, force): return subprocess.run(purge_command, shell=True).returncode -def stop_celery_workers(queues=None, spec_worker_names=None, worker_regex=None): +def stop_celery_workers(queues=None, spec_worker_names=None, worker_regex=None): # pylint: disable=R0912 """Send a stop command to celery workers. Default behavior is to stop all connected workers. diff --git a/merlin/study/dag.py b/merlin/study/dag.py index f7ba3532f..7281875f5 100644 --- a/merlin/study/dag.py +++ b/merlin/study/dag.py @@ -205,11 +205,9 @@ def find_independent_chains(self, list_of_groups_of_chains): if self.num_children(task_name) == 1 and task_name != "_source": child = self.children(task_name)[0] - if self.num_parents(child) == 1: - if self.compatible_merlin_expansion(child, task_name): - self.find_chain(child, list_of_groups_of_chains).remove(child) - - chain.append(child) + if self.num_parents(child) == 1 and self.compatible_merlin_expansion(child, task_name): + self.find_chain(child, list_of_groups_of_chains).remove(child) + chain.append(child) new_list = [[chain for chain in group if len(chain) > 0] for group in list_of_groups_of_chains] new_list_2 = [group for group in new_list if len(group) > 0] diff --git a/merlin/study/script_adapter.py b/merlin/study/script_adapter.py index 027ebaff9..ad6843c51 100644 --- a/merlin/study/script_adapter.py +++ b/merlin/study/script_adapter.py @@ -65,7 +65,7 @@ def __init__(self, **kwargs): :param **kwargs: A dictionary with default settings for the adapter. """ - super(MerlinLSFScriptAdapter, self).__init__(**kwargs) + super().__init__(**kwargs) self._cmd_flags: Dict[str, str] = { "cmd": "jsrun", @@ -99,6 +99,9 @@ def __init__(self, **kwargs): "walltime", } + def get_priority(self, priority): + """This is implemented to override the abstract method and fix a pylint error""" + def get_header(self, step): """ Generate the header present at the top of LSF execution scripts. @@ -107,7 +110,7 @@ def get_header(self, step): :returns: A string of the header based on internal batch parameters and the parameter step. """ - return "#!{}".format(self._exec) + return f"#!{self._exec}" def get_parallelize_command(self, procs, nodes=None, **kwargs): """ @@ -149,7 +152,7 @@ def get_parallelize_command(self, procs, nodes=None, **kwargs): LOG.warning("'%s' is not supported -- ommitted.", key) continue if value: - args += [self._cmd_flags[key], "{}".format(str(value))] + args += [self._cmd_flags[key], f"{str(value)}"] return " ".join(args) @@ -171,7 +174,7 @@ def __init__(self, **kwargs): :param **kwargs: A dictionary with default settings for the adapter. """ - super(MerlinSlurmScriptAdapter, self).__init__(**kwargs) + super().__init__(**kwargs) self._cmd_flags["slurm"] = "" self._cmd_flags["walltime"] = "-t" @@ -192,6 +195,9 @@ def __init__(self, **kwargs): ] self._unsupported: Set[str] = set(list(self._unsupported) + new_unsupported) + def get_priority(self, priority): + """This is implemented to override the abstract method and fix a pylint error""" + def get_header(self, step): """ Generate the header present at the top of Slurm execution scripts. @@ -200,7 +206,7 @@ def get_header(self, step): :returns: A string of the header based on internal batch parameters and the parameter step. """ - return "#!{}".format(self._exec) + return f"#!{self._exec}" def time_format(self, val): """ @@ -241,35 +247,16 @@ def get_parallelize_command(self, procs, nodes=None, **kwargs): if key == "walltime": args += [ self._cmd_flags[key], - "{}".format(str(self.time_format(value))), + f"{str(self.time_format(value))}", ] elif "=" in self._cmd_flags[key]: - args += ["{0}{1}".format(self._cmd_flags[key], str(value))] + args += [f"{self._cmd_flags[key]}{str(value)}"] else: - args += [self._cmd_flags[key], "{}".format(str(value))] + args += [self._cmd_flags[key], f"{str(value)}"] return " ".join(args) -class MerlinLSFSrunScriptAdapter(MerlinSlurmScriptAdapter): - """ - A SchedulerScriptAdapter class for lsf blocking parallel launches, using the srun wrapper - """ - - key = "merlin-lsf-srun" - - def __init__(self, **kwargs): - """ - Initialize an instance of the MerinLSFSrunScriptAdapter. - The MerlinLSFSrunScriptAdapter is the adapter that is used for workflows that - will execute LSF parallel jobs in a celery worker with an srun wrapper. The only - configurable aspect to this adapter is the shell that scripts are executed in. - - :param **kwargs: A dictionary with default settings for the adapter. - """ - super(MerlinLSFSrunScriptAdapter, self).__init__(**kwargs) - - class MerlinFluxScriptAdapter(MerlinSlurmScriptAdapter): """ A SchedulerScriptAdapter class for flux blocking parallel launches, @@ -288,7 +275,7 @@ def __init__(self, **kwargs): :param **kwargs: A dictionary with default settings for the adapter. """ flux_command = kwargs.pop("flux_command", "flux mini run") - super(MerlinFluxScriptAdapter, self).__init__(**kwargs) + super().__init__(**kwargs) # "cmd": "flux mini run", self._cmd_flags = { @@ -323,6 +310,9 @@ def __init__(self, **kwargs): ] self._unsupported = set(new_unsupported) # noqa + def get_priority(self, priority): + """This is implemented to override the abstract method and fix a pylint error""" + def time_format(self, val): """ Convert a time format to flux standard designation. @@ -346,19 +336,19 @@ def __init__(self, **kwargs): :param **kwargs: A dictionary with default settings for the adapter. """ - super(MerlinScriptAdapter, self).__init__(**kwargs) + super().__init__(**kwargs) self.batch_type = "merlin-" + kwargs.get("batch_type", "local") - if "host" not in kwargs.keys(): + if "host" not in kwargs: kwargs["host"] = "None" - if "bank" not in kwargs.keys(): + if "bank" not in kwargs: kwargs["bank"] = "None" - if "queue" not in kwargs.keys(): + if "queue" not in kwargs: kwargs["queue"] = "None" # Using super prevents recursion. - self.batch_adapter = super(MerlinScriptAdapter, self) + self.batch_adapter = super() if self.batch_type != "merlin-local": self.batch_adapter = MerlinScriptAdapterFactory.get_adapter(self.batch_type)(**kwargs) @@ -369,7 +359,8 @@ def write_script(self, *args, **kwargs): _, script, restart_script = self.batch_adapter.write_script(*args, **kwargs) return True, script, restart_script - def submit(self, step, path, cwd, job_map=None, env=None): + # Pylint complains that there's too many arguments but it's fine in this case + def submit(self, step, path, cwd, job_map=None, env=None): # pylint: disable=R0913 """ Execute the step locally. If cwd is specified, the submit method will operate outside of the path @@ -387,8 +378,8 @@ def submit(self, step, path, cwd, job_map=None, env=None): """ LOG.debug("cwd = %s", cwd) LOG.debug("Script to execute: %s", path) - LOG.debug("starting process %s in cwd %s called %s" % (path, cwd, step.name)) - submission_record = self._execute_subprocess(step.name, path, cwd, env, False) + LOG.debug(f"starting process {path} in cwd {cwd} called {step.name}") + submission_record = self._execute_subprocess(step.name, path, cwd, env=env, join_output=False) retcode = submission_record.return_code if retcode == ReturnCode.OK: LOG.debug("Execution returned status OK.") @@ -406,17 +397,21 @@ def submit(self, step, path, cwd, job_map=None, env=None): LOG.debug("Execution returned status STOP_WORKERS") else: LOG.warning(f"Unrecognized Merlin Return code: {retcode}, returning SOFT_FAIL") - submission_record._info["retcode"] = retcode + submission_record.add_info("retcode", retcode) retcode = ReturnCode.SOFT_FAIL # Currently, we use Maestro's execute method, which is returning the # submission code we want it to return the return code, so we are # setting it in here. - submission_record._subcode = retcode + # TODO: In the refactor/status branch we're overwriting Maestro's execute method (I think) so + # we should be able to change this (i.e. add code in the overridden execute and remove this line) + submission_record._subcode = retcode # pylint: disable=W0212 return submission_record - def _execute_subprocess(self, output_name, script_path, cwd, env=None, join_output=False): + # TODO is there currently ever a scenario where join output is True? We should look into this + # Pylint is complaining there's too many local variables and args but it makes this function cleaner so ignore + def _execute_subprocess(self, output_name, script_path, cwd, env=None, join_output=False): # pylint: disable=R0913,R0914 """ Execute the subprocess script locally. If cwd is specified, the submit method will operate outside of the path @@ -435,16 +430,14 @@ def _execute_subprocess(self, output_name, script_path, cwd, env=None, join_outp script_bn = os.path.basename(script_path) new_output_name = os.path.splitext(script_bn)[0] LOG.debug(f"script_path={script_path}, output_name={output_name}, new_output_name={new_output_name}") - p = start_process(script_path, shell=False, cwd=cwd, env=env) - pid = p.pid - output, err = p.communicate() - retcode = p.wait() + process = start_process(script_path, shell=False, cwd=cwd, env=env) + output, err = process.communicate() + retcode = process.wait() # This allows us to save on iNodes by not writing the output, # or by appending error to output if output_name is not None: - o_path = os.path.join(cwd, "{}.out".format(new_output_name)) - + o_path = os.path.join(cwd, f"{new_output_name}.out") with open(o_path, "a") as out: out.write(output) @@ -453,40 +446,42 @@ def _execute_subprocess(self, output_name, script_path, cwd, env=None, join_outp out.write(err) if not join_output: - e_path = os.path.join(cwd, "{}.err".format(new_output_name)) + e_path = os.path.join(cwd, f"{new_output_name}.err") with open(e_path, "a") as out: out.write(err) if retcode == 0: LOG.info("Execution returned status OK.") - return SubmissionRecord(ReturnCode.OK, retcode, pid) - else: - _record = SubmissionRecord(ReturnCode.ERROR, retcode, pid) - _record.add_info("stderr", str(err)) - return _record + return SubmissionRecord(ReturnCode.OK, retcode, process.pid) + + _record = SubmissionRecord(ReturnCode.ERROR, retcode, process.pid) + _record.add_info("stderr", str(err)) + return _record + +class MerlinScriptAdapterFactory: + """This class routes to the correct ScriptAdapter""" -class MerlinScriptAdapterFactory(object): factories = { "merlin-flux": MerlinFluxScriptAdapter, "merlin-lsf": MerlinLSFScriptAdapter, - "merlin-lsf-srun": MerlinLSFSrunScriptAdapter, + "merlin-lsf-srun": MerlinSlurmScriptAdapter, "merlin-slurm": MerlinSlurmScriptAdapter, "merlin-local": MerlinScriptAdapter, } @classmethod def get_adapter(cls, adapter_id): + """Returns the appropriate ScriptAdapter to use""" if adapter_id.lower() not in cls.factories: - msg = ( - "Adapter '{0}' not found. Specify an adapter that exists " - "or implement a new one mapping to the '{0}'".format(str(adapter_id)) - ) + msg = f"""Adapter '{str(adapter_id)}' not found. Specify an adapter that exists + or implement a new one mapping to the '{str(adapter_id)}'""" LOG.error(msg) - raise Exception(msg) + raise ValueError(msg) return cls.factories[adapter_id] @classmethod def get_valid_adapters(cls): + """Returns the valid ScriptAdapters""" return cls.factories.keys() diff --git a/merlin/study/step.py b/merlin/study/step.py index 031302480..b80dd2766 100644 --- a/merlin/study/step.py +++ b/merlin/study/step.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module represents all of the logic that goes into a step""" import logging import re @@ -78,7 +79,7 @@ def __init__(self, maestro_step_record): :param maestro_step_record: The StepRecord object. """ self.mstep = maestro_step_record - self.restart = False + self.__restart = False def get_cmd(self): """ @@ -133,7 +134,7 @@ def get_task_queue(self): @staticmethod def get_task_queue_from_dict(step_dict): """given a maestro step dict, get the task queue""" - from merlin.config.configfile import CONFIG + from merlin.config.configfile import CONFIG # pylint: disable=C0415 queue_tag = CONFIG.celery.queue_tag omit_tag = CONFIG.celery.omit_queue_tag @@ -153,6 +154,7 @@ def get_task_queue_from_dict(step_dict): @property def retry_delay(self): + """Returns the retry delay (default 1)""" default_retry_delay = 1 return self.mstep.step.__dict__["run"].get("retry_delay", default_retry_delay) @@ -163,20 +165,20 @@ def max_retries(self): """ return self.mstep.step.__dict__["run"]["max_retries"] - def __get_restart(self): + @property + def restart(self): """ - Set the restart property ensuring that restart is false + Get the restart property """ return self.__restart - def __set_restart(self, val): + @restart.setter + def restart(self, val): """ Set the restart property ensuring that restart is false """ self.__restart = val - restart = property(__get_restart, __set_restart) - def needs_merlin_expansion(self, labels): """ :return : True if the cmd has any of the default keywords or spec @@ -273,5 +275,5 @@ def execute(self, adapter_config): # calls to the step execute and restart functions. if self.restart and self.get_restart_cmd(): return ReturnCode(self.mstep.restart(adapter)) - else: - return ReturnCode(self.mstep.execute(adapter)) + + return ReturnCode(self.mstep.execute(adapter)) diff --git a/merlin/study/study.py b/merlin/study/study.py index 6bf07653f..9d016bc0c 100644 --- a/merlin/study/study.py +++ b/merlin/study/study.py @@ -27,6 +27,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. ############################################################################### +"""This module represents all of the logic for a study""" import logging import os @@ -53,7 +54,10 @@ LOG = logging.getLogger(__name__) -class MerlinStudy: +# TODO: see if there's any way to split this class up (pylint doesn't like how many attributes there are) +# - Might be able to create an object to store files and handle file modifications +# - If we don't want to create entirely new classes we could try grouping args into dicts +class MerlinStudy: # pylint: disable=R0902 """ Represents a Merlin study run on a specification. Used for 'merlin run'. @@ -68,7 +72,7 @@ class MerlinStudy: :param `no_errors`: Flag to ignore some errors for testing. """ - def __init__( + def __init__( # pylint: disable=R0913 self, filepath, override_vars=None, @@ -108,9 +112,7 @@ def __init__( "MERLIN_HARD_FAIL": str(int(ReturnCode.HARD_FAIL)), "MERLIN_RETRY": str(int(ReturnCode.RETRY)), # below will be substituted for sample values on execution - "MERLIN_SAMPLE_VECTOR": " ".join( - ["$({})".format(k) for k in self.get_sample_labels(from_spec=self.original_spec)] - ), + "MERLIN_SAMPLE_VECTOR": " ".join([f"$({k})" for k in self.get_sample_labels(from_spec=self.original_spec)]), "MERLIN_SAMPLE_NAMES": " ".join(self.get_sample_labels(from_spec=self.original_spec)), "MERLIN_SPEC_ORIGINAL_TEMPLATE": os.path.join( self.info, @@ -151,6 +153,9 @@ def label_clash_error(self): if label in self.original_spec.globals: raise ValueError(f"column_label {label} cannot also be in global.parameters!") + # There's similar code inside expansion.py but the whole point of the function inside that file is + # to not use the MerlinStudy object so we disable this pylint error + # pylint: disable=duplicate-code @staticmethod def get_user_vars(spec): """ @@ -164,8 +169,11 @@ def get_user_vars(spec): uvars.append(spec.environment["labels"]) return determine_user_variables(*uvars) + # pylint: enable=duplicate-code + @property def user_vars(self): + """Get the user defined variables""" return MerlinStudy.get_user_vars(self.original_spec) def get_expanded_spec(self): @@ -200,6 +208,7 @@ def samples(self): return [] def get_sample_labels(self, from_spec): + """Return the column labels of the samples (if any)""" if from_spec.merlin["samples"]: return from_spec.merlin["samples"]["column_labels"] return [] @@ -289,19 +298,18 @@ def output_path(self): raise ValueError(f"Restart dir '{self.restart_dir}' does not exist!") return os.path.abspath(output_path) - else: - output_path = str(self.original_spec.output_path) + output_path = str(self.original_spec.output_path) - if (self.override_vars is not None) and ("OUTPUT_PATH" in self.override_vars): - output_path = str(self.override_vars["OUTPUT_PATH"]) + if (self.override_vars is not None) and ("OUTPUT_PATH" in self.override_vars): + output_path = str(self.override_vars["OUTPUT_PATH"]) - output_path = expand_line(output_path, self.user_vars, env_vars=True) - output_path = os.path.abspath(output_path) - if not os.path.isdir(output_path): - os.makedirs(output_path) - LOG.info(f"Made dir(s) to output path '{output_path}'.") + output_path = expand_line(output_path, self.user_vars, env_vars=True) + output_path = os.path.abspath(output_path) + if not os.path.isdir(output_path): + os.makedirs(output_path) + LOG.info(f"Made dir(s) to output path '{output_path}'.") - return output_path + return output_path @cached_property def timestamp(self): @@ -313,8 +321,10 @@ def timestamp(self): return self.restart_dir.strip("/")[-15:] return time.strftime("%Y%m%d-%H%M%S") + # TODO look into why pylint complains that this method is hidden + # - might be because we reset self.workspace's value in the expanded_spec method @cached_property - def workspace(self): + def workspace(self): # pylint: disable=E0202 """ Determines, makes, and returns the path to this study's workspace directory. This directory holds workspace directories @@ -334,8 +344,10 @@ def workspace(self): return workspace + # TODO look into why pylint complains that this method is hidden + # - might be because we reset self.info's value in the expanded_spec method @cached_property - def info(self): + def info(self): # pylint: disable=E0202 """ Creates the 'merlin_info' directory inside this study's workspace directory. """ @@ -401,7 +413,7 @@ def expanded_spec(self): ) # write expanded spec for provenance - with open(expanded_filepath, "w") as f: + with open(expanded_filepath, "w") as f: # pylint: disable=C0103 f.write(result.dump()) # write original spec for provenance @@ -417,7 +429,7 @@ def expanded_spec(self): if "labels" in result.environment: partial_spec.environment["labels"] = result.environment["labels"] partial_spec_path = os.path.join(self.info, name + ".partial.yaml") - with open(partial_spec_path, "w") as f: + with open(partial_spec_path, "w") as f: # pylint: disable=C0103 f.write(partial_spec.dump()) LOG.info(f"Study workspace is '{self.workspace}'.") @@ -452,37 +464,39 @@ def generate_samples(self): if not os.path.exists(self.samples_file): sample_generate = self.expanded_spec.merlin["samples"]["generate"]["cmd"] LOG.info("Generating samples...") - sample_process = subprocess.Popen( + sample_process = subprocess.Popen( # pylint: disable=R1732 sample_generate, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True, ) stdout, stderr = sample_process.communicate() - with open(os.path.join(self.info, "cmd.sh"), "w") as f: + with open(os.path.join(self.info, "cmd.sh"), "w") as f: # pylint: disable=C0103 f.write(sample_generate) - with open(os.path.join(self.info, "cmd.out"), "wb") as f: + with open(os.path.join(self.info, "cmd.out"), "wb") as f: # pylint: disable=C0103 f.write(stdout) - with open(os.path.join(self.info, "cmd.err"), "wb") as f: + with open(os.path.join(self.info, "cmd.err"), "wb") as f: # pylint: disable=C0103 f.write(stderr) LOG.info("Generating samples complete!") return - except (IndexError, TypeError) as e: + except (IndexError, TypeError) as e: # pylint: disable=C0103 LOG.error(f"Could not generate samples:\n{e}") return def load_pgen(self, filepath, pargs, env): + """Creates a dict of variable names and values defined in a pgen script""" if filepath: if pargs is None: pargs = [] kwargs = create_dictionary(pargs) params = load_parameter_generator(filepath, env, kwargs) result = {} - for k, v in params.labels.items(): - result[k] = {"values": None, "label": v} - for k, v in params.parameters.items(): - result[k]["values"] = v + for key, val in params.labels.items(): + result[key] = {"values": None, "label": val} + for key, val in params.parameters.items(): + result[key]["values"] = val return result + return None def load_dag(self): """ @@ -526,6 +540,7 @@ def load_dag(self): self.dag = DAG(maestro_dag.adjacency_table, maestro_dag.values, labels) def get_adapter_config(self, override_type=None): + """Builds and returns the adapter configuration dictionary""" adapter_config = dict(self.expanded_spec.batch) if "type" not in adapter_config.keys(): diff --git a/merlin/utils.py b/merlin/utils.py index 48def3a10..dd8f69d29 100644 --- a/merlin/utils.py +++ b/merlin/utils.py @@ -78,8 +78,7 @@ def get_user_process_info(user=None, attrs=None): if user == "all_users": return [p.info for p in psutil.process_iter(attrs=attrs)] - else: - return [p.info for p in psutil.process_iter(attrs=attrs) if user in p.info["username"]] + return [p.info for p in psutil.process_iter(attrs=attrs) if user in p.info["username"]] def check_pid(pid, user=None): @@ -91,8 +90,8 @@ def check_pid(pid, user=None): all processes """ user_processes = get_user_process_info(user=user) - for p in user_processes: - if int(p["pid"]) == pid: + for process in user_processes: + if int(process["pid"]) == pid: return True return False @@ -149,12 +148,14 @@ def is_running(name, all_users=False): if all_users: cmd[1] = "aux" + # pylint: disable=consider-using-with try: - ps = subprocess.Popen(cmd, stdout=subprocess.PIPE, encoding="utf8").communicate()[0] + process_status = subprocess.Popen(cmd, stdout=subprocess.PIPE, encoding="utf8").communicate()[0] except TypeError: - ps = subprocess.Popen(cmd, stdout=subprocess.PIPE).communicate()[0] + process_status = subprocess.Popen(cmd, stdout=subprocess.PIPE).communicate()[0] + # pylint: enable=consider-using-with - if name in ps: + if name in process_status: return True return False @@ -178,7 +179,7 @@ def regex_list_filter(regex, list_to_filter, match=True): :return `new_list` """ - r = re.compile(regex) + r = re.compile(regex) # pylint: disable=C0103 if match: return list(filter(r.match, list_to_filter)) return list(filter(r.search, list_to_filter)) @@ -268,7 +269,7 @@ def determine_protocol(fname): @contextmanager -def cd(path): +def cd(path): # pylint: disable=C0103 """ TODO """ @@ -282,7 +283,7 @@ def cd(path): def pickle_data(filepath, content): """Dump content to a pickle file""" - with open(filepath, "w") as f: + with open(filepath, "w") as f: # pylint: disable=C0103 pickle.dump(content, f) @@ -343,22 +344,22 @@ def recurse(dic): return recurse(new_dic) -def nested_namespace_to_dicts(ns): +def nested_namespace_to_dicts(namespaces): """Code for recursively converting namespaces of namespaces into dictionaries instead. """ - def recurse(ns): - if not isinstance(ns, SimpleNamespace): - return ns - for key, val in list(ns.__dict__.items()): - setattr(ns, key, recurse(val)) - return ns.__dict__ + def recurse(namespaces): + if not isinstance(namespaces, SimpleNamespace): + return namespaces + for key, val in list(namespaces.__dict__.items()): + setattr(namespaces, key, recurse(val)) + return namespaces.__dict__ - if not isinstance(ns, SimpleNamespace): - raise TypeError(f"{ns} is not a SimpleNamespace") + if not isinstance(namespaces, SimpleNamespace): + raise TypeError(f"{namespaces} is not a SimpleNamespace") - new_ns = deepcopy(ns) + new_ns = deepcopy(namespaces) return recurse(new_ns) @@ -371,26 +372,25 @@ def get_flux_version(flux_path, no_errors=False): """ cmd = [flux_path, "version"] - ps = None + process = None try: - ps = subprocess.Popen(cmd, stdout=subprocess.PIPE, encoding="utf8").communicate() - except FileNotFoundError as e: + process = subprocess.Popen(cmd, stdout=subprocess.PIPE, encoding="utf8").communicate() # pylint: disable=R1732 + except FileNotFoundError as e: # pylint: disable=C0103 if not no_errors: LOG.error(f"The flux path {flux_path} canot be found") LOG.error("Suppress this error with no_errors=True") raise e try: - flux_ver = re.search(r"\s*([\d.]+)", ps[0]).group(1) - except (ValueError, TypeError) as e: + flux_ver = re.search(r"\s*([\d.]+)", process[0]).group(1) + except (ValueError, TypeError) as e: # pylint: disable=C0103 if not no_errors: LOG.error("The flux version cannot be determined") LOG.error("Suppress this error with no_errors=True") raise e - else: - flux_ver = DEFAULT_FLUX_VERSION - LOG.warning(f"Using syntax for default version: {flux_ver}") + flux_ver = DEFAULT_FLUX_VERSION + LOG.warning(f"Using syntax for default version: {flux_ver}") return flux_ver @@ -465,40 +465,39 @@ def convert_to_timedelta(timestr: Union[str, int]) -> timedelta: nfields = len(timestr.split(":")) if nfields > 4: raise ValueError(f"Cannot convert {timestr} to a timedelta. Valid format: days:hours:minutes:seconds.") - _, d, h, m, s = (":0" * 10 + timestr).rsplit(":", 4) + _, d, h, m, s = (":0" * 10 + timestr).rsplit(":", 4) # pylint: disable=C0103 tdelta = timedelta(days=int(d), hours=int(h), minutes=int(m), seconds=int(s)) return tdelta -def _repr_timedelta_HMS(td: timedelta) -> str: +def _repr_timedelta_HMS(time_delta: timedelta) -> str: # pylint: disable=C0103 """Represent a timedelta object as a string in hours:minutes:seconds""" - hours, remainder = divmod(td.total_seconds(), 3600) + hours, remainder = divmod(time_delta.total_seconds(), 3600) minutes, seconds = divmod(remainder, 60) hours, minutes, seconds = int(hours), int(minutes), int(seconds) return f"{hours:02d}:{minutes:02d}:{seconds:02d}" -def _repr_timedelta_FSD(td: timedelta) -> str: +def _repr_timedelta_FSD(time_delta: timedelta) -> str: # pylint: disable=C0103 """Represent a timedelta as a flux standard duration string, using seconds. flux standard duration (FSD) is a floating point number with a single character suffix: s,m,h or d. This uses seconds for simplicity. """ - fsd = f"{td.total_seconds()}s" + fsd = f"{time_delta.total_seconds()}s" return fsd -def repr_timedelta(td: timedelta, method: str = "HMS") -> str: +def repr_timedelta(time_delta: timedelta, method: str = "HMS") -> str: """Represent a timedelta object as a string using a particular method. method - HMS: 'hours:minutes:seconds' method - FSD: flux standard duration: 'seconds.s'""" if method == "HMS": - return _repr_timedelta_HMS(td) - elif method == "FSD": - return _repr_timedelta_FSD(td) - else: - raise ValueError("Invalid method for formatting timedelta! Valid choices: HMS, FSD") + return _repr_timedelta_HMS(time_delta) + if method == "FSD": + return _repr_timedelta_FSD(time_delta) + raise ValueError("Invalid method for formatting timedelta! Valid choices: HMS, FSD") def convert_timestring(timestring: Union[str, int], format_method: str = "HMS") -> str: diff --git a/setup.cfg b/setup.cfg index b81cb1afa..a000df59a 100644 --- a/setup.cfg +++ b/setup.cfg @@ -11,7 +11,6 @@ lines_after_imports=2 [flake8] ignore = E203, E266, E501, W503 -max-line-length = 127 max-complexity = 15 select = B,C,E,F,W,T4 exclude = .git,__pycache__,ascii_art.py,merlin/examples/*,*venv* @@ -19,6 +18,8 @@ exclude = .git,__pycache__,ascii_art.py,merlin/examples/*,*venv* [pylint.FORMAT] ignore=*venv* +disable=unspecified-encoding,subprocess-run-check +max-line-length = 127 [mypy] diff --git a/tests/integration/conditions.py b/tests/integration/conditions.py index 9c44e1f5f..b3acdde4e 100644 --- a/tests/integration/conditions.py +++ b/tests/integration/conditions.py @@ -50,7 +50,7 @@ def ingest_info(self, info): @abstractmethod def passes(self): """The method that will check if the test passes or not""" - pass + raise NotImplementedError("The 'passes' property should be defined in all Condition subclasses.") # pylint: disable=no-member @@ -142,6 +142,12 @@ def glob(self, glob_string): return sorted(candidates)[-1] return candidates + @property + @abstractmethod + def passes(self): + """The method that will check if the test passes or not""" + raise NotImplementedError("The 'passes' property should be defined in all StudyOutputAware subclasses.") + class StepFileExists(StudyOutputAware): """ diff --git a/tests/integration/run_tests.py b/tests/integration/run_tests.py index 0282f888b..effa9ca6f 100644 --- a/tests/integration/run_tests.py +++ b/tests/integration/run_tests.py @@ -39,8 +39,11 @@ from contextlib import suppress from subprocess import TimeoutExpired, run +# Pylint complains that we didn't install this module but it's defined locally so ignore from test_definitions import OUTPUT_DIR, define_tests # pylint: disable=E0401 +from merlin.display import tabulate_info + def get_definition_issues(test): """ @@ -84,8 +87,8 @@ def run_single_test(test): and information about the test for logging purposes. :param `test`: A dictionary that defines the test :returns: A tuple of type (bool, dict) where the bool - represents if the test passed and the dict - contains info about the test. + represents if the test passed and the dict + contains info about the test. """ # Parse the test definition commands = test.pop("cmds", None) @@ -212,6 +215,7 @@ def filter_tests_to_run(args, tests): return selective, n_to_run +# TODO split this function up so it's not as large (this will fix the pylint issue here too) def run_tests(args, tests): # pylint: disable=R0914 """ Run all inputted tests. @@ -310,8 +314,6 @@ def display_tests(tests): the --id flag. :param `tests`: A dict of tests (Dict) """ - from merlin.display import tabulate_info # pylint: disable=C0415 - test_names = list(tests.keys()) test_table = [(i + 1, test_names[i]) for i in range(len(test_names))] test_table.insert(0, ("ID", "Test Name")) diff --git a/tests/integration/test_definitions.py b/tests/integration/test_definitions.py index aafbabe2f..551056f95 100644 --- a/tests/integration/test_definitions.py +++ b/tests/integration/test_definitions.py @@ -40,6 +40,7 @@ } """ +# Pylint complains that we didn't install this module but it's defined locally so ignore from conditions import ( # pylint: disable=E0401 FileHasNoRegex, FileHasRegex,