From 6538b5ac8b154e00bed6e4bfa9197e33b63a28dc Mon Sep 17 00:00:00 2001 From: Taha Abdullah Date: Thu, 5 Sep 2024 13:58:43 +0200 Subject: [PATCH] Moving Fastsurfer and Pytest environment setup to local_runner image, thus simplifying, and streamlining the workflow and improving execution time significantly. Adding Labelled output directories for each run Removing unnecessary import statements from pytest files Cleaning up code: - migrating from os.path.join to pathlib Path - changing str to Path - cleaning up comments and docstrings --- .github/workflows/quicktest_runner.yaml | 64 ++---------------- test/quick_test/conftest.py | 2 +- test/quick_test/test_errors_in_logfiles.py | 19 +++--- test/quick_test/test_file_existence.py | 50 ++++---------- test/quick_test/test_images.py | 55 ++++++++-------- test/quick_test/test_stats.py | 76 +++++++++++++--------- 6 files changed, 104 insertions(+), 162 deletions(-) diff --git a/.github/workflows/quicktest_runner.yaml b/.github/workflows/quicktest_runner.yaml index e9469e01..8d316dff 100644 --- a/.github/workflows/quicktest_runner.yaml +++ b/.github/workflows/quicktest_runner.yaml @@ -62,76 +62,26 @@ jobs: echo "Error: FreeSurfer installation directory does not exist at $FREESURFER_HOME" exit 1 fi - # Set up Python using setup-python@v3 action - - name: Set up Python 3.10 - uses: actions/setup-python@v3 - with: - python-version: "3.10" - # Check if FastSurfer environment already exists - - name: Check Environment - run: | - if [ ! -f "$MAMBAPATH" ]; then - echo "FastSurfer environment does not exist. Creating FastSurfer Environment..." - echo "HAS_MAMBA=true" >> $GITHUB_OUTPUT - else - echo "FastSurfer environment exists. Skipping FastSurfer Environment creation..." - echo "HAS_MAMBA=false" >> $GITHUB_OUTPUT - fi - id: check-environment - # Create FastSurfer environment if it does not exist - - name: Create FastSurfer Environment - uses: mamba-org/setup-micromamba@v1 - with: - environment-file: env/fastsurfer.yml - micromamba-root-path: $MAMBAROOT - micromamba-binary-path: $MAMBAPATH - cache-downloads: true - cache-environment: true - init-shell: none - if: steps.check-environment.outputs.HAS_MAMBA == 'true' - # Set up cache for python packages - - name: Cache Python packages - uses: actions/cache@v3 - with: - path: /home/.cache/pip - key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} - restore-keys: | - ${{ runner.os }}-pip- - # Install required python packages - - name: Install package - run: | - python -m pip install --progress-bar off --upgrade pip \ - setuptools wheel - python -m pip install --progress-bar off .[test] - # Run fastsurfer on list of subjects + # Run FastSurfer on test subjects - name: Run FastSurfer run: | echo "Running FastSurfer..." - export FREESURFER_HOME=/freesurfer - source $FREESURFER_HOME/SetUpFreeSurfer.sh + echo "Output will be saved in data/${GITHUB_SHA:0:7}" export FASTSURFER_HOME=$(pwd) export THIS_RUN_OUTDIR=${GITHUB_SHA:0:7} mkdir -p $SUBJECTS_DIR/$THIS_RUN_OUTDIR ./brun_fastsurfer.sh --subject_list $RUNNER_FS_MRI_DATA/subjects_list.txt \ - --sd $SUBJECTS_DIR/$THIS_RUN_OUTDIR \ + --sd $SUBJECTS_DIR/$THIS_RUN_OUTDIR --license $FS_LICENSE\ --parallel --threads 4 --3T --parallel_subjects surf export TEST_DIR=$THIS_RUN_OUTDIR # Test fastsurfer output run-pytest: runs-on: self-hosted + if: always() needs: run-fastsurfer steps: - - name: Set up Python 3.10 - uses: actions/setup-python@v3 - with: - python-version: "3.10" - - name: Cache Python packages - uses: actions/cache@v3 - with: - path: /home/.cache/pip - key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} - restore-keys: | - ${{ runner.os }}-pip- - name: Run pytest - run: python -m pytest test/quick_test + run: | + source /venv-pytest/bin/activate + python -m pytest test/quick_test diff --git a/test/quick_test/conftest.py b/test/quick_test/conftest.py index d58772cb..4e64b754 100644 --- a/test/quick_test/conftest.py +++ b/test/quick_test/conftest.py @@ -20,4 +20,4 @@ def reference_dir(): @pytest.fixture def subjects_list(): - return os.environ["SUBJECTS_LIST"] + return Path(os.environ["SUBJECTS_LIST"]) diff --git a/test/quick_test/test_errors_in_logfiles.py b/test/quick_test/test_errors_in_logfiles.py index dab5f58e..73d5e806 100644 --- a/test/quick_test/test_errors_in_logfiles.py +++ b/test/quick_test/test_errors_in_logfiles.py @@ -1,4 +1,3 @@ -import os import pytest import yaml from pathlib import Path @@ -24,7 +23,7 @@ def load_errors(): # Open the error_file_path and read the errors and whitelist into arrays - error_file_path = os.path.join(Path(__file__).parent, "data/logfile.errors.yaml") + error_file_path = Path(__file__).parent / "data" / "logfile.errors.yaml" with open(error_file_path, 'r') as file: data = yaml.safe_load(file) @@ -34,13 +33,13 @@ def load_errors(): return errors, whitelist -def load_log_files(test_subject: str): +def load_log_files(test_subject: Path): """ Retrieve the log files in the given log directory. Parameters ---------- - test_subject : str + test_subject : Path Subject directory to test. Returns @@ -51,24 +50,24 @@ def load_log_files(test_subject: str): # Retrieve the log files in given log directory - log_directory = os.path.join(test_subject, "scripts") + log_directory = test_subject / "scripts" log_files = [file for file in Path(log_directory).iterdir() if file.suffix == '.log'] return log_files @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_errors(subjects_dir, test_dir, test_subject): +def test_errors(subjects_dir: Path, test_dir: Path, test_subject: Path): """ Test if there are any errors in the log files. Parameters ---------- - subjects_dir : str + subjects_dir : Path Subjects directory. - test_dir : str + test_dir : Path Tests directory. - test_subject : str + test_subject : Path Subject to test. Raises @@ -77,7 +76,7 @@ def test_errors(subjects_dir, test_dir, test_subject): If any of the keywords are in the log files. """ - test_subject = os.path.join(subjects_dir, test_dir, test_subject) + test_subject = subjects_dir / test_dir / test_subject log_files = load_log_files(test_subject) error_flag = False diff --git a/test/quick_test/test_file_existence.py b/test/quick_test/test_file_existence.py index 6e85d8d5..a0585836 100644 --- a/test/quick_test/test_file_existence.py +++ b/test/quick_test/test_file_existence.py @@ -1,44 +1,21 @@ -import os import pytest -import yaml from .common import * from logging import getLogger +from pathlib import Path + logger = getLogger(__name__) -# def get_files_from_yaml(file_path: str): -# """ -# Get the list of files from the YAML file. -# -# Parameters -# ---------- -# file_path : str -# Path to the YAML file. -# -# Returns -# ------- -# list -# List of files specified in the YAML file. -# """ -# -# # Open the file_path and read the files into an array -# with open(file_path, 'r') as file: -# data = yaml.safe_load(file) -# files = data.get('files', []) -# -# return files - - -def get_files_from_folder(folder_path: str): +def get_files_from_folder(folder_path: Path): """ Get the list of files in the directory relative to the folder path. Parameters ---------- - folder_path : str + folder_path : Path Path to the folder. Returns @@ -49,27 +26,26 @@ def get_files_from_folder(folder_path: str): # Get a list of all files in the folder recursively filenames = [] - for root, _, files in os.walk(folder_path): - for file in files: - filenames.append(os.path.relpath(os.path.join(root, file), folder_path)) + for file in Path(folder_path).rglob('*'): + filenames.append(str(file.relative_to(folder_path))) return filenames @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_file_existence(subjects_dir, test_dir, reference_dir, test_subject): +def test_file_existence(subjects_dir: Path, test_dir: Path, reference_dir: Path, test_subject: Path): """ Test the existence of files in the folder. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of the test directory. - reference_dir : str + reference_dir : Path Name of the reference directory. - test_subject : str + test_subject : Path Name of the test subject. Raises @@ -81,11 +57,11 @@ def test_file_existence(subjects_dir, test_dir, reference_dir, test_subject): print(test_subject) # Get reference files from the reference subject directory - reference_subject = os.path.join(subjects_dir, reference_dir, test_subject) + reference_subject = subjects_dir / reference_dir / test_subject reference_files = get_files_from_folder(reference_subject) # Get test list of files in the test subject directory - test_subject = os.path.join(subjects_dir, test_dir, test_subject) + test_subject = subjects_dir / test_dir / test_subject test_files = get_files_from_folder(test_subject) # Check if each file in the reference list exists in the test list diff --git a/test/quick_test/test_images.py b/test/quick_test/test_images.py index 129d324f..abd488a6 100644 --- a/test/quick_test/test_images.py +++ b/test/quick_test/test_images.py @@ -1,10 +1,8 @@ -import os import pytest -from pathlib import Path import nibabel as nib import nibabel.cmdline.diff import numpy as np -import yaml +from pathlib import Path from collections import OrderedDict @@ -17,15 +15,15 @@ logger = getLogger(__name__) -def load_image(subject_path, image_name): +def load_image(subject_path: Path, image_name: Path): """ Load the image data using nibabel. Parameters ---------- - subject_path : str + subject_path : Path Path to the subject directory. - image_name : str + image_name : Path Name of the image file. Returns @@ -33,7 +31,7 @@ def load_image(subject_path, image_name): nibabel.nifti1.Nifti1Image Image data. """ - image_path = os.path.join(subject_path, "mri", image_name) + image_path = subject_path / "mri" / image_name image = nib.load(image_path) return image @@ -100,19 +98,19 @@ def compute_mean_square_error(test_data, reference_data): @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_image_headers(subjects_dir, test_dir, reference_dir, test_subject): +def test_image_headers(subjects_dir: Path, test_dir: Path, reference_dir: Path, test_subject: Path): """ Test the image headers by comparing the headers of the test and reference images. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of test directory. - reference_dir: str + reference_dir: Path Name of reference directory. - test_subject : str + test_subject : Path Name of the test subject. Raises @@ -122,9 +120,10 @@ def test_image_headers(subjects_dir, test_dir, reference_dir, test_subject): """ # Load images - test_subject = os.path.join(subjects_dir, test_dir, test_subject) + test_subject = subjects_dir / test_dir / test_subject test_image = load_image(test_subject, "brain.mgz") - reference_subject = os.path.join(subjects_dir, reference_dir, test_subject) + + reference_subject = subjects_dir / reference_dir / test_subject reference_image = load_image(reference_subject, "brain.mgz") # Get the image headers @@ -137,19 +136,19 @@ def test_image_headers(subjects_dir, test_dir, reference_dir, test_subject): @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_seg_data(subjects_dir, test_dir, reference_dir, test_subject): +def test_seg_data(subjects_dir: Path, test_dir: Path, reference_dir: Path, test_subject: Path): """ Test the segmentation data by calculating and comparing dice scores. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of test directory. - reference_dir : str + reference_dir : Path Name of reference directory. - test_subject : str + test_subject : Path Name of the test subject. Raises @@ -158,10 +157,10 @@ def test_seg_data(subjects_dir, test_dir, reference_dir, test_subject): If the dice score is not 0 for all classes """ - test_file = os.path.join(subjects_dir, test_dir, test_subject) + test_file = subjects_dir / test_dir / test_subject test_image = load_image(test_file, "aseg.mgz") - reference_subject = os.path.join(subjects_dir, reference_dir, test_subject) + reference_subject = subjects_dir / reference_dir / test_subject reference_image = load_image(reference_subject, "aseg.mgz") labels = np.unique([np.asarray(reference_image.dataobj), np.asarray(test_image.dataobj)]) @@ -183,19 +182,19 @@ def test_seg_data(subjects_dir, test_dir, reference_dir, test_subject): @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_int_data(subjects_dir, test_dir, reference_dir, test_subject): +def test_int_data(subjects_dir: Path, test_dir: Path, reference_dir: Path, test_subject: Path): """ Test the intensity data by calculating and comparing the mean square error. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of test directory. - reference_dir : str + reference_dir : Path Name of reference directory. - test_subject : str + test_subject : Path Name of the test subject. Raises @@ -204,10 +203,10 @@ def test_int_data(subjects_dir, test_dir, reference_dir, test_subject): If the mean square error is not 0 """ - test_file = os.path.join(subjects_dir, test_dir, test_subject) + test_file = subjects_dir / test_dir / test_subject test_image = load_image(test_file, "brain.mgz") - reference_subject = os.path.join(subjects_dir, reference_dir, test_subject) + reference_subject = subjects_dir / reference_dir / test_subject reference_image = load_image(reference_subject, "brain.mgz") # Get the image data diff --git a/test/quick_test/test_stats.py b/test/quick_test/test_stats.py index 1cc300bf..ae6304cb 100644 --- a/test/quick_test/test_stats.py +++ b/test/quick_test/test_stats.py @@ -1,12 +1,10 @@ from .conftest import * -import os from pathlib import Path import pandas as pd import pytest import yaml -import numpy as np from .common import * @@ -28,7 +26,7 @@ def thresholds(): Dictionary containing the thresholds """ - thresholds_file = os.path.join(Path(__file__).parent, "data/thresholds/aseg.stats.yaml") + thresholds_file = Path(__file__).parent / "data/thresholds/aseg.stats.yaml" # Open the file_path and read the thresholds into a dictionary with open(thresholds_file, 'r') as file: @@ -39,32 +37,50 @@ def thresholds(): return default_threshold, thresholds -def load_stats_file(test_subject: str): +def load_stats_file(test_subject: Path): - files = os.listdir(os.path.join(test_subject, "stats")) + """ + Load the stats file from the given file path. + + Parameters + ---------- + test_subject : Path + Path to the test subject. + + Returns + ------- + stats_file : Path + """ + + files = os.listdir(test_subject / "stats") if "aseg.stats" in files: - return os.path.join(test_subject, "stats", "aseg.stats") + return test_subject / "stats" / "aseg.stats" elif "aparc+DKT.stats" in files: - return os.path.join(test_subject, "stats", "aparc+DKT.stats") + return test_subject / "stats" / "aparc+DKT.stats" else: raise ValueError("Unknown stats file") -def load_structs(test_file: str): +def load_structs(test_file: Path): """ Load the structs from the given file path. + Parameters + ---------- + test_file : Path + Path to the test file. + Returns ------- structs : list List of structs. """ - if "aseg" in test_file: - structs_file = os.path.join(Path(__file__).parent, "data/thresholds/aseg.stats.yaml") - elif "aparc+DKT" in test_file: - structs_file = os.path.join(Path(__file__).parent, "data/thresholds/aparc+DKT.stats.yaml") + if "aseg" in str(test_file): + structs_file = Path(__file__).parent / "data/thresholds/aseg.stats.yaml" + elif "aparc+DKT" in str(test_file): + structs_file = Path(__file__).parent / "data/thresholds/aparc+DKT.stats.yaml" else: raise ValueError("Unknown test file") @@ -76,13 +92,13 @@ def load_structs(test_file: str): return structs -def read_measure_stats(file_path: str): +def read_measure_stats(file_path: Path): """ Read the measure stats from the given file path. Parameters ---------- - file_path : str + file_path : Path Path to the stats file. Returns @@ -117,13 +133,13 @@ def read_measure_stats(file_path: str): return measure, measurements -def read_table(file_path: str): +def read_table(file_path: Path): """ Read the table from the given file path. Parameters ---------- - file_path : str + file_path : Path Path to the stats file. Returns @@ -135,7 +151,7 @@ def read_table(file_path: str): table_start = 0 columns = [] - file_path = os.path.join(file_path, "stats", "aseg.stats") + file_path = file_path / "stats" / "aseg.stats" # Retrieve stats table from the stats file with open(file_path, 'r') as file: @@ -156,17 +172,17 @@ def read_table(file_path: str): @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_measure_exists(subjects_dir, test_dir, test_subject): +def test_measure_exists(subjects_dir: Path, test_dir: Path, test_subject: Path): """ Test if the measure exists in the stats file. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of the test directory. - test_subject : str + test_subject : Path Name of the test subject. Raises @@ -175,7 +191,7 @@ def test_measure_exists(subjects_dir, test_dir, test_subject): If the measure does not exist in the stats file. """ - test_subject = os.path.join(subjects_dir, test_dir, test_subject) + test_subject = subjects_dir / test_dir / test_subject test_file = load_stats_file(test_subject) data = read_measure_stats(test_file) ref_data = read_measure_stats(test_file) @@ -193,20 +209,22 @@ def test_measure_exists(subjects_dir, test_dir, test_subject): @pytest.mark.parametrize("test_subject", load_test_subjects()) -def test_tables(subjects_dir, test_dir, reference_dir, test_subject, thresholds): +def test_tables(subjects_dir: Path, test_dir: Path, reference_dir: Path, test_subject: Path, thresholds): """ Test if the tables are within the threshold. Parameters ---------- - subjects_dir : str + subjects_dir : Path Path to the subjects directory. - test_dir : str + test_dir : Path Name of the test directory. - reference_dir : str + reference_dir : Path Name of the reference directory. - test_subject : str + test_subject : Path Name of the test subject. + thresholds : tuple + Tuple containing the default threshold and the thresholds. Raises ------ @@ -215,10 +233,10 @@ def test_tables(subjects_dir, test_dir, reference_dir, test_subject, thresholds) """ # Load the test and reference tables - test_file = os.path.join(subjects_dir, test_dir, test_subject) + test_file = subjects_dir / test_dir / test_subject test_table = read_table(test_file) - reference_subject = os.path.join(subjects_dir, reference_dir, test_subject) + reference_subject = subjects_dir / reference_dir / test_subject ref_table = read_table(reference_subject) # Load the thresholds