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

Program group reorganization #1043

Merged
merged 1 commit into from
Jan 5, 2018
Merged

Program group reorganization #1043

merged 1 commit into from
Jan 5, 2018

Conversation

cmnbroad
Copy link
Contributor

@cmnbroad cmnbroad commented Jan 2, 2018

Reorganize program group categories and tool assignments, per @vdauwera. There are 3 separate commits; one with the new program group class definitions; one with the tool assignments; and one to delete the obsolete program groups.

Checklist (never delete this)

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage decreased (-0.03%) to 77.621% when pulling c15fa46 on cn_program_groups into 7c0980b on master.

Copy link
Contributor

@sooheelee sooheelee left a comment

Choose a reason for hiding this comment

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

Everything looks great with one exception.

MakeSitesOnlyVcf should be under Variant Manipulation. Currently, it is under Variant Evaluation and Refinement.

Thank you for all this work.

@@ -36,7 +37,7 @@
"all site level information, including annotations based on genotypes (e.g. AN, AF). Output an be " +
"any support variant format including .vcf, .vcf.gz or .bcf.",
oneLineSummary = "Creates a VCF bereft of genotype information from an input VCF or BCF",
programGroup = VcfOrBcf.class)
programGroup = VariantEvaluationProgramGroup.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

programGroup = VariantManipulationProgramGroup.class

@sooheelee
Copy link
Contributor

I am good with these. @vdauwera Will you want to review too?

@coveralls
Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage decreased (-0.03%) to 77.621% when pulling 9928091 on cn_program_groups into a86b2fc on master.

@coveralls
Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage decreased (-0.03%) to 77.621% when pulling 9928091 on cn_program_groups into a86b2fc on master.

@vdauwera
Copy link
Contributor

vdauwera commented Jan 4, 2018

Ah, I had already looked at it but failed to post my review. All looks good to me!

@sooheelee
Copy link
Contributor

👍🏽

@cmnbroad cmnbroad mentioned this pull request Jan 4, 2018
4 tasks
@ktibbett
Copy link
Contributor

ktibbett commented Jan 4, 2018

I followed the directions for checking out and testing this PR. When I try to run it in the old picard-style without arguments (which should give a list of the tools and their program groups) I get the following error:

Exception in thread "main" java.lang.RuntimeException: The following classes are missing the required CommandLineProgramProperties annotation: CollectWgsMetricsFromQuerySorted, DummyProgram
at picard.cmdline.PicardCommandLine.extractCommandLineProgram(PicardCommandLine.java:132)
at picard.cmdline.PicardCommandLine.instanceMain(PicardCommandLine.java:93)
at picard.cmdline.PicardCommandLine.main(PicardCommandLine.java:108)

@cmnbroad
Copy link
Contributor Author

cmnbroad commented Jan 4, 2018

@ktibbett Hi Kathleen - I'm not sure what directions you're referring to, but CollectWgsMetricsFromQuerySorted has been removed from the Picard repo for a while now (I have no idea what DummyProgram is). Are you sure you did a clean build from this branch ? If I pull this PR and build it, and just run the jar, I get the tool/program group list as expected.

@vdauwera
Copy link
Contributor

vdauwera commented Jan 4, 2018

@ktibbett all basic invocations work for me -- @ktibbett, can you post the command you ran?

@vdauwera
Copy link
Contributor

vdauwera commented Jan 4, 2018

@nh13 @tfenne Would you care to review this? This PR affects how tool docs are categorized; the main driver was to standardize doc categories across Picard and GATK ahead of the GATK 4.0 release, which will produce more user-friendly documentation fro both projects. Shouldn't affect anything on the dev side, except if you have any open branches you'll need to rebase to take in the new program group definitions.

We're hoping to get this merged ASAP once everything checks out in order to finalize preparations on the GATK side, so we would need a quick turnaround. Thanks!

@sooheelee
Copy link
Contributor

@ktibbett, I don't know what old picard-style without arguments refers to. Can you please clarify?

I think the following will be helpful to you.

I built a jar off of this branch and if I run java -jar build/libs/picard.jar -h, I see an index of tools that reflect the new categorization, e.g.:

screenshot 2018-01-04 12 19 34

I am able to look at tool-specific documentation, e.g. with java -jar build/libs/picard.jar FifoBuffer --help:

screenshot 2018-01-04 12 22 38

Sidenote for @cmnbroad: If I run --list instead of -h I get the same tool list and a funny message at the end:

'--list' is not a valid command. See PicardCommandLine -h for more information.

@ktibbett
Copy link
Contributor

ktibbett commented Jan 4, 2018

@vdauwera / @sooheelee , I did a clean checkout and this time it worked; apologies.

@sooheelee
Copy link
Contributor

sooheelee commented Jan 5, 2018

We've waited till EOD for input from @nh13 @tfenne and so will proceed assuming this is all agreeable with them. Since this PR is a blocker for many other Picard program group assignments, I will go ahead and merge. look into why there are conficts when before there were none.

@sooheelee
Copy link
Contributor

Looks like these are due to other doc update PRs that have since been merged. Sorry about that @cmnbroad. Hope you find them easy to resolve.

@nh13
Copy link
Collaborator

nh13 commented Jan 5, 2018

Sorry but I am on vacation so I won’t be able to review. The amount of documentation changes is a bit overwhelming to follow, especially over the holidays! The changes also differ significantly from how I write documentation (not a bad thing, just different), so I will pass on reviewing any documentation changes and focus on functional changes moving forward. Thanks for pushing on all these PRs and including me.

@coveralls
Copy link

coveralls commented Jan 5, 2018

Coverage Status

Coverage decreased (-0.03%) to 77.654% when pulling 5fab7db on cn_program_groups into d8b7653 on master.

@cmnbroad cmnbroad merged commit b9b8777 into master Jan 5, 2018
@cmnbroad cmnbroad deleted the cn_program_groups branch January 5, 2018 14:59
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.

6 participants