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

Add new configuration entry for plugins #4611

Conversation

magicDGS
Copy link
Contributor

Includes:

  • Configuration for packages to search read filters
  • Configuration for packages to search annotations

In addition, it changes the behavior of VariantAnnotatorEngine to use the annotation packages from the configuration, and mimic what the plugin is doing. This closes #2155

@magicDGS magicDGS force-pushed the dgs_configure_plugin_packages branch from 610e00d to 891230c Compare April 16, 2018 16:01
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.

@magicDGS One question for you, it seems like it would be better to use the existing method in a loop instead of factoring out another one, but maybe I am missing some detail. Looks good to me otherwise.

for ( final String codecPackage : config.annotation_packages() ) {
finder.find(codecPackage, GenotypeAnnotation.class);
}
return finder.getConcreteClasses().stream().map(c -> (GenotypeAnnotation) ClassUtils.makeInstanceOf(c))
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

final GATKConfig config = ConfigFactory.getInstance().getGATKConfig();

final ClassFinder finder = new ClassFinder();
for ( final String codecPackage : config.annotation_packages() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do this instead of calling ClassUtils.makeInstanceOfSubclasses on each package and merging the results?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is all going to be removed soon by #4674. It might be best to wait for that, since it will reduce what this PR needs to do.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Ok, lets wait for that then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How close is that PR to be accepted? I can divide this one into the ReadFilter config entry and wait for the annotator to be accepted to include the entry (or include it directly in #4674). What do you prefer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer not to add anything else to #4674 (and also to not split this one up). I would just wait until its merged (@droazen still needs to review it) and then rebase this on top of that.

@lbergelson
Copy link
Member

Closing because I've taken this over as #5573

@lbergelson lbergelson closed this Jan 18, 2019
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.

Request: Allow custom packages for Annotations in VariantAnnotatorEngine
3 participants