Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dockerfiles and latest analysis script #8

Merged
merged 9 commits into from
Aug 29, 2024
Merged

Conversation

K-Meech
Copy link
Collaborator

@K-Meech K-Meech commented Aug 28, 2024

Closes #2

This adds the docker setup files + the latest analysis script
Main changes from the old setup:

  • Added installation of zip to the dockerfile
  • Updated FSL to the latest version (6.0.7.13)
  • Updated distancemap commands for latest FSL version (fixes suggested by Hamied)
  • Made sure t1size is an array (otherwise causes errors as t1size[1] etc is empty)
  • Minor changes to docker / analysis script to satisfy the linters. I also removed many of the commented out sections in the analysis script + lines mentioning dates/times when code was added
  • I modified the linting setup slightly. Both shellcheck and hadolint now use a warning threshold, otherwise they were failing for many minor style issues (flagged as info). I still had to exclude some rules for both that didn't have easy fixes. Happy to change this if you think there's a better solution!

@K-Meech K-Meech requested a review from p-j-smith August 28, 2024 16:51
Copy link
Collaborator

@p-j-smith p-j-smith left a comment

Choose a reason for hiding this comment

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

Thanks Kimberly for getting this in, it looks great! A couple of small suggestions but nothing blocking

I'll open an issue to switch to using conda to install FSL

docker build -f Dockerfile -t fsl_test .
```

- Create `code` and `data` directories inside the `Enigma-PD-WML` directory
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could create an empty code directory with a .gitkeep file in it and commit it to this repo just so users don't need to create it themselves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Although I'm wondering if we should move away from recommending creating the code/data dirs directly inside the github repo dir anyway? As we're setting each as explicit volumes here: docker run -v "$(pwd)":/home -v "$(pwd)"/code:/code -v "$(pwd)"/data:/data fsl_test >> docker_log.txt 2>&1, it should work with a directory in any location. This would also mean we could use the same setup instructions for pulling the image from dockerhub, where we won't have the github repo locally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ooo that's a much better idea! Shall we leave it as is for now then and update it once the image is on dockerhub?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also (side note) - I'm not sure we need all three volumes? /home is probably sufficient by itself, as long as the code and data dirs are always nested inside?

Comment on lines +8 to +10
RUN wget https://fsl.fmrib.ox.ac.uk/fsldownloads/fslconda/releases/fslinstaller.py && python fslinstaller.py -d /usr/local/fsl/ -V 6.0.7.13

RUN echo '\n # FSL Setup \nFSLDIR=/usr/local/fsl \nPATH=${FSLDIR}/share/fsl/bin:${PATH} \nexport FSLDIR PATH \n. ${FSLDIR}/etc/fslconf/fsl.sh' >> /root/.bashrc
Copy link
Collaborator

Choose a reason for hiding this comment

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

not for this PR, but as you mentioned offline, let's think about using installing only conda to install only the necessary FSL packages, perhaps using miniforge as a base image

- --ignore=DL3008
- --ignore=DL3007
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I think this makes sense here - the base image only has one tag anyway and that's latest

Dockerfile Outdated Show resolved Hide resolved
@K-Meech K-Meech merged commit ec719a5 into main Aug 29, 2024
1 check passed
@K-Meech K-Meech deleted the km/add-new-script branch August 29, 2024 09:41
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.

Add Dockerfile for running analysis pipeline
2 participants