diff --git a/datashuttle/datashuttle_class.py b/datashuttle/datashuttle_class.py index 267990c82..868df4f02 100644 --- a/datashuttle/datashuttle_class.py +++ b/datashuttle/datashuttle_class.py @@ -26,6 +26,7 @@ OverwriteExistingFiles, Prefix, TopLevelFolder, + TransferErrors, ) import yaml @@ -315,7 +316,8 @@ def upload_custom( overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, init_log: bool = True, - ) -> None: + display_errors: bool = True, + ) -> TransferErrors: """Upload data from a local project to the central project folder. Parameters @@ -354,13 +356,16 @@ def upload_custom( always be ``True``, unless logger is handled elsewhere (e.g. in a calling function). + display_errors + if `True`, a summary of errors will be printed alongside the Rclone logs. + """ if init_log: self._start_log("upload-custom", local_vars=locals()) self._check_top_level_folder(top_level_folder) - TransferData( + errors = TransferData( self.cfg, "upload", top_level_folder, @@ -369,12 +374,16 @@ def upload_custom( datatype, overwrite_existing_files, dry_run, - log=True, - ) + ).run() + + if display_errors: + rclone.log_rclone_copy_errors_api(errors) if init_log: ds_logger.close_log_filehandler() + return errors + @check_configs_set @check_is_not_local_project def download_custom( @@ -386,7 +395,8 @@ def download_custom( overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, init_log: bool = True, - ) -> None: + display_errors: bool = True, + ) -> TransferErrors: """Download data from the central project to the local project folder. Parameters @@ -425,13 +435,16 @@ def download_custom( always be ``True``, unless logger is handled elsewhere (e.g. in a calling function). + display_errors + if `True`, a summary of errors will be printed alongside the Rclone logs. + """ if init_log: self._start_log("download-custom", local_vars=locals()) self._check_top_level_folder(top_level_folder) - TransferData( + errors = TransferData( self.cfg, "download", top_level_folder, @@ -440,12 +453,16 @@ def download_custom( datatype, overwrite_existing_files, dry_run, - log=True, - ) + ).run() + + if display_errors: + rclone.log_rclone_copy_errors_api(errors) if init_log: ds_logger.close_log_filehandler() + return errors + # Specific top-level folder # ---------------------------------------------------------------------------------- # A set of convenience functions are provided to abstract @@ -457,7 +474,7 @@ def upload_rawdata( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Upload all files in the `rawdata` top level folder. Parameters @@ -474,7 +491,7 @@ def upload_rawdata( transfer was taking place, but no files will be moved. """ - self._transfer_top_level_folder( + return self._transfer_top_level_folder( "upload", "rawdata", overwrite_existing_files=overwrite_existing_files, @@ -487,7 +504,7 @@ def upload_derivatives( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Upload all files in the `derivatives` top level folder. Parameters @@ -504,7 +521,7 @@ def upload_derivatives( transfer was taking place, but no files will be moved. """ - self._transfer_top_level_folder( + return self._transfer_top_level_folder( "upload", "derivatives", overwrite_existing_files=overwrite_existing_files, @@ -517,7 +534,7 @@ def download_rawdata( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Download all files in the `rawdata` top level folder. Parameters @@ -534,7 +551,7 @@ def download_rawdata( transfer was taking place, but no files will be moved.. """ - self._transfer_top_level_folder( + return self._transfer_top_level_folder( "download", "rawdata", overwrite_existing_files=overwrite_existing_files, @@ -547,7 +564,7 @@ def download_derivatives( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Download all files in the `derivatives` top level folder. Parameters @@ -564,7 +581,7 @@ def download_derivatives( transfer was taking place, but no files will be moved. """ - self._transfer_top_level_folder( + return self._transfer_top_level_folder( "download", "derivatives", overwrite_existing_files=overwrite_existing_files, @@ -577,7 +594,7 @@ def upload_entire_project( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Upload the entire project. Includes every top level folder (e.g. ``rawdata``, ``derivatives``). @@ -597,18 +614,21 @@ def upload_entire_project( """ self._start_log("upload-entire-project", local_vars=locals()) - self._transfer_entire_project( + + errors = self._transfer_entire_project( "upload", overwrite_existing_files, dry_run ) ds_logger.close_log_filehandler() + return errors + @check_configs_set @check_is_not_local_project def download_entire_project( self, overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Download the entire project. Includes every top level folder (e.g. ``rawdata``, ``derivatives``). @@ -628,11 +648,15 @@ def download_entire_project( """ self._start_log("download-entire-project", local_vars=locals()) - self._transfer_entire_project( + + errors = self._transfer_entire_project( "download", overwrite_existing_files, dry_run ) + ds_logger.close_log_filehandler() + return errors + @check_configs_set @check_is_not_local_project def upload_specific_folder_or_file( @@ -640,7 +664,7 @@ def upload_specific_folder_or_file( filepath: Union[str, Path], overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Upload a specific file or folder. If transferring a single file, the path including the filename is @@ -666,12 +690,14 @@ def upload_specific_folder_or_file( """ self._start_log("upload-specific-folder-or-file", local_vars=locals()) - self._transfer_specific_file_or_folder( + errors = self._transfer_specific_file_or_folder( "upload", filepath, overwrite_existing_files, dry_run ) ds_logger.close_log_filehandler() + return errors + @check_configs_set @check_is_not_local_project def download_specific_folder_or_file( @@ -679,7 +705,7 @@ def download_specific_folder_or_file( filepath: Union[str, Path], overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, - ) -> None: + ) -> TransferErrors: """Download a specific file or folder. If transferring a single file, the path including the filename is @@ -708,12 +734,14 @@ def download_specific_folder_or_file( "download-specific-folder-or-file", local_vars=locals() ) - self._transfer_specific_file_or_folder( + errors = self._transfer_specific_file_or_folder( "download", filepath, overwrite_existing_files, dry_run ) ds_logger.close_log_filehandler() + return errors + def _transfer_top_level_folder( self, upload_or_download: Literal["upload", "download"], @@ -721,7 +749,8 @@ def _transfer_top_level_folder( overwrite_existing_files: OverwriteExistingFiles = "never", dry_run: bool = False, init_log: bool = True, - ) -> None: + display_errors: bool = True, + ) -> TransferErrors: """Upload or download files within a particular top-level-folder. A centralised function to upload or download data within @@ -738,7 +767,7 @@ def _transfer_top_level_folder( else self.download_custom ) - transfer_func( + errors = transfer_func( top_level_folder, "all", "all", @@ -746,14 +775,17 @@ def _transfer_top_level_folder( overwrite_existing_files=overwrite_existing_files, dry_run=dry_run, init_log=False, + display_errors=display_errors, ) if init_log: ds_logger.close_log_filehandler() + return errors + def _transfer_specific_file_or_folder( self, upload_or_download, filepath, overwrite_existing_files, dry_run - ) -> None: + ) -> TransferErrors: """Core function for upload/download_specific_folder_or_file().""" if isinstance(filepath, str): filepath = Path(filepath) @@ -784,6 +816,7 @@ def _transfer_specific_file_or_folder( processed_filepath = filepath include_list = [f"--include /{processed_filepath.as_posix()}"] + output = rclone.transfer_data( self.cfg, upload_or_download, @@ -793,8 +826,13 @@ def _transfer_specific_file_or_folder( overwrite_existing_files, dry_run ), ) + stdout, stderr, errors = rclone.parse_rclone_copy_output( + top_level_folder, output + ) + rclone.log_stdout_stderr_python_api(stdout, stderr) + rclone.log_rclone_copy_errors_api(errors) - utils.log(output.stderr.decode("utf-8")) + return errors # ------------------------------------------------------------------------- # SSH @@ -1434,23 +1472,40 @@ def _transfer_entire_project( upload_or_download: Literal["upload", "download"], overwrite_existing_files: OverwriteExistingFiles, dry_run: bool, - ) -> None: + ) -> TransferErrors: """Transfer the entire project. i.e. every 'top level folder' (e.g. 'rawdata', 'derivatives'). See ``upload_custom()`` or ``download_custom()`` for parameters. """ + all_errors = rclone.get_empty_errors_dict() + for top_level_folder in canonical_folders.get_top_level_folders(): - utils.log_and_message(f"Transferring `{top_level_folder}`") + utils.log_and_message( + f"\n\n*************************************\n" + f"Transferring `{top_level_folder}`\n" + f"*************************************\n" + ) - self._transfer_top_level_folder( + errors = self._transfer_top_level_folder( upload_or_download, top_level_folder, overwrite_existing_files=overwrite_existing_files, dry_run=dry_run, init_log=False, + display_errors=False, ) + all_errors["file_names"] += errors["file_names"] + all_errors["messages"] += errors["messages"] + + key = f"nothing_was_transferred_{top_level_folder}" + all_errors[key] = errors[key] # type: ignore + + rclone.log_rclone_copy_errors_api(all_errors) + + return all_errors + def _start_log( self, command_name: str, diff --git a/datashuttle/tui/css/tui_menu.tcss b/datashuttle/tui/css/tui_menu.tcss index b3f8b915b..b795d355d 100644 --- a/datashuttle/tui/css/tui_menu.tcss +++ b/datashuttle/tui/css/tui_menu.tcss @@ -91,11 +91,6 @@ MessageBox { align: center middle; } -#messagebox_top_container { - height: 15; - width: 65; - } - #messagebox_message_container { align: center middle; overflow: hidden auto; @@ -112,6 +107,8 @@ MessageBox { height: 3; } +/* NOTE: `messagebox_top_container` width and height are defined on the `MessageBox` class. + /* Light Mode Error Screen */ MessageBox:light > #messagebox_top_container { diff --git a/datashuttle/tui/interface.py b/datashuttle/tui/interface.py index 00f6e0697..cc5be7a78 100644 --- a/datashuttle/tui/interface.py +++ b/datashuttle/tui/interface.py @@ -259,20 +259,20 @@ def transfer_entire_project(self, upload: bool) -> InterfaceOutput: else: transfer_func = self.project.download_entire_project - transfer_func( + errors = transfer_func( overwrite_existing_files=self.tui_settings[ "overwrite_existing_files" ], dry_run=self.tui_settings["dry_run"], ) - return True, None + return True, errors except BaseException as e: return False, str(e) def transfer_top_level_only( - self, selected_top_level_folder: str, upload: bool + self, selected_top_level_folder: TopLevelFolder, upload: bool ) -> InterfaceOutput: """Transfer all files within a selected top level folder. @@ -302,14 +302,14 @@ def transfer_top_level_only( else self.project.download_derivatives ) - transfer_func( + errors = transfer_func( overwrite_existing_files=self.tui_settings[ "overwrite_existing_files" ], dry_run=self.tui_settings["dry_run"], ) - return True, None + return True, errors except BaseException as e: return False, str(e) @@ -349,7 +349,7 @@ def transfer_custom_selection( else: transfer_func = self.project.download_custom - transfer_func( + errors = transfer_func( selected_top_level_folder, sub_names=sub_names, ses_names=ses_names, @@ -360,7 +360,7 @@ def transfer_custom_selection( dry_run=self.tui_settings["dry_run"], ) - return True, None + return True, errors except BaseException as e: return False, str(e) diff --git a/datashuttle/tui/screens/modal_dialogs.py b/datashuttle/tui/screens/modal_dialogs.py index 0d4c94c57..9fb48b244 100644 --- a/datashuttle/tui/screens/modal_dialogs.py +++ b/datashuttle/tui/screens/modal_dialogs.py @@ -49,7 +49,13 @@ class MessageBox(ModalScreen): """ - def __init__(self, message: str, border_color: str) -> None: + def __init__( + self, + message: str, + border_color: str, + width: str = "65", + height: str = "15", + ) -> None: """Initialise the MessageBox. Parameters @@ -60,15 +66,23 @@ def __init__(self, message: str, border_color: str) -> None: border_color Color of the MessageBox border (e.g. green if the message is positive). + height + The height of the messagebox. + + width + The width of the messagebox. + """ super(MessageBox, self).__init__() self.message = message self.border_color = border_color + self.top_container_width = width + self.top_container_height = height def compose(self) -> ComposeResult: """Add widgets to the MessageBox.""" - yield Container( + self.top_container = Container( Container( Static(self.message, id="messagebox_message_label"), id="messagebox_message_container", @@ -77,6 +91,8 @@ def compose(self) -> ComposeResult: id="messagebox_top_container", ) + yield self.top_container + def on_mount(self) -> None: """Update widgets immediately after mounting.""" if self.border_color == "red": @@ -93,6 +109,9 @@ def on_mount(self) -> None: color, ) + self.top_container.styles.width = self.top_container_width + self.top_container.styles.height = self.top_container_height + def on_button_pressed(self) -> None: """Handle button press.""" self.dismiss(True) @@ -218,24 +237,70 @@ def on_button_pressed(self, event: Button.Pressed) -> None: self.dismiss() async def handle_transfer_and_update_ui_when_complete(self) -> None: - """Run the data transfer worker and updates the UI on completion.""" - data_transfer_worker = self.transfer_func() - await data_transfer_worker.wait() - success, output = data_transfer_worker.result - self.dismiss() - - if success: - self.app.push_screen( - MessageBox( - "Transfer finished." - "\n\n" - "Check the most recent logs to " - "ensure transfer completed successfully.", - border_color="grey", + """Run the data transfer worker and updates the UI on completion. + + Note this function is very similar to `log_rclone_copy_errors_api` + but kept separate for flexibility. + """ + try: + data_transfer_worker = self.transfer_func() + await data_transfer_worker.wait() + success, output = data_transfer_worker.result + + self.dismiss() + + if success: + errors = output + + errors_message = "" + + messagebox_kwargs = {} + + no_transfer_col = ( + "blue" + if self.app.theme == "textual-light" + else "lightblue" ) - ) - else: - self.app.show_modal_error_dialog(output) + + if errors["nothing_was_transferred_rawdata"] is True: + errors_message += f"[{no_transfer_col}]\nNothing was transferred from rawdata.[/{no_transfer_col}]\n" + + if errors["nothing_was_transferred_derivatives"] is True: + errors_message += f"[{no_transfer_col}]\nNothing was transferred from derivatives.[/{no_transfer_col}]\n" + + if any(errors["messages"]): + if errors["file_names"]: + errors_message += ( + "\n[red]Errors detected! in files:[/red]\n" + ) + errors_message += "\n".join(errors["file_names"]) + else: + errors_message += "\n[red]Errors detected![/red]" + errors_message += ( + "[red]\n\nThe error messages are:[/red]\n" + ) + errors_message += "\n\n".join(errors["messages"]) + messagebox_kwargs = {"width": "75%", "height": "75%"} + + if errors_message == "": + errors_message += "No errors detected" + + message = ( + f"Transfer finished.\n" + f"{errors_message}\n\n" + f"Check the most recent logs for full details." + ) + + self.app.push_screen( + MessageBox( + message, border_color="grey", **messagebox_kwargs + ) + ) + else: + self.app.show_modal_error_dialog(output) + + except BaseException as e: + self.app.show_modal_error_dialog(str(e)) class SearchingCentralForNextSubSesPopup(ModalScreen): diff --git a/datashuttle/utils/custom_types.py b/datashuttle/utils/custom_types.py index 44c8de353..af5c48124 100644 --- a/datashuttle/utils/custom_types.py +++ b/datashuttle/utils/custom_types.py @@ -1,4 +1,6 @@ -from typing import Any, Literal, Tuple +from __future__ import annotations + +from typing import Any, Literal, Tuple, TypedDict DisplayMode = Literal["error", "warn", "print"] @@ -13,3 +15,12 @@ ConnectionMethods = Literal[ "ssh", "local_filesystem", "gdrive", "aws", "local_only" ] + + +class TransferErrors(TypedDict): + """Type `errors` dictionary (used for collecting `rclone copy` output).""" + + file_names: list[str] + messages: list[str] + nothing_was_transferred_rawdata: bool | None + nothing_was_transferred_derivatives: bool | None diff --git a/datashuttle/utils/data_transfer.py b/datashuttle/utils/data_transfer.py index c21d39bda..c7a021d47 100644 --- a/datashuttle/utils/data_transfer.py +++ b/datashuttle/utils/data_transfer.py @@ -33,7 +33,6 @@ def __init__( datatype: Union[str, List[str]], overwrite_existing_files: OverwriteExistingFiles, dry_run: bool, - log: bool, ): """Initialise TransferData. @@ -71,9 +70,6 @@ def __init__( Perform a dry-run of transfer. This will output as if file transfer was taking place, but no files will be moved. - log - if `True`, log and print the transfer output. - """ self.__cfg = cfg self.__upload_or_download = upload_or_download @@ -84,6 +80,8 @@ def __init__( self.__base_folder = self.__cfg.get_base_folder( self.__local_or_central, self.__top_level_folder ) + self.__overwrite_existing_files = overwrite_existing_files + self.__dry_run = dry_run self.sub_names = self.to_list(sub_names) self.ses_names = self.to_list(ses_names) @@ -91,6 +89,8 @@ def __init__( self.check_input_arguments() + def run(self): + """Run the transfer.""" include_list = self.build_a_list_of_all_files_and_folders_to_transfer() if any(include_list): @@ -99,16 +99,29 @@ def __init__( self.__upload_or_download, self.__top_level_folder, include_list, - cfg.make_rclone_transfer_options( - overwrite_existing_files, dry_run + self.__cfg.make_rclone_transfer_options( + self.__overwrite_existing_files, self.__dry_run ), ) - if log: - utils.log_and_message(output.stderr.decode("utf-8")) + stdout, stderr, errors = rclone.parse_rclone_copy_output( + self.__top_level_folder, output + ) + + if output.returncode != 0 and not any(errors["messages"]): + raise RuntimeError( + "Errors were detected in transfer but not reported properly. " + "Please contact the datashuttle team." + ) + + rclone.log_stdout_stderr_python_api(stdout, stderr) + else: - if log: - utils.log_and_message("No files included. None transferred.") + utils.log_and_message("No files included. None transferred.") + errors = rclone.get_empty_errors_dict() + errors[f"nothing_was_transferred_{self.__top_level_folder}"] = True + + return errors # ------------------------------------------------------------------------- # Build the --include list diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index fe1909119..e87deb779 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -4,13 +4,15 @@ if TYPE_CHECKING: from datashuttle.configs.config_class import Configs - from datashuttle.utils.custom_types import TopLevelFolder + from datashuttle.utils.custom_types import TopLevelFolder, TransferErrors +import json import os import platform import shlex import subprocess import tempfile +from pathlib import Path from subprocess import CompletedProcess from datashuttle.configs import canonical_configs @@ -461,19 +463,206 @@ def transfer_data( output = call_rclone_through_script( f"{rclone_args('copy')} " f'"{local_filepath}" "{cfg.get_rclone_config_name()}:' - f'{central_filepath}" {extra_arguments}', + f'{central_filepath}" {extra_arguments} --use-json-log', ) elif upload_or_download == "download": output = call_rclone_through_script( f"{rclone_args('copy')} " f'"{cfg.get_rclone_config_name()}:' - f'{central_filepath}" "{local_filepath}" {extra_arguments}', + f'{central_filepath}" "{local_filepath}" {extra_arguments} --use-json-log', ) return output +def log_stdout_stderr_python_api(stdout: str, stderr: str) -> None: + """Log `stdout` and `stderr`.""" + message = ( + f"\n\n************** STDOUT **************\n" + f"{stdout}" + f"\n\n************** STDERR **************\n" + f"{stderr}" + ) + + utils.log_and_message(message) + + +def log_rclone_copy_errors_api(errors): + """Log the `errors` dictionary. + + The `errors` dictionary contains all pertinent information on + issues that occurred when running `rclone copy`. Note this logs + for the API, the TUI display is handled separately. + + Note this function is very similar + to `handle_transfer_and_update_ui_when_complete` + but kept separate for flexibility. + """ + message = "" + + if errors["nothing_was_transferred_rawdata"] is True: + message += "\n\nNothing was transferred from rawdata.\n" + + if errors["nothing_was_transferred_derivatives"] is True: + message += "\n\nNothing was transferred from derivatives.\n" + + if any(errors["messages"]): + if any(errors["file_names"]): + message += ( + "\n\nErrors were detected! In files:" + "\n-------------------------------\n" + ) + message += "\n".join(errors["file_names"]) + else: + message += "\n\n[red]Errors detected![/red]" + message += "\n\nThe error messages are:\n-----------------------\n" + message += "\n".join(errors["messages"]) + message += "\n" + + if message == "": + message = "No errors detected" + + utils.log_and_message(message, use_rich=True) + + +def parse_rclone_copy_output(top_level_folder, output): + """Format the `rclone copy` output ready for logging. + + Reformat and combine the string streams and `errors` + dictionary from stdout and stderr output of `rclone copy`. + see `reformat_rclone_copy_output() for details. + """ + stdout, out_errors = reformat_rclone_copy_output( + output.stdout, top_level_folder=top_level_folder + ) + + stderr, err_errors = reformat_rclone_copy_output( + output.stderr, top_level_folder=top_level_folder + ) + + # Combine the two `errors` output + all_errors = { + "file_names": out_errors["file_names"] + err_errors["file_names"], + "messages": out_errors["messages"] + err_errors["messages"], + "nothing_was_transferred_rawdata": err_errors[ + "nothing_was_transferred_rawdata" + ], + "nothing_was_transferred_derivatives": err_errors[ + "nothing_was_transferred_derivatives" + ], + } + + all_errors["file_names"] = list(set(all_errors["file_names"])) + + return stdout, stderr, all_errors + + +def get_empty_errors_dict() -> TransferErrors: + """Return the `errors` dictionary with default values. + + The `errors` dictionary holds information + about errors which occurred during `rclone copy` transfer. + The dict entries are: + + file_names + A list of file names associated with errors. + + messages + A list of messages associated with errors. For each file name, + there will be an associated message, but it is also possible to + have messages that are not associated with any file name. + + nothing_was_transferred_rawdata + A flag that can take the value `None`, `True` or `False`. + If `None`, this top-level folder was not attempted to be transferred. + If `True`, it was attempted and nothing was transferred. If `False`, + it was attempted and something was transferred. + + nothing_was_transferred_derivatives + See `nothing_was_transferred_rawdata`, this is the equivalent for + the derivatives' folder. + + The rawdata and derivatives flags must be split as some functions + transfer a single, or both, top level folders in one command. + """ + return { + "file_names": [], + "messages": [], + "nothing_was_transferred_rawdata": None, + "nothing_was_transferred_derivatives": None, + } + + +def reformat_rclone_copy_output( + stream: bytes, + top_level_folder: TopLevelFolder | None = None, +) -> tuple[str, TransferErrors]: + """Parse the output of `rclone copy` for convenient error checking. + + Rclone's `copy` command (called with `--use-json-log`) outputs a lot of + information related to the transfer. We dump this in text form to a log + file. However, we also want to grab any key events (errors, or complete + lack of transferred files) so these can be displayed separately. + + This function iterates through all lines in the `rclone copy` output. + This output is typically a mix of string format and json format. + If the line is json-encoded, then we extract important information + and format it to string, and re-insert it into the output. + + In this way, we have a string-format output ready to be + dumped to the logs, as well as an `errors` dictionary containing + details on all key information. + + Returns + ------- + format_stream + The input stream, converted to string and with all + json-formatted lines reformatted as string. This is ready + to be dumped to a log file. + + errors + A dictionary (`TransferErrors`) containing key information + about issues in the transfer. + + """ + split_stream = stream.decode("utf-8").split("\n") + + errors = get_empty_errors_dict() + + for idx, line in enumerate(split_stream): + try: + line_json = json.loads(line) + except json.JSONDecodeError: + continue + + if line_json["level"] in ["error", "critical"]: + if "object" in line_json: + full_filepath = Path( + f"{top_level_folder}/{line_json['object']}" + ).as_posix() + errors["file_names"].append(full_filepath) + errors["messages"].append( + f"The file {full_filepath} failed to transfer. Reason: {line_json['msg']}" + ) + else: + errors["messages"].append(f"ERROR : {line_json['msg']}") + + elif "stats" in line_json and "totalTransfers" in line_json["stats"]: + if line_json["stats"]["totalTransfers"] == 0: + errors[f"nothing_was_transferred_{top_level_folder}"] = True # type:ignore + else: + errors[f"nothing_was_transferred_{top_level_folder}"] = False # type:ignore + + split_stream[idx] = ( + f"{line_json['time'][:19]} {line_json['level'].upper()} : {line_json['msg']}" + ) + + format_stream = "\n".join(split_stream) + + return format_stream, errors + + def get_local_and_central_file_differences( cfg: Configs, top_level_folders_to_check: List[TopLevelFolder], diff --git a/datashuttle/utils/ssh.py b/datashuttle/utils/ssh.py index c7d6256a4..b31379c1c 100644 --- a/datashuttle/utils/ssh.py +++ b/datashuttle/utils/ssh.py @@ -210,7 +210,7 @@ def connect_client( f"3) The central_host_id: {cfg['central_host_id']} is" f" correct.\n" f"4) The central username:" - f" {cfg['central_host_username']}, and password are correct." + f" {cfg['central_host_username']}, and password are correct.\n" f"Original error: {e}", ConnectionError, ) diff --git a/pyproject.toml b/pyproject.toml index 2e9402411..66abee6c1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,7 @@ dev = [ "types-setuptools", "pytest-asyncio", "validators", + "filelock" ] [build-system] diff --git a/tests/test_utils.py b/tests/test_utils.py index bb38c5851..54f80a21b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -6,6 +6,8 @@ import os import pathlib import shutil +import threading +import time import warnings from os.path import join from pathlib import Path @@ -728,3 +730,21 @@ def mock_get_datashuttle_path(): "datashuttle.configs.canonical_folders.get_datashuttle_path", mock_get_datashuttle_path, ) + + +def lock_a_file(file_path, duration=5): + """""" + + def continually_write_to_file(path, duration): + end_time = time.time() + duration + with open(path, "a") as f: + while time.time() < end_time: + f.write("LOCKED\n") + f.flush() + + t = threading.Thread( + target=continually_write_to_file, args=(file_path, duration) + ) + t.start() + + return t diff --git a/tests/tests_integration/test_logging.py b/tests/tests_integration/test_logging.py index 9b5bd4a0b..233ad3d0b 100644 --- a/tests/tests_integration/test_logging.py +++ b/tests/tests_integration/test_logging.py @@ -1,12 +1,15 @@ import glob import logging import os +import platform import re from pathlib import Path import pytest +from filelock import FileLock from datashuttle.configs import canonical_configs +from datashuttle.configs.canonical_configs import get_broad_datatypes from datashuttle.configs.canonical_tags import tags from datashuttle.utils import ds_logger from datashuttle.utils.custom_exceptions import ( @@ -482,3 +485,94 @@ def test_validate_names_against_project_logging(self, project): assert "ERROR" in log assert str(e.value) in log + + def test_errors_are_caught_and_logged(self, project): + """ + Create errors in the transfer by locking files, and check + the errors are correctly flagged in logs and `errors`. Also, + perform a transfer where no files are transferred, and check + this is flagged in logs and `errors`. + """ + + # Set up a folder to transfer + subs, sessions = test_utils.get_default_sub_sessions_to_test() + + test_utils.make_and_check_local_project_folders( + project, + "rawdata", + subs, + sessions, + get_broad_datatypes(), + ) + + # Lock a file then perform the transfer, causing errors. + relative_path = ( + Path("rawdata") + / subs[0] + / sessions[0] + / "ephys" + / "placeholder_file.txt" + ) + a_transferred_file = project.get_local_path() / relative_path + + test_utils.delete_log_files(project.cfg.logging_path) + + test_utils.delete_log_files(project.cfg.logging_path) + + if platform.system() == "Windows": + lock = FileLock(a_transferred_file, timeout=5) + with lock: + errors = project.upload_custom("rawdata", "all", "all", "all") + error_message = "because another process has locked " + else: + thread = test_utils.lock_a_file(a_transferred_file) + errors = project.upload_custom("rawdata", "all", "all", "all") + thread.join() + error_message = "size changed" + + # Check that errors and logs flag the transfer errors + assert errors["file_names"] == [relative_path.as_posix()] + assert error_message in errors["messages"][0] + + log = test_utils.read_log_file(project.cfg.logging_path) + + assert Path(errors["file_names"][0]).as_posix() in log + assert "Errors were detected!" in log + assert error_message in log + + # now just upload everything + project.upload_entire_project() + test_utils.delete_log_files(project.cfg.logging_path) + + # Check that it is flagged that no transfer took place for rawdata + errors = project.upload_custom("rawdata", "all", "all", "all") + + assert errors["nothing_was_transferred_rawdata"] is True + assert errors["nothing_was_transferred_derivatives"] is None + + log = test_utils.read_log_file(project.cfg.logging_path) + assert "Nothing was transferred from rawdata." in log + + test_utils.delete_log_files(project.cfg.logging_path) + + # Check that it is flagged that no transfer took place for derivatives + errors = project.upload_custom("derivatives", "all", "all", "all") + + assert errors["nothing_was_transferred_rawdata"] is None + assert errors["nothing_was_transferred_derivatives"] is True + + log = test_utils.read_log_file(project.cfg.logging_path) + assert "Nothing was transferred from derivatives." in log + + test_utils.delete_log_files(project.cfg.logging_path) + + # Check that it is flagged that no transfer took place + # for both rawdata and derivatives + errors = project.upload_entire_project() + + assert errors["nothing_was_transferred_rawdata"] is True + assert errors["nothing_was_transferred_derivatives"] is True + + log = test_utils.read_log_file(project.cfg.logging_path) + assert "Nothing was transferred from rawdata." in log + assert "Nothing was transferred from derivatives." in log diff --git a/tests/tests_transfers/local_filesystem/test_transfer.py b/tests/tests_transfers/local_filesystem/test_transfer.py index b924650e5..039300a55 100644 --- a/tests/tests_transfers/local_filesystem/test_transfer.py +++ b/tests/tests_transfers/local_filesystem/test_transfer.py @@ -641,6 +641,90 @@ def test_overwrite_different_size_different_times( elif overwrite_existing_files == "always": assert test_utils.read_file(central_file_path) == ["file earlier"] + def test_errors_variable(self, project, monkeypatch): + """ + Test that the `errors` variable is correctly + returned from every transfer function. `errors` is a variable + that contains information about any errors that were encountered + during transfer. + """ + + # Monkeypatch the error-parsing function so it returns + # predictable values. + import datashuttle + + def test_errors(top_level_folder): + return { + "file_names": [f"{top_level_folder}/hello_world.txt"], + "messages": ["how are you?"], + "nothing_was_transferred_rawdata": None, + "nothing_was_transferred_derivatives": None, + } + + def monkeypatch_parse_output(top_level_folder, b): + stdout = "stdout" + stderr = "stderr" + return stdout, stderr, test_errors(top_level_folder) + + monkeypatch.setattr( + datashuttle.utils.rclone, + "parse_rclone_copy_output", + monkeypatch_parse_output, + ) + + # Generate some test files so the transfer runs properly + subs, sessions = test_utils.get_default_sub_sessions_to_test() + + for top_level_folder in ["rawdata", "derivatives"]: + test_utils.make_and_check_local_project_folders( + project, + top_level_folder, + subs, + sessions, + get_broad_datatypes(), + ) + + # Run every transfer function and check that + # `errors` is returned correctly. + specific_file = ( + lambda path_: f"{path_}/rawdata/{subs[0]}/{sessions[0]}/ephys/placeholder_file.txt" + ) + + # All 'rawdata' functions + for func in [ + lambda: project.upload_specific_folder_or_file( + specific_file(project.get_local_path()) + ), + lambda: project.download_specific_folder_or_file( + specific_file(project.get_central_path()) + ), + lambda: project.upload_custom("rawdata", "all", "all", "all"), + lambda: project.download_custom("rawdata", "all", "all", "all"), + project.upload_rawdata, + project.download_rawdata, + ]: + assert func() == test_errors("rawdata") + + # All 'derivatives' functions + for func in [project.upload_derivatives, project.download_derivatives]: + assert func() == test_errors("derivatives") + + # Entire project functions should merge the errors + # of rawdata and derivatives + for func in [ + project.upload_entire_project, + project.download_entire_project, + ]: + assert func() == { + "file_names": [ + "rawdata/hello_world.txt", + "derivatives/hello_world.txt", + ], + "messages": ["how are you?", "how are you?"], + "nothing_was_transferred_rawdata": None, + "nothing_was_transferred_derivatives": None, + } + def get_paths_to_a_local_and_central_file(self, project, top_level_folder): path_to_test_file = ( Path(top_level_folder) diff --git a/tests/tests_tui/test_tui_transfer.py b/tests/tests_tui/test_tui_transfer.py index 03b036508..67a46606a 100644 --- a/tests/tests_tui/test_tui_transfer.py +++ b/tests/tests_tui/test_tui_transfer.py @@ -1,4 +1,8 @@ +import platform +from pathlib import Path + import pytest +from filelock import FileLock from datashuttle.configs import canonical_configs from datashuttle.tui.app import TuiApp @@ -218,6 +222,86 @@ async def test_transfer_custom( await pilot.pause() + @pytest.mark.asyncio + async def test_errors_are_reported_on_pop_up(self, setup_project_paths): + """ + Check that transfer errors, or the case where no files are transferred, + are displayed properly on the modal dialog that displays following + the transfer. + """ + tmp_config_path, tmp_path, project_name = setup_project_paths.values() + + subs, sessions = test_utils.get_default_sub_sessions_to_test() + + app = TuiApp() + async with app.run_test(size=self.tui_size()) as pilot: + # Set up the project files to transfer and navigate + # to the transfer screen + await self.check_and_click_onto_existing_project( + pilot, project_name + ) + await self.switch_tab(pilot, "transfer") + + project = pilot.app.screen.interface.project + + self.setup_project_for_data_transfer( + project, + subs, + sessions, + ["rawdata", "derivatives"], + "upload", + ) + + relative_path = ( + Path("rawdata") + / subs[0] + / sessions[0] + / "ephys" + / "placeholder_file.txt" + ) + + # Lock a file and perform the transfer, which will have errors. + # Check the errors are displayed in the pop-up window. + a_transferred_file = project.get_local_path() / relative_path + + if platform.system() == "Windows": + lock = FileLock(a_transferred_file, timeout=5) + with lock: + await self.run_transfer( + pilot, "upload", close_final_messagebox=False + ) + error_message = "because another process has locked " + else: + thread = test_utils.lock_a_file( + a_transferred_file, duration=20 + ) + + await self.run_transfer( + pilot, "upload", close_final_messagebox=False + ) + thread.join() + error_message = "size changed" + + displayed_message = app.screen.message + + assert relative_path.as_posix() in displayed_message + + assert error_message in displayed_message + + await self.close_messagebox(pilot) + + # Transfer again to check the message displays indicating + # no files were transferred. + await self.run_transfer(pilot, "upload") + + await self.run_transfer( + pilot, "upload", close_final_messagebox=False + ) + + assert "Nothing was transferred from rawdata" in app.screen.message + + await pilot.pause() + async def switch_top_level_folder_select( self, pilot, id, top_level_folder ): @@ -227,14 +311,16 @@ async def switch_top_level_folder_select( await self.move_select_to_position(pilot, id, position=5) assert pilot.app.screen.query_one(id).value == "derivatives" - async def run_transfer(self, pilot, upload_or_download): + async def run_transfer( + self, pilot, upload_or_download, close_final_messagebox=True + ): # Check assumed default is correct on the transfer switch assert pilot.app.screen.query_one("#transfer_switch").value is False if upload_or_download == "download": await self.scroll_to_click_pause(pilot, "#transfer_switch") - await self.click_and_await_transfer(pilot) + await self.click_and_await_transfer(pilot, close_final_messagebox) def setup_project_for_data_transfer( self, diff --git a/tests/tests_tui/tui_base.py b/tests/tests_tui/tui_base.py index a0c8f16d6..2d08e5ffe 100644 --- a/tests/tests_tui/tui_base.py +++ b/tests/tests_tui/tui_base.py @@ -239,7 +239,9 @@ async def move_select_to_position(self, pilot, id, position): await pilot.click(id, offset=(2, position)) await pilot.pause() - async def click_and_await_transfer(self, pilot): + async def click_and_await_transfer( + self, pilot, close_final_messagebox=True + ): await self.scroll_to_click_pause(pilot, "#transfer_transfer_button") await self.scroll_to_click_pause(pilot, "#confirm_ok_button") @@ -248,7 +250,8 @@ async def click_and_await_transfer(self, pilot): if transfer_task: await transfer_task - await self.close_messagebox(pilot) + if close_final_messagebox: + await self.close_messagebox(pilot) async def double_click_input(self, pilot, sub_or_ses, control=False): """Helper function to double click input to suggest next sub or ses.