Skip to content

Conversation

@taha-abdullah
Copy link
Contributor

Developed a workflow to build, run, and test FastSurfer on every push to the dev repository:

  • The workflow is running on a local runner on the Dirac machine
  • The workflow builds a FastSurfer GPU docker image and runs it on a sample MRI dataset stored on the local runner memory
  • File existence test is run on the output to ensure all the necessary log files have been outputted by the FastSurfer run
  • Error message test is run on the log files to check for any errors which may have occurred during the run

Running this workflow requires a local runner.

For now, the workflow is run manually using a workflow-dispatch trigger for easy testing purposes

taha-abdullah and others added 5 commits March 19, 2024 14:43
- Added yaml files for test_file_existence and test_error_messages
- Added error handling in test_file_existence and test_error_handling
installing dependencies: PyYAML
small workflow to run tests while avoiding building and running FastSurfer
Adding a file existence test to make sure all the
files that are supposed to be in the results are
actually there.
adding a job to test file existence
making jobs sequential by adding dependencies
changed license directory
removed read-only flags from MRI data and License
Added environment variables for MRI data and output
adding docker run command
changed PYTHONPATH to variable
renaming the workflow to 'FastSurfer Docker Image Build'
Create docker_build.yml
Building GPU FastSurfer Image
configuring python
commenting out PYTHONPATH
Building GPU FastSurfer Image and running analysis
simple workflow to test if self hosted runner 'dirac' is running correctly
test_error_messages.py
Rename docker_build.yml to fastsurfer_build.yml
Update tests.yml
separating test-file-existence into new job and adding test-error-messages job
Update tests.yml
updated test_file_existence

update tests.yml

fixed issues in test_file_existence

removed debug print statements

updated test_error_messages
Copy link
Member

@dkuegler dkuegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also multiple extra files which should be deleted.

folder structure for test should be

/test
/test/quick_test/...files....
/test/quick_test/data/...files...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not commit __pycache__ files.


if __name__ == '__main__':

if len(sys.argv) > 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use argparse for parameters.

jobs:

build:
runs-on: dirac
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
runs-on: dirac
runs-on: ci-gpu

# The runner that 'build' will run on
runs-on:
# Self hosted runner called 'dirac'
- dirac
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- dirac
- ci-gpu

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to run all of these tests as singularity containers. I think we do not want to always build the image for that container, but just mount the "checked out" version into the container. But to get the version information in there we need to run the build script.

So:

  1. uses: actions/checkout
  2. prepare job: build.py --device cuda --dry_only (on the checkout -- this creates the BUILD.info file there)
  3. run singularity, but "mount" the "checked out" directory as /fastsurfer-dev as read-only and set the environment variable FASTSURFER_HOME to /fastsurfer-dev run inside the container

bash <<EOF
ln -sf /fastsurfer-dev/checkpoints /fastsurfer/checkpoints
brun_fastsurfer.sh <<EEOF
subject07=/data/...
subject08=/data/...
subject10)=/data/...
EOF

Similarly run the tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add docstrings to python files

# Get the folder path from the YAML file
cls.folder_path = cls.data.get('folder_path')
if not cls.folder_path:
raise Exception("The 'folder_path' key was not found in the YAML file")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise Exception("The 'folder_path' key was not found in the YAML file")
raise ValueError("The 'folder_path' key was not found in the YAML file")

with open(yaml_file_path, 'r') as file:
cls.data = yaml.safe_load(file)
except FileNotFoundError:
raise Exception(f"YAML file not found at path: {yaml_file_path}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise Exception(f"YAML file not found at path: {yaml_file_path}")
raise FileNotFoundError(f"YAML file not found at path: {yaml_file_path}")

except FileNotFoundError:
raise Exception(f"YAML file not found at path: {yaml_file_path}")
except yaml.YAMLError as e:
raise Exception(f"Error parsing YAML file: {e}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise Exception(f"Error parsing YAML file: {e}")
raise RuntimeError(f"Error parsing YAML file: {e}") from e


This method is executed once before any test methods in the class.
"""
yaml_file_path = './test/files.yaml' # './' refers to the current directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best use Path(__file__).parents[0] / "files.yaml"

added fastsurfer_singularity.yaml to run FastSurfer in singularity container
@dkuegler
Copy link
Member

superceeded by #496

@dkuegler dkuegler closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants