From 4d88dd2eef712761eda012ceecdd834760e679a8 Mon Sep 17 00:00:00 2001 From: Jon Hagg Date: Thu, 22 Apr 2021 15:51:30 -0700 Subject: [PATCH 1/4] feat: os agnostic rm functionality --- powersimdata/data_access/data_access.py | 31 ++++++++++++++++--- ...t_transfer_data.py => test_data_access.py} | 0 powersimdata/scenario/delete.py | 12 ++----- powersimdata/scenario/move.py | 15 ++++----- powersimdata/utility/helpers.py | 12 +++---- powersimdata/utility/tests/test_helpers.py | 8 ++--- 6 files changed, 44 insertions(+), 34 deletions(-) rename powersimdata/data_access/tests/{test_transfer_data.py => test_data_access.py} (100%) diff --git a/powersimdata/data_access/data_access.py b/powersimdata/data_access/data_access.py index c51ffb502..2f1b2b283 100644 --- a/powersimdata/data_access/data_access.py +++ b/powersimdata/data_access/data_access.py @@ -1,3 +1,4 @@ +import glob import operator import os import posixpath @@ -46,15 +47,13 @@ def copy(self, src, dest, recursive=False, update=False): command = CommandBuilder.copy(src, dest, recursive, update) return self.execute_command(command) - def remove(self, target, recursive=False, force=False): + def remove(self, target, recursive=False): """Wrapper around rm command :param str target: path to remove :param bool recursive: delete directories recursively - :param bool force: remove without confirmation """ - command = CommandBuilder.remove(target, recursive, force) - return self.execute_command(command) + raise NotImplementedError def _exists(self, filepath): """Return whether the file exists @@ -196,6 +195,18 @@ def makedir(self, relative_path): target = os.path.join(self.root, relative_path) os.makedirs(target, exist_ok=True) + def remove(self, target, recursive=False): + """Remove target using rm semantics + + :param str target: path to remove + :param bool recursive: delete directories recursively + """ + if recursive: + target = os.path.join(target, "**") + files = glob.glob(target, recursive=recursive) + for f in files: + os.remove(f) + def execute_command(self, command): """Execute a command locally at the data access. @@ -431,6 +442,18 @@ def makedir(self, relative_path): if len(stderr.readlines()) != 0: raise IOError("Failed to create %s on server" % full_path) + def remove(self, target, recursive=False): + """Run rm command on server + + :param str target: path to remove + :param bool recursive: delete directories recursively + :raises IOError: if command generated stderr + """ + command = CommandBuilder.remove(target, recursive) + _, _, stderr = self.execute_command(command) + if len(stderr.readlines()) != 0: + raise IOError(f"Failed to delete target={target} on server") + def _exists(self, filepath): """Return whether the file exists diff --git a/powersimdata/data_access/tests/test_transfer_data.py b/powersimdata/data_access/tests/test_data_access.py similarity index 100% rename from powersimdata/data_access/tests/test_transfer_data.py rename to powersimdata/data_access/tests/test_data_access.py diff --git a/powersimdata/scenario/delete.py b/powersimdata/scenario/delete.py index d00cd9981..d77020a60 100644 --- a/powersimdata/scenario/delete.py +++ b/powersimdata/scenario/delete.py @@ -43,25 +43,19 @@ def delete_scenario(self): # Delete links to base profiles on server print("--> Deleting scenario input data on server") target = posixpath.join(self.path_config.input_dir(), wildcard) - _, _, stderr = self._data_access.remove(target, recursive=False, force=True) - if len(stderr.readlines()) != 0: - raise IOError("Failed to delete scenario input data on server") + self._data_access.remove(target, recursive=False) # Delete output profiles print("--> Deleting scenario output data on server") target = posixpath.join(self.path_config.output_dir(), wildcard) - _, _, stderr = self._data_access.remove(target, recursive=False, force=True) - if len(stderr.readlines()) != 0: - raise IOError("Failed to delete scenario output data on server") + self._data_access.remove(target, recursive=False) # Delete temporary folder enclosing simulation inputs print("--> Deleting temporary folder on server") tmp_dir = posixpath.join( self.path_config.execute_dir(), f"scenario_{scenario_id}" ) - _, _, stderr = self._data_access.remove(tmp_dir, recursive=True, force=True) - if len(stderr.readlines()) != 0: - raise IOError("Failed to delete temporary folder on server") + self._data_access.remove(tmp_dir, recursive=True) # Delete local files print("--> Deleting input and output data on local machine") diff --git a/powersimdata/scenario/move.py b/powersimdata/scenario/move.py index 657d8d827..344ff9dd7 100644 --- a/powersimdata/scenario/move.py +++ b/powersimdata/scenario/move.py @@ -67,36 +67,37 @@ def __init__(self, data_access, scenario_info): self._scenario_info = scenario_info self.backup_config = server_setup.PathConfig(server_setup.BACKUP_DATA_ROOT_DIR) self.server_config = server_setup.PathConfig(server_setup.DATA_ROOT_DIR) + self.scenario_id = self._scenario_info["id"] + self.wildcard = f"{self.scenario_id}_*" def move_input_data(self): """Moves input data.""" print("--> Moving scenario input data to backup disk") source = posixpath.join( self.server_config.input_dir(), - self._scenario_info["id"] + "_*", + self.wildcard, ) target = self.backup_config.input_dir() self._data_access.copy(source, target, update=True) - self._data_access.remove(source, recursive=True, force=True) + self._data_access.remove(source, recursive=False) def move_output_data(self): """Moves output data""" print("--> Moving scenario output data to backup disk") source = posixpath.join( self.server_config.output_dir(), - self._scenario_info["id"] + "_*", + self.wildcard, ) target = self.backup_config.output_dir() self._data_access.copy(source, target, update=True) - self._data_access.remove(source, recursive=True, force=True) + self._data_access.remove(source, recursive=False) def move_temporary_folder(self): """Moves temporary folder.""" print("--> Moving temporary folder to backup disk") source = posixpath.join( - self.server_config.execute_dir(), - "scenario_" + self._scenario_info["id"], + self.server_config.execute_dir(), "scenario_" + self.scenario_id ) target = self.backup_config.execute_dir() self._data_access.copy(source, target, recursive=True, update=True) - self._data_access.remove(source, recursive=True, force=True) + self._data_access.remove(source, recursive=True) diff --git a/powersimdata/utility/helpers.py b/powersimdata/utility/helpers.py index dc1752f59..13c450277 100644 --- a/powersimdata/utility/helpers.py +++ b/powersimdata/utility/helpers.py @@ -25,19 +25,15 @@ def copy(src, dest, recursive=False, update=False): return fr"\cp {flags} {src} {dest}" @staticmethod - def remove(target, recursive=False, force=False): + def remove(target, recursive=False): """Builds a rm command with some options :param str target: the path or file to be removed :param bool recursive: whether to pass -r option - :param bool force: whether to pass -f option """ - r_flag = "r" if recursive else "" - f_flag = "f" if force else "" - if recursive or force: - flags = f"-{r_flag}{f_flag}" - return f"rm {flags} {target}" - return f"rm {target}" + if recursive: + return f"rm -rf {target}" + return f"rm -f {target}" class MemoryCache: diff --git a/powersimdata/utility/tests/test_helpers.py b/powersimdata/utility/tests/test_helpers.py index d6cea1552..c90abbdb6 100644 --- a/powersimdata/utility/tests/test_helpers.py +++ b/powersimdata/utility/tests/test_helpers.py @@ -98,14 +98,10 @@ def test_copy_command(): def test_remove_command(): - expected = "rm target" + expected = "rm -f target" command = CommandBuilder.remove("target") assert expected == command - expected = "rm -r target" - command = CommandBuilder.remove("target", recursive=True) - assert expected == command - expected = "rm -rf target" - command = CommandBuilder.remove("target", recursive=True, force=True) + command = CommandBuilder.remove("target", recursive=True) assert expected == command From c32d3a48386d0d6652e8965eb007946c575572fa Mon Sep 17 00:00:00 2001 From: Jon Hagg Date: Thu, 22 Apr 2021 18:28:22 -0700 Subject: [PATCH 2/4] feat: add confirmation prompt for delete operations --- powersimdata/data_access/data_access.py | 25 +++++++++++++++++++++---- powersimdata/scenario/delete.py | 22 +++++++++++----------- powersimdata/scenario/move.py | 21 +++++++++++---------- 3 files changed, 43 insertions(+), 25 deletions(-) diff --git a/powersimdata/data_access/data_access.py b/powersimdata/data_access/data_access.py index 2f1b2b283..9466191a6 100644 --- a/powersimdata/data_access/data_access.py +++ b/powersimdata/data_access/data_access.py @@ -47,11 +47,12 @@ def copy(self, src, dest, recursive=False, update=False): command = CommandBuilder.copy(src, dest, recursive, update) return self.execute_command(command) - def remove(self, target, recursive=False): + def remove(self, target, recursive=False, confirm=True): """Wrapper around rm command :param str target: path to remove :param bool recursive: delete directories recursively + :param bool confirm: prompt before executing command """ raise NotImplementedError @@ -195,17 +196,26 @@ def makedir(self, relative_path): target = os.path.join(self.root, relative_path) os.makedirs(target, exist_ok=True) - def remove(self, target, recursive=False): + def remove(self, target, recursive=False, confirm=True): """Remove target using rm semantics :param str target: path to remove :param bool recursive: delete directories recursively + :param bool confirm: prompt before executing command """ - if recursive: + if recursive and "**" not in target: target = os.path.join(target, "**") files = glob.glob(target, recursive=recursive) + if confirm: + print("This will delete the following files:") + print(files) + confirmed = input("Proceed? [y/n] (default is 'n')") + if confirmed.lower() != "y": + print("Operation cancelled.") + return for f in files: os.remove(f) + print("--> Done!") def execute_command(self, command): """Execute a command locally at the data access. @@ -442,17 +452,24 @@ def makedir(self, relative_path): if len(stderr.readlines()) != 0: raise IOError("Failed to create %s on server" % full_path) - def remove(self, target, recursive=False): + def remove(self, target, recursive=False, confirm=True): """Run rm command on server :param str target: path to remove :param bool recursive: delete directories recursively + :param bool confirm: prompt before executing command :raises IOError: if command generated stderr """ command = CommandBuilder.remove(target, recursive) + if confirm: + confirmed = input(f"Execute '{command}'? [y/n] (default is 'n')") + if confirmed.lower() != "y": + print("Operation cancelled.") + return _, _, stderr = self.execute_command(command) if len(stderr.readlines()) != 0: raise IOError(f"Failed to delete target={target} on server") + print("--> Done!") def _exists(self, filepath): """Return whether the file exists diff --git a/powersimdata/scenario/delete.py b/powersimdata/scenario/delete.py index d77020a60..2f8fc0886 100644 --- a/powersimdata/scenario/delete.py +++ b/powersimdata/scenario/delete.py @@ -1,7 +1,7 @@ -import glob import os import posixpath +from powersimdata.data_access.data_access import LocalDataAccess from powersimdata.scenario.state import State from powersimdata.utility import server_setup @@ -30,8 +30,11 @@ def print_scenario_info(self): except AttributeError: print("Scenario has been deleted") - def delete_scenario(self): - """Deletes scenario on server.""" + def delete_scenario(self, confirm=True): + """Deletes scenario on server. + + :param bool confirm: prompt before each batch + """ # Delete entry in scenario list scenario_id = self._scenario_info["id"] @@ -43,27 +46,24 @@ def delete_scenario(self): # Delete links to base profiles on server print("--> Deleting scenario input data on server") target = posixpath.join(self.path_config.input_dir(), wildcard) - self._data_access.remove(target, recursive=False) + self._data_access.remove(target, recursive=False, confirm=confirm) # Delete output profiles print("--> Deleting scenario output data on server") target = posixpath.join(self.path_config.output_dir(), wildcard) - self._data_access.remove(target, recursive=False) + self._data_access.remove(target, recursive=False, confirm=confirm) # Delete temporary folder enclosing simulation inputs print("--> Deleting temporary folder on server") tmp_dir = posixpath.join( self.path_config.execute_dir(), f"scenario_{scenario_id}" ) - self._data_access.remove(tmp_dir, recursive=True) + self._data_access.remove(tmp_dir, recursive=True, confirm=confirm) # Delete local files print("--> Deleting input and output data on local machine") - local_file = glob.glob( - os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard) - ) - for f in local_file: - os.remove(f) + target = os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard) + LocalDataAccess().remove(target, recursive=True, confirm=confirm) # Delete attributes self._clean() diff --git a/powersimdata/scenario/move.py b/powersimdata/scenario/move.py index 344ff9dd7..e2916ed6c 100644 --- a/powersimdata/scenario/move.py +++ b/powersimdata/scenario/move.py @@ -25,10 +25,11 @@ def print_scenario_info(self): for key, val in self._scenario_info.items(): print("%s: %s" % (key, val)) - def move_scenario(self, target="disk"): + def move_scenario(self, target="disk", confirm=True): """Move scenario. :param str target: optional argument specifying the backup system. + :param bool confirm: prompt before deleting each batch of files """ if not isinstance(target, str): raise TypeError("string is expected for optional argument target") @@ -38,9 +39,9 @@ def move_scenario(self, target="disk"): backup = BackUpDisk(self._data_access, self._scenario_info) - backup.move_input_data() - backup.move_output_data() - backup.move_temporary_folder() + backup.move_input_data(confirm=confirm) + backup.move_output_data(confirm=confirm) + backup.move_temporary_folder(confirm=confirm) sid = self._scenario_info["id"] self._execute_list_manager.set_status(sid, "moved") @@ -70,7 +71,7 @@ def __init__(self, data_access, scenario_info): self.scenario_id = self._scenario_info["id"] self.wildcard = f"{self.scenario_id}_*" - def move_input_data(self): + def move_input_data(self, confirm=True): """Moves input data.""" print("--> Moving scenario input data to backup disk") source = posixpath.join( @@ -79,9 +80,9 @@ def move_input_data(self): ) target = self.backup_config.input_dir() self._data_access.copy(source, target, update=True) - self._data_access.remove(source, recursive=False) + self._data_access.remove(source, recursive=False, confirm=confirm) - def move_output_data(self): + def move_output_data(self, confirm=True): """Moves output data""" print("--> Moving scenario output data to backup disk") source = posixpath.join( @@ -90,9 +91,9 @@ def move_output_data(self): ) target = self.backup_config.output_dir() self._data_access.copy(source, target, update=True) - self._data_access.remove(source, recursive=False) + self._data_access.remove(source, recursive=False, confirm=confirm) - def move_temporary_folder(self): + def move_temporary_folder(self, confirm=True): """Moves temporary folder.""" print("--> Moving temporary folder to backup disk") source = posixpath.join( @@ -100,4 +101,4 @@ def move_temporary_folder(self): ) target = self.backup_config.execute_dir() self._data_access.copy(source, target, recursive=True, update=True) - self._data_access.remove(source, recursive=True) + self._data_access.remove(source, recursive=True, confirm=confirm) From 20e1bbd3da0daea7397e1d81778da9a2989d3e16 Mon Sep 17 00:00:00 2001 From: Jon Hagg Date: Fri, 23 Apr 2021 14:56:39 -0700 Subject: [PATCH 3/4] fix: more predictable recursive delete --- powersimdata/data_access/data_access.py | 15 +++++++-------- powersimdata/scenario/delete.py | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/powersimdata/data_access/data_access.py b/powersimdata/data_access/data_access.py index 9466191a6..34b550e62 100644 --- a/powersimdata/data_access/data_access.py +++ b/powersimdata/data_access/data_access.py @@ -203,18 +203,17 @@ def remove(self, target, recursive=False, confirm=True): :param bool recursive: delete directories recursively :param bool confirm: prompt before executing command """ - if recursive and "**" not in target: - target = os.path.join(target, "**") - files = glob.glob(target, recursive=recursive) if confirm: - print("This will delete the following files:") - print(files) - confirmed = input("Proceed? [y/n] (default is 'n')") + confirmed = input(f"Delete {target}? [y/n] (default is 'n')") if confirmed.lower() != "y": print("Operation cancelled.") return - for f in files: - os.remove(f) + if recursive: + shutil.rmtree(target) + else: + files = [f for f in glob.glob(target) if os.path.isfile(f)] + for f in files: + os.remove(f) print("--> Done!") def execute_command(self, command): diff --git a/powersimdata/scenario/delete.py b/powersimdata/scenario/delete.py index 2f8fc0886..b8f93ac34 100644 --- a/powersimdata/scenario/delete.py +++ b/powersimdata/scenario/delete.py @@ -63,7 +63,7 @@ def delete_scenario(self, confirm=True): # Delete local files print("--> Deleting input and output data on local machine") target = os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard) - LocalDataAccess().remove(target, recursive=True, confirm=confirm) + LocalDataAccess().remove(target, recursive=False, confirm=confirm) # Delete attributes self._clean() From 5809d3c590c721771fc569d882541dcfd5abb90d Mon Sep 17 00:00:00 2001 From: Jon Hagg Date: Mon, 26 Apr 2021 13:10:44 -0700 Subject: [PATCH 4/4] docs: cleanup comments and docstrings --- powersimdata/scenario/delete.py | 3 --- powersimdata/scenario/move.py | 4 +++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/powersimdata/scenario/delete.py b/powersimdata/scenario/delete.py index b8f93ac34..5be2c455c 100644 --- a/powersimdata/scenario/delete.py +++ b/powersimdata/scenario/delete.py @@ -43,12 +43,10 @@ def delete_scenario(self, confirm=True): wildcard = f"{scenario_id}_*" - # Delete links to base profiles on server print("--> Deleting scenario input data on server") target = posixpath.join(self.path_config.input_dir(), wildcard) self._data_access.remove(target, recursive=False, confirm=confirm) - # Delete output profiles print("--> Deleting scenario output data on server") target = posixpath.join(self.path_config.output_dir(), wildcard) self._data_access.remove(target, recursive=False, confirm=confirm) @@ -60,7 +58,6 @@ def delete_scenario(self, confirm=True): ) self._data_access.remove(tmp_dir, recursive=True, confirm=confirm) - # Delete local files print("--> Deleting input and output data on local machine") target = os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard) LocalDataAccess().remove(target, recursive=False, confirm=confirm) diff --git a/powersimdata/scenario/move.py b/powersimdata/scenario/move.py index e2916ed6c..ee5eb840d 100644 --- a/powersimdata/scenario/move.py +++ b/powersimdata/scenario/move.py @@ -30,6 +30,8 @@ def move_scenario(self, target="disk", confirm=True): :param str target: optional argument specifying the backup system. :param bool confirm: prompt before deleting each batch of files + :raises TypeError: if target is not a str + :raises ValueError: if target is unknown (only "disk" is supported) """ if not isinstance(target, str): raise TypeError("string is expected for optional argument target") @@ -59,7 +61,7 @@ class BackUpDisk(object): :param powersimdata.data_access.data_access.DataAccess data_access: data access object. - :param dict scenario: scenario information. + :param dict scenario_info: scenario information. """ def __init__(self, data_access, scenario_info):