Skip to content

TASK-6324 - Simplify QC analysis by launching a single job #2469

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 25 commits into
base: release-3.x.x
Choose a base branch
from
Open

TASK-6324 - Simplify QC analysis by launching a single job #2469

wants to merge 25 commits into from

Conversation

@jtarraga
Copy link
Member

@jtarraga jtarraga commented Jun 19, 2024 •
edited
Loading

TASK-6324 Simplify QC analysis by launching a single job:

  • Alignment QC analysis

  • Sample QC analysis

jtarraga added 16 commits May 31, 2024 13:38
@jtarraga
analysis: update alignment QC analysis in order to launch a single jo…
833d0f3 
…b that executes sequentially samtools, plot-bamstats and fastqc, #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentFastQcMetricsAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/executors/DockerWrapperAnalysisExecutor.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/samtools/SamtoolsWrapperAnalysisExecutor.java
	modified:   opencga-analysis/src/test/java/org/opencb/opencga/analysis/alignment/AlignmentAnalysisTest.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/OpenCgaToolExecutor.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/result/ExecutionResultManager.java
@jtarraga
Merge branch 'develop' into TASK-6324
483c1be
@jtarraga
app: fix circos.R and move to app/analysis/genome-plot, #TASK-6326, #…
b0aea7b 
…TASK-6324

On branch TASK-6324
Changes to be committed:
	new file:   opencga-app/app/analysis/genome-plot/circos.R
@jtarraga
analysis: add FastQC docker cli in the result.json file, and remove u…
333e6dc 
…nused parameters, #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/fastqc/FastqcWrapperAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/fastqc/FastqcWrapperAnalysisExecutor.java
@jtarraga
analysis: add Samtools docker CLI in the result.json file, and remove…
177f1a2 
… unused parameters, #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/samtools/SamtoolsWrapperAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/samtools/SamtoolsWrapperAnalysisExecutor.java
@jtarraga
analysis: use ToolRunner to execute each alignment QC step (i.e.: sam…
512c17d 
…tools stats/flagstats, fastqc), #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
@jtarraga
analysis: support the flag overwrite, and forward excetions, and mino…
65a1bdd 
…r sonnar issues, #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
@jtarraga
test: add JUnit tests for alignment QC, #TASK-6325, #TASK-6324
55c81ff 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/test/java/org/opencb/opencga/analysis/alignment/AlignmentAnalysisTest.java
@jtarraga
analysis: add docker CLI and parameters in the tool step, fix some ty…
0d78abd 
…pos and sonnar issues, #TASK-6326, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/mutationalSignature/MutationalSignatureAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/mutationalSignature/MutationalSignatureLocalAnalysisExecutor.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/variant/MutationalSignatureAnalysisExecutor.java
@jtarraga
analysis: add parameters in the tool step, #TASK-6326, #TASK-6324
af024c6 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/stats/SampleVariantStatsAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/stats/SampleVariantStatsLocalAnalysisExecutor.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/variant/SampleVariantStatsAnalysisExecutor.java
@jtarraga
analysis: add docker CLI and parameters in the tool step (in genome p…
edb882b 
…lot analysis), fix some typos and sonnar issues, #TASK-6326, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/genomePlot/GenomePlotAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/genomePlot/GenomePlotLocalAnalysisExecutor.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/variant/GenomePlotAnalysisExecutor.java
@jtarraga
analysis: add the method addStepAttributes, #TASK-6326, #TASK-6324
39c2f6a 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java
@jtarraga
analysis: use the ToolRunner instead of launching jobs for each sampl…
7e3ca47 
…e QC step, #TASK-6326, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/sample/qc/SampleQcAnalysis.java
@jtarraga
analysis: remove unused get/set methods and R scripts, #TASK-6325, #T…
f88c703 
…ASK-6324

On branch TASK-6324
Changes to be committed:
	deleted:    opencga-analysis/src/main/R/genome-plot/circos.R
	deleted:    opencga-analysis/src/main/R/mutational-signature/mutational-signature.r
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java
@jtarraga
analysis: update circos/genome plot to take the R script from the ana…
aed7911 
…lysis folder, #TASK-6326, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/circos/CircosLocalAnalysisExecutor.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/genomePlot/GenomePlotLocalAnalysisExecutor.java
@jtarraga
tests: add and update JUnit tests for sample QC, #TASK-6326, #TASK-6324
592c793 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/OpenCGATestExternalResource.java
	modified:   opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/VariantAnalysisTest.java
	deleted:    opencga-storage/opencga-storage-core/src/test/resources/AR2.10039966-01T.copynumber.caveman.vcf.gz
	deleted:    opencga-storage/opencga-storage-core/src/test/resources/AR2.10039966-01T_vs_AR2.10039966-01G.annot.brass.vcf.gz
	deleted:    opencga-storage/opencga-storage-core/src/test/resources/AR2.10039966-01T_vs_AR2.10039966-01G.annot.pindel.vcf.gz
	new file:   opencga-storage/opencga-storage-core/src/test/resources/cancer-cnvs.vcf.gz
	new file:   opencga-storage/opencga-storage-core/src/test/resources/cancer-indels.vcf.gz
	new file:   opencga-storage/opencga-storage-core/src/test/resources/cancer-rearrs.vcf.gz
	new file:   opencga-storage/opencga-storage-core/src/test/resources/cancer-snvs.vcf.gz
	new file:   opencga-storage/opencga-storage-core/src/test/resources/genome-plot-config.json
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 19, 2024
View reviewed changes
...ava/org/opencb/opencga/analysis/variant/mutationalSignature/MutationalSignatureAnalysis.java
logger.info("Signagture fit max. rare sigs.: {}", signatureParams.getFitMaxRareSigs());
logger.info("Signagture fit signatures file: {}", signaturesFile);
logger.info("Signagture fit rare signatures file: {}", rareSignaturesFile);
logger.info("Signature id: {}", signatureParams.getId());

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZAvf8j-gFvsJ1eI17VU-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=opencb_opencga&issues=AZAvf8j-gFvsJ1eI17VU&open=AZAvf8j-gFvsJ1eI17VU&branch=TASK-6324">SonarCloud</a></p>
...ava/org/opencb/opencga/analysis/variant/mutationalSignature/MutationalSignatureAnalysis.java
logger.info("Signagture fit signatures file: {}", signaturesFile);
logger.info("Signagture fit rare signatures file: {}", rareSignaturesFile);
logger.info("Signature id: {}", signatureParams.getId());
logger.info("Signature description: {}", signatureParams.getDescription());

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZAvf8j-gFvsJ1eI17VV-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=opencb_opencga&issues=AZAvf8j-gFvsJ1eI17VV&open=AZAvf8j-gFvsJ1eI17VV&branch=TASK-6324">SonarCloud</a></p>
@jtarraga jtarraga requested a review from j-coll June 19, 2024 09:36
j-coll
j-coll requested changes Jun 19, 2024
View reviewed changes
...cga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java Outdated
throw new ToolException("Error copying Samtools flagstat results", e);
}
} else {
throw new ToolException("Something wrong happened running Samtools flagstat analysis");
Copy link
Member

Choose a reason for hiding this comment

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

Your future self will appreciate if you add some extra information here. Like the number of lines, and/or the actual first line

opencga-analysis/src/main/java/org/opencb/opencga/analysis/sample/qc/SampleQcAnalysis.java
}
} else {
String msg = "Skipping sample variant stats analysis by user";
addWarning(msg);
Copy link
Member

Choose a reason for hiding this comment

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

Is this even a warning, or should it be just an info event?

Copy link
Member Author

Choose a reason for hiding this comment

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

hard to say!

opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java Outdated
erm.addAttribute(key, value);
}

protected final void addStepAttributes(ExecutionResult executionResult) throws ToolException {
Copy link
Member

Choose a reason for hiding this comment

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

You might want to include more information from the nested ExecutionResult, like events (maybe that's the only interesting thing)

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean to add the "nested" ExecutionResult events into the list of "parent" Execution events?,

Copy link
Member

Choose a reason for hiding this comment

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

Yes, something like that. Otherwise, they're lost (not technically, as you can always read the nested job result file, but they'd be quite hidden).

jtarraga added 3 commits June 19, 2024 17:11
@jtarraga
analysis: add a new tool step to update the alignment QC, #TASK-6325,…
9f6913b 
… #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
@jtarraga
analysis: minor improvements in sample QC analysis, #TASK-6326, #TASK…
4518a72 
…-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/sample/qc/SampleQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java
	modified:   opencga-core/src/main/java/org/opencb/opencga/core/tools/result/ExecutionResultManager.java
@jtarraga
analysis: read the stderr file to get error messages when something w…
06d46a7 
…as wrong running dockers (samtools stats/flagstats and fastqc), #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentFastQcMetricsAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/wrappers/executors/DockerWrapperAnalysisExecutor.java
	modified:   opencga-analysis/src/test/java/org/opencb/opencga/analysis/alignment/AlignmentAnalysisTest.java
@jtarraga jtarraga requested a review from j-coll June 20, 2024 07:53
j-coll
j-coll reviewed Jun 20, 2024
View reviewed changes
...cga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java Outdated
protected void run() throws ToolException {
// Create the tool runner
toolRunner = new ToolRunner(opencgaHome, catalogManager, StorageEngineFactory.get(variantStorageManager.getStorageConfiguration()));
toolRunner = new ToolRunner(getOpencgaHome().toString(), catalogManager,
Copy link
Member

Choose a reason for hiding this comment

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

You can better use the constructor

  • public ToolRunner(String opencgaHome, CatalogManager catalogManager, VariantStorageManager variantStorageManager)

instead of

  • ToolRunner(String opencgaHome, CatalogManager catalogManager, StorageEngineFactory storageEngineFactory)

@jtarraga
analysis: add the result of the "nested" analysis to the current step…
37faea0 
… attributes, #TASK-6325, #TASK-6324

On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/sample/qc/SampleQcAnalysis.java
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/tools/OpenCgaTool.java
@jtarraga jtarraga requested a review from j-coll June 21, 2024 09:40
jtarraga added 2 commits June 21, 2024 11:43
@jtarraga
analysis: use a simpler ToolRunner constructor, #TASK-6325, #TASK-6324
0531c48 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/alignment/qc/AlignmentQcAnalysis.java
@jtarraga
analysis: use a simpler ToolRunner constructor, #TASK-6326, #TASK-6324
a53bd04 
On branch TASK-6324
Changes to be committed:
	modified:   opencga-analysis/src/main/java/org/opencb/opencga/analysis/sample/qc/SampleQcAnalysis.java
@jtarraga jtarraga changed the title Simplify QC analysis by launching a single job TASK-6324 Simplify QC analysis by launching a single job Jun 21, 2024
j-coll
j-coll previously approved these changes Jun 25, 2024
View reviewed changes
@j-coll j-coll changed the title TASK-6324 Simplify QC analysis by launching a single job TASK-6324 - Simplify QC analysis by launching a single job Jun 25, 2024
@jtarraga jtarraga changed the base branch from develop to release-3.2.x July 22, 2024 10:17
@jtarraga jtarraga dismissed j-coll’s stale review July 22, 2024 10:17

The base branch was changed.

@halender
Copy link
Contributor

Task linked: TASK-6324 Simplify QC analysis by launching a single job

@juanfeSanahuja juanfeSanahuja self-requested a review August 14, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juanfeSanahuja juanfeSanahuja juanfeSanahuja approved these changes

@j-coll j-coll j-coll left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtarraga @halender @j-coll @juanfeSanahuja