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

Don't require python to just instantiate tool classes. #8128

Merged
merged 2 commits into from
Dec 23, 2022

Conversation

cmnbroad
Copy link
Collaborator

@cmnbroad cmnbroad commented Dec 13, 2022

The CNN tools launch python immediately even when they're just instantiated, rather than waiting until the tool actually starts executing. This can cause some build tasks (gatkDoc, gatkWDLGen, etc.) to fail if python isn't available.

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@cmnbroad I would the initializers to onStartup with the pythonPackage check unless there's a reason not to that I'm missing.

@@ -140,6 +139,9 @@ protected void onStartup() {

@Override
protected Object doWork() {
// Start the Python executor. This does not actually start the Python process, but fails if python can't be located
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this should be in onStartup.

@@ -127,6 +126,9 @@ protected void onStartup() {

@Override
protected Object doWork() {
// Start the Python executor. This does not actually start the Python process, but fails if python can't be located
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 this should probably go in onStartup

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #8128 (b33ff46) into master (84ade40) will increase coverage by 40.205%.
The diff coverage is 100.000%.

Additional details and impacted files
@@               Coverage Diff                @@
##              master     #8128        +/-   ##
================================================
+ Coverage     46.436%   86.641%   +40.205%     
- Complexity     26473     38958     +12485     
================================================
  Files           2336      2336                
  Lines         182709    182731        +22     
  Branches       20060     20067         +7     
================================================
+ Hits           84843    158320     +73477     
+ Misses         91960     17367     -74593     
- Partials        5906      7044      +1138     
Impacted Files Coverage Δ
...ellbender/tools/walkers/vqsr/CNNScoreVariants.java 78.696% <100.000%> (+61.304%) ⬆️
...hellbender/tools/walkers/vqsr/CNNVariantTrain.java 60.563% <100.000%> (+29.577%) ⬆️
...der/tools/walkers/vqsr/CNNVariantWriteTensors.java 85.714% <100.000%> (+53.571%) ⬆️
.../scalable/data/LabeledVariantAnnotationsDatum.java 68.421% <0.000%> (-1.023%) ⬇️
...titute/hellbender/utils/help/GATKGSONWorkUnit.java 100.000% <0.000%> (ø)
...notyper/GenotypeCalculationArgumentCollection.java 89.474% <0.000%> (ø)
...allerReadThreadingAssemblerArgumentCollection.java 62.500% <0.000%> (ø)
...roadinstitute/hellbender/tools/LocalAssembler.java 67.425% <0.000%> (+0.073%) ⬆️
...r/tools/walkers/varianteval/VariantEvalEngine.java 84.040% <0.000%> (+0.249%) ⬆️
...itute/hellbender/utils/report/GATKReportTable.java 70.370% <0.000%> (+0.370%) ⬆️
... and 830 more

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

👍

@lbergelson lbergelson merged commit 633aa4a into master Dec 23, 2022
@lbergelson lbergelson deleted the cn_fix_cnn_tools branch December 23, 2022 20:18
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