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

Update Battenberg v2.2.9 Dockerfile #9

Merged
merged 20 commits into from
Jun 27, 2023

Conversation

Faizal-Eeman
Copy link
Contributor

@Faizal-Eeman Faizal-Eeman commented May 25, 2023

Description

Standardize Dockerfile and update Battenberg v2.2.9

Closes #1

Closes #2

Testing Results

Docker Image Testing

  • I have tested the Docker image with the docker run command as described below.

Test the Docker image with at least one sample. Verify the new Docker image works using:

docker run --rm -u $(id -u):$(id -g) -w=`pwd` --volume=/hot/:/hot/ \
    -v /hot/ref/tool-specific-input/Battenberg/download_202204/GRCh38/battenberg_ref_hg38_chr/:/opt/battenberg_reference/ battenberg:2.2.9 \
    Rscript /usr/local/bin/battenberg_wgs.R \
        -t $tumor_sample_name \
        -n $normal_sample_name \
        --nb $normal_bam \
        --tb $tumor_bam \
        -o $sample_out_dir \
        --sex male

Note: The sex of the sample was arbitrarily assigned to test the docker run.

My command:

Battenberg docker command was wrapped in an sbatch shell script and run on an F32.
sbatch /hot/user/mmootor/temp_files/battenberg/run/test_run_battenberg.sh
Log:

  • /hot/user/mmootor/temp_files/battenberg/run/battenberg.error
  • /hot/user/mmootor/temp_files/battenberg/run/battenberg.log

Test Cases

  • Case 1
    • sample: ILHNLNEV000009-N001-B01-F , ILHNLNEV000009-T002-L01-F
    • normal: /hot/project/disease/HeadNeckTumor/HNSC-000084-LNMEvolution/pipelines/call-gSNP/2020-12-22/ILHNLNEV000009-T002-L01-F//gSNP/2021-01-22_11.01.06/ILHNLNEV000009/SAMtools-1.10_Picard-2.23.3/recalibrated_reheadered_bam_and_bai/ILHNLNEV000009-N001-B01-F_realigned_recalibrated_reheadered.bam
    • tumor: /hot/project/disease/HeadNeckTumor/HNSC-000084-LNMEvolution/pipelines/call-gSNP/2020-12-22/ILHNLNEV000009-T002-L01-F//gSNP/2021-01-22_11.01.06/ILHNLNEV000009/SAMtools-1.10_Picard-2.23.3/recalibrated_reheadered_bam_and_bai/ILHNLNEV000009-T002-L01-F_realigned_recalibrated_reheadered.bam
    • sbatch: /hot/user/mmootor/temp_files/battenberg/run/test_run_battenberg.sh
    • output: /hot/user/mmootor/temp_files/battenberg/run/ILHNLNEV000009-T002-L01-F_subclones.txt
    • Germline plot: /hot/user/mmootor/temp_files/battenberg/run/ILHNLNEV000009-T002-L01-F.germline.png
    • Tumor plot: /hot/user/mmootor/temp_files/battenberg/run/ILHNLNEV000009-T002-L01-F.tumour.png

Checklist

Formatting

File Updates

  • I have ensured that the version number update follows the versioning standards.

  • I have updated the version number in the Dockerfile, README.md and metadata.yaml files.

  • I have updated the dependencies and added my name to the maintainer list in the Dockerfile.

  • I have updated the feature changes in the README.md (optional).

  • I have added the changes included in this pull request to the CHANGELOG.md under the next release version or unreleased, and updated the date.

GitHub Packages Auto Build Rules

  • I have not manually pushed this Docker image to the uclahs-cds container registry (ghcr.io/uclahs-cds) on GitHub.

  • I have updated the image_name in the metadata.yaml which is required by GitHub action to automatically build and push the image.

@Faizal-Eeman
Copy link
Contributor Author

I'll create /hot/software/image/docker-Battengberg after we release this docker image and move all relevant logs and test results to the dev dir of this repo.

Copy link

@yashpatel6 yashpatel6 left a comment

Choose a reason for hiding this comment

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

Some comments:

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
modify_reference_path.sh Outdated Show resolved Hide resolved
@Faizal-Eeman
Copy link
Contributor Author

Tested the image again, all works good.
Any other comments? @tyamaguchi-ucla @yashpatel6

@Faizal-Eeman
Copy link
Contributor Author

Did you want to specify the ASCAT version here?

Image tested with ASCAT version specified. Test was successful. Test location same as before.

Copy link

@tyamaguchi-ucla tyamaguchi-ucla left a comment

Choose a reason for hiding this comment

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

Added a few minor comments but otherwise, looks good to me. Anything else to add @yashpatel6 ?

Comment on lines +6 to +14
ARG HTSLIB_VERSION=1.16
ARG ALLELECOUNT_VERSION=4.3.0
ARG IMPUTE2_VERSION=2.3.2
RUN mamba create -qy -p /usr/local \
-c bioconda \
-c conda-forge \
htslib==${HTSLIB_VERSION} \
cancerit-allelecount==${ALLELECOUNT_VERSION} \
impute2==${IMPUTE2_VERSION}

Choose a reason for hiding this comment

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

No need to worry about this in this PR but we may want to consider creating an image for each tool.

Choose a reason for hiding this comment

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

@Faizal-Eeman can you create a GH issue on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue #12 created.

Dockerfile Outdated Show resolved Hide resolved
Copy link

@yashpatel6 yashpatel6 left a comment

Choose a reason for hiding this comment

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

Looks fine to me, agreed on Taka's comment about trying to have fewer layers

Dockerfile Outdated
Comment on lines 35 to 38
RUN R -q -e 'devtools::install_github("Crick-CancerGenomics/ascat/[email protected]")'

# Install Battenberg 2.2.9
RUN R -q -e 'devtools::install_github("Wedge-Oxford/[email protected]")'

Choose a reason for hiding this comment

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

We might also want to make ASCAT and Battenberg versions args rather than hard-coded here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to take a bit of dirty work to use ARG here as there are both single and double quotes. I'll update this in another PR later.

@Faizal-Eeman
Copy link
Contributor Author

Reduced layers, verified the image with a successful test run.

@Faizal-Eeman Faizal-Eeman merged commit ee2a4a0 into main Jun 27, 2023
@Faizal-Eeman Faizal-Eeman deleted the mmootor-update-battenberg-2-2-9-dockerfile branch June 27, 2023 19:14
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.

Standardize repo Update battenberg image
3 participants