Bugfixes3 #6

suhrig · 2019-06-07T13:56:46Z

prevent ACEseq from stalling and Mpileup from running into the walltime

… R and system commands

…processes as single user

…gs when part of the path does not exist, which makes error messages useless

…where the directory is created or else singularity chokes

vinjana

What a big PR. Looks like a lot of work. Thanks for that!

I guess the XMLs are just copies of the old ones (old PBS cluster!) from our department, right?
Better reconsider your (non)use of quoting and of braces in variables in Bash.

Note that we use these plugin versions (SNVCallingWorkflow_1.2.166-1, ACEseqWorkflow_1.2.10) with much newer versions of Roddy that are better in many respects (bugfixes, better diagnostic output, better parameter checks and diagnostics, etc.). Did you consider updating? Did you have a look at the versions used in OTP? This would also make some hacks with Toollib and BE versions unnecessary. Also the hack with the DefaultPlugin version may get unnecessary. This is just a suggestion.

vinjana · 2019-06-13T14:13:06Z

docker/ACEseqDockerContainer/Dockerfile

 	cd $RODDY_BASE_PLUGINS &&  \
 	ls -l && \
-	(cd DefaultPlugin && git checkout 1.2.0) && \
+	(cd DefaultPlugin && git checkout 951fc80) && \


Why is this necessary? The commit is pretty old -- actually pre-Roddy 3!

vinjana · 2019-06-13T14:15:47Z

docker/ACEseqDockerContainer/Dockerfile

 	cd ACEseqWorkflow_1.2.10  &&  \
-	git checkout 1.2.10
+	git checkout 1.2.10 && \
+	sed -i -e 's/[ \t]\${EMAIL}[ \t]/ "${EMAIL}" /g' $ACESEQ_FOLDER/resources/analysisTools/copyNumberEstimationWorkflow/correct_gc_bias*.sh # wrap variable in quotes, or else e-mail is sent, even when $EMAIL is empty


Typical -- though interesting that this now produces problems. Actually, all variables should (almost) always get quoted in Bash (there are few exceptions). I will also change this code in the ACEseq head with reference to your PR.

vinjana · 2019-06-13T14:22:20Z

docker/ACEseqDockerContainer/Dockerfile

+	sed -i -e 's/[ \t]\${EMAIL}[ \t]/ "${EMAIL}" /g' $ACESEQ_FOLDER/resources/analysisTools/copyNumberEstimationWorkflow/correct_gc_bias*.sh # wrap variable in quotes, or else e-mail is sent, even when $EMAIL is empty

-RUN	conda env create -n ACEseqWorkflow -f $ACESEQ_FOLDER/resources/analysisTools/copyNumberEstimationWorkflow/environments/conda.yml;
+RUN	sed -i -e 's/.*gsl.*//' $ACESEQ_FOLDER/resources/analysisTools/copyNumberEstimationWorkflow/environments/conda.yml && \


BTW: Also in the Docker script all variables should be quoted, unless there is a good reason not to do so (e.g. if R_COMMAND should be allowed to refer to a multi-component command, such as /path/to/R --vanilla).

For instance here, if $ACESEQ_FOLDER contains whitespace your code will fail. You may argue that the variable should not contain whitespace, nobody knows how users are going to use this code.

vinjana · 2019-06-13T14:23:35Z

docker/ACEseqDockerContainer/Dockerfile

+COPY config /home/roddy/config
+
+# create an empty DELLY SV file, for when the workflow is called without SVs
+RUN echo -e 'chrom1\tstart1\tend1\tchrom2\tstart2\tend2\tid\tpairs\tstrand1\tstrand2\tsvtype\tsize\torient\tmapq\tsplit_reads\tsplit_mapq\tsplit_consensus\tpid\taf\tgenotypes\trd_ratio\ttumor_count\tgerm_count\tBf\tdepth.ratio\tCNt\tA\tB\tCN_supp' > /home/roddy/empty_sv.tsv


What a hack. I understand and agree, but anyway, what a hack.

vinjana · 2019-06-13T14:25:54Z

docker/ACEseqDockerContainer/roddy.sh

 # 12: Optional: The SV file

 if [[ $# -lt 11 ]]; then
 	echo "Wrong number of arguments"


Msg. could be improved:

`echo "Wrong number of arguments: Expected 11 but got $#"

vinjana · 2019-06-13T14:59:47Z

docker/BaseDockerContainer/scripts/gridEngineSetup.sh

    qconf -am roddy
    qconf -au roddy users
    qconf -as $HOST
+	# define roddy as the user to run SGE (required by Singularity)


indentation

vinjana · 2019-06-13T15:00:49Z

docker/BaseDockerContainer/scripts/gridEngineSetup.sh

-qconf -aq main.q
-qconf -mq main.q
+function setupGridEngine() {
+changeGridEngineConfig hostname "$hostName" -ae


indentation

vinjana · 2019-06-13T15:01:50Z

docker/BaseDockerContainer/scripts/gridEngineSetup.sh

+# this function creates a temporary non-interactive script that mimicks an interactive editor
+# Usage: changeGridEngineConfig PARAMETER_TO_CHANGE NEW_VALUE QCONF_ARGUMENTS
+function changeGridEngineConfig() {
+PARAMETER="$1"


Note: You can avoid creating global variables using local

local PARAMETER="$1"

Also consider checking correct function usage

local PARAMETER="${1:?PARAMETER no defined}"

Both will improve you code stability for this function that you use multiple times.

vinjana · 2019-06-13T15:10:59Z

docker/MpileupDockerContainer/Dockerfile

+	cd COWorkflowsBasePlugin_1.0.2 && git checkout 1.0.2
+
+# Update BatchEuphoria to a version compatible with SGE
+RUN wget -qO - https://github.com/TheRoddyWMS/BatchEuphoria/releases/download/0.0.3/BatchEuphoria-0.0.3.jar > $RODDY_LIBS/BatchEuphoria-0.0.2.jar


Did you consider instead modifying the build.gradle?

compile 'com.github.theroddywms:BatchEuphoria:0.0.2'
--> compile 'com.github.theroddywms:BatchEuphoria:0.0.3

compile "com.github.theroddywms:RoddyToolLib:0.0.3"
--> compile "com.github.theroddywms:RoddyToolLib:0.0.4"

and then do the build afterwards? This would have the advantage that in every call log (with bash -x roddy.sh the correct version would be reported.

vinjana · 2019-06-13T15:16:24Z

docker/PlatypusDockerContainer/config/ini/alllocal.ini

+pluginDirectories=/home/roddy/binaries/Roddy/plugins_R2.4
+
+[COMMANDS]
+commandLogTruncate=80                       # Truncate logged commands to this length. If <= 0, then no truncation.


Consider setting this to zero, such that the diagnostic messages don't get truncated.

suhrig added 24 commits November 14, 2017 19:15

fix errors in passing SV file

6b9f3ba

quote input and output files of docker run command

d2d750c

mount input files of docker read-only

4d3cd73

increase sharedmem size in docker container for communication between…

d2ca3a2

… R and system commands

usage was truncated

98eef2d

fix incompatibility between versions of DefaultPlugin and Roddy

0c3a136

add mpileup Docker container

8f15409

add platypus Docker container

87e8f38

compatibility with Singularity: make container read-only and run all …

d67b4ff

…processes as single user

move remaining jobs counter into ENTRYPOINT.sh

e611426

use same paths inside container as outside

b45d035

remove interactive flags from docker call

f3a4aa9

move config into container

b96498e

parameterize number of threads

78eec22

suppress some 'Permission denied' messages

a33cd26

use readlink -m instead of -f, because the latter returns empty strin…

4c43bba

…gs when part of the path does not exist, which makes error messages useless

use empty SV file when no SV file is given

1887a4b

move all files edited at runtime to one directory

d7875ab

fixes to enable running as root

904fc7e

add dependencies for ACEseq's HRD computation

02d3568

implement cleanup to reduce container size

fdb8732

replacing writable directories with symlinks must occur in the layer …

4a9f369

…where the directory is created or else singularity chokes

prevent ACEseq from sending mails on QC-failed samples

8404f15

extend walltime of paired-end overlap filter and apply performance hack

b691cca

vinjana requested changes Jun 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfixes3 #6

Bugfixes3 #6

Uh oh!

suhrig commented Jun 7, 2019

Uh oh!

vinjana left a comment

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

vinjana Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bugfixes3 #6

Are you sure you want to change the base?

Bugfixes3 #6

Uh oh!

Conversation

suhrig commented Jun 7, 2019

Uh oh!

vinjana left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants