Integrate variation-commons into eva-pipeline #150

108krohan · 2019-03-24T13:38:44Z

Mechanism Iterative success testing with Travis CLI with 1 file change. Why? To ensure test-cases failures can be identified effectively and corrected.

With this pull-request, eva-pipeline shall make use of following existing classes from variation-commons:

Phase 1 (Viability Test)

HgvsMongo (replaces HgvsMongo of eva-pipeline/commons/models)

Phase 2

XrefMongo (replaces Xref of eva-pipeline/commons/models)

Phase 3

VariantAtMongo (replaces VariantAt of eva-pipeline/commons/models)

Phase 4

VariantSourceEntryMongo (replaces VariantSourceEntry of eva-pipeline/commons/models)

Phase 5

Annotation (replaces Annotation of eva-pipeline/commons/models)
ConsequenceTypeMongo (replaces ConsequenceType of eva-pipeline/commons/models)
ScoreMongo (replaces Score of eva-pipeline/commons/models)

Phase 6

Correct README.md URL for Variant Browser
Simplified Annotation (replaces SimplifiedAnnotation of /eva-pipeline/commons/models)

…urceEntryMongo

sundarvenkata-EBI · 2019-04-01T10:31:28Z

src/main/java/uk/ac/ebi/eva/pipeline/io/mappers/AnnotationLineMapper.java

+    public AnnotationMongo mapLine(String line, int lineNumber) {
+        ConsequenceTypeMongo consequenceType = new ConsequenceTypeMongo(null, null,
+            null, null, null, null, null, null,
+            null, null, null, null, null, null);


Why not use the Empty constructor for ConsequenceTypeMongo?

Use of an empty constructor for ConsequenceTypeMongo results in build error - (actual and formal argument lists differ in length). This is because the default constructor isn't public.

PR - 81 tries to address this issue.

The error-message is as follows:
constructor uk.ac.ebi.eva.commons.mongodb.entities.subdocuments.ConsequenceTypeMongo.ConsequenceTypeMongo( java.lang.String, java.lang.String, java.lang.String, java.lang.String, java.lang.String, java.lang.Integer, java.lang.Integer, java.lang.Integer, java.lang.String, java.lang.String, uk.ac.ebi.eva.commons.mongodb.entities.subdocuments.ScoreMongo, uk.ac.ebi.eva.commons.mongodb.entities.subdocuments.ScoreMongo, java.util.Set<java.lang.Integer>, java.lang.Integer) is not applicable

(actual and formal argument lists differ in length)

For reference: https://github.com/EBIvariation/variation-commons/blob/fda0d8bbf44f029ab7ef1f756f31964b3bc9e2c5/variation-commons-mongodb/src/main/java/uk/ac/ebi/eva/commons/mongodb/entities/subdocuments/ConsequenceTypeMongo.java#L99

Hi Sundar, I was mulling over this comment and I found the following SO article useful: https://stackoverflow.com/questions/19395973/maven-adding-a-reference-to-a-parent-pom-project

The last two paragraphs in the answer are:

"Update: You can do that in a limited way only for the dependencyManagement area with the scope import but this is only intended for using in the depdencyManagement and not for dependencies. An other solution might be to create a separate dummy project which has the dependencies too two childs as transitive deps. But this is not really beauty.

So the solution is simply to add the two or more dependencies directly which you are using in you project and that's it.

Parent POM isn't for that. To create a meta-project, create a regular Maven project with a name like common-libraries and reference all of your other projects as dependencies. Parent POM is to build several libraries in one step, and automatically use the correct build order."

What this suggests is to add the the dependencies directly. I just wanted to know how this can be resolved by tonight or tomorrow.

sundarvenkata-EBI · 2019-04-01T10:35:08Z

pom.xml

@@ -113,6 +113,16 @@
                </exclusion>
            </exclusions>
        </dependency>


Any reason why the DependencyManagement tag (referring to variation-commons) isn't included?

I did not include the DependencyManagement tag because it results in test errors if I do so The test errors say they cannot find the configurations. This might possibly be because eva-pipeline's POM already has a parent POM - for reference: Spring Boot Dependency Management with a Custom Parent.

Also, Travis CLI build passes without the tag. So the other reason was to keep the eva-pipeline POM lean.

To test if I could add dependencyManagement, I added the following to eva-pipeline's POM, which results in test errors (because it is unable to find Spring configurations).
I'm new to Maven so I'm not 100% sure about my reasons. So please find the code used to include the tag:

<dependencyManagement> <dependencies> <dependency> <groupId>uk.ac.ebi.eva</groupId> <artifactId>variation-commons</artifactId> <version>0.7-SNAPSHOT</version> <type>pom</type> <scope>import</scope> </dependency> </dependencies> </dependencyManagement>

Errors received were like so:

java.lang.NoSuchMethodError: org.springframework.util.ClassUtils.getQualifiedMethodName(Ljava/lang/reflect/Method;Ljava/lang/Class;)Ljava/lang/String; at org.springframework.transaction.interceptor.TransactionAspectSupport.methodIdentification(TransactionAspectSupport.java:396)

Hi, please have a look at this comment thread.

My current understanding (something I haven't tried yet) is to include dependencies via a singular variation-commons.

So for example, only the following tag should suffice:

<dependency> <groupid>uk.ac.eba.eva</groupid> <artifactid>variation-commons</artifactid> <version>0.7-SNAPSHOT</version> </dependency>

sundarvenkata-EBI · 2019-04-01T10:39:58Z

src/main/java/uk/ac/ebi/eva/pipeline/io/mappers/AnnotationLineMapper.java

+            null,
+            Collections.singleton(consequenceType));
+
+        AnnotationMongo AnnotationWithXref = new AnnotationMongo(


I am not sure what this additional annotation object is about??

This additional annotation object is because of the way AnnotationMongo.java's class methods are defined in VariationCommons. The addConsequenceType and addConsequenceTypes functions are private. They are made accessible via concatenate command, which accesses addConsequenceTypes. addConsequenceTypes accesses private addConsequenceType.

Additional annotation object is created because as of now, we need to call concatenate to access addConsequenceType of AnnotationMongo. And concatenate accepts an Annotation as parameter. That is also why 1st I create ConsequenceTypeMongo, and then pass it into constructor of Annotation object.

Why we need additional annotation object is because when concatenate method also updates XrefMongo attribute. That's why I named the additional object AnnotationWithXref and did not pass currentAnnotation directly.

Referring code snippet AnnotationMongo.java#L207

public AnnotationMongo concatenate(AnnotationMongo annotation) { AnnotationMongo temp = new AnnotationMongo(this); if (annotation.getConsequenceTypes() != null) { temp.addConsequenceTypes(annotation.getConsequenceTypes()); } return temp; }

sundarvenkata-EBI · 2019-04-01T10:47:11Z

.../uk/ac/ebi/eva/commons/models/converters/data/VariantSourceEntryToDBObjectConverterTest.java

@@ -63,6 +63,7 @@ public void setUp() {
        file.addAttribute("QUAL", "0.01");
        file.addAttribute("AN", "2");
        file.addAttribute("MAX.PROC", "2");
+        file.setFormat("gzip");


This is so because of the new VariantSourceEntryMongo. The samp field doesn't populate if format field of file isn't set. So that the samp field doesn't populate, which results in following test assertion-error:

VariantSourceEntryToDBObjectConverterTest.testConvertToStorageTypeWithSamples:114 expected:<{ "fid" : "f1" , "sid" : "s1" , "attrs" : { "QUAL" : "0.01" , "AN" : "2" , "MAX?PROC" : "2"} , "samp" : { "def" : "0/0" , "0/1" : [ 1] , "1/1" : [ 2]}}> but was:<{ "fid" : "f1" , "sid" : "s1" , "attrs" : { "QUAL" : "0.01" , "AN" : "2" , "MAX?PROC" : "2"}}>

Added the following line to correct this:
file.setFormat("gzip");

which then allowed for samp to be set, but now also required that mongoFileWithIds has format field present, as you can see in following test assertion-error message:

VariantSourceEntryToDBObjectConverterTest.testConvertToStorageTypeWithSamples:115 expected:<{ "fid" : "f1" , "sid" : "s1" , "attrs" : { "QUAL" : "0.01" , "AN" : "2" , "MAX?PROC" : "2"} , "samp" : { "def" : "0/0" , "0/1" : [ 1] , "1/1" : [ 2]} , **"fm" : "gzip"**}> but was:<{ "fid" : "f1" , "sid" : "s1" , "attrs" : { "QUAL" : "0.01" , "AN" : "2" , "MAX?PROC" : "2"}}>

So also added mongoFileWithIds.put("fm", "gzip"); line to mongoFileWithIds because now we have file.setFormat("gzip");.

sundarvenkata-EBI · 2019-04-01T10:51:21Z

src/test/java/uk/ac/ebi/eva/pipeline/io/writers/AnnotationMongoWriterTest.java

+            VEP_VERSION,
+            VEP_CACHE_VERSION,
+            null,
+            Collections.singleton(consequenceType));


Would moving this up, to where the Annotation instantiation originally was, have any impact?

Yes, when I test with moving this up to where the Annotation was originally instantiated, it is not possible for me to assign consequenceType object to the Annotation object. This is because AnnotationMongo does not have a setter, and the addConsequenceType method is private as of now (accessible only via concatenate method. But concatenate method also expects an annotation object which has it's consequenceType field populated. So the only way to set consequenceType is via AnnotationMongo's constructor. So that's why it's instantiation now happens after consequenceType. So consequence type can be passed as parameter in the constructor of AnnotationMongo.

sundarvenkata-EBI · 2019-04-01T10:51:47Z

src/test/java/uk/ac/ebi/eva/pipeline/io/writers/AnnotationMongoWriterTest.java

+            null,
+            Collections.singleton(consequenceType));
+
+        annotation = annotation.concatenate(annotation);


Again I am a bit unclear on why we append an Annotation object to itself...

Concatenate method calls within it addConsequenceTypes which calls addConsequenceType for each consequenceType in the consequenceType set. AddConsequenceType also sets the Xref fields for AnnotationMongo object, besides just copying over ConsequenceType information. So that's why we call the concatenate function, and not return the annotation object directly (so as to set Xref fields as well).

Rohan Kumar added 3 commits March 24, 2019 19:00

Replace Annotation Metadata

56e7f0e

Replace HgvsMongo with import from variation-commons-mongodb

1474e08

Erase Xref and import XrefMongo instead

2c861ef

108krohan changed the title ~~Replace Annotation Metadata~~ Integrate variaton-commons into eva-pipeline Mar 24, 2019

Rohan Kumar added 3 commits March 24, 2019 20:32

Replace VariantAt with VariantAtMongo from commons-mongodb

4f58bf1

Make eva-pipeline use variation-commons' VariantAtMongo and VariantSo…

daca45a

…urceEntryMongo

Use Annotation, ConsequenceType, and Score from variation-commons

be45575

108krohan changed the title ~~Integrate variaton-commons into eva-pipeline~~ Integrate variation-commons into eva-pipeline Mar 25, 2019

sundarvenkata-EBI requested review from cyenyxe and sundarvenkata-EBI March 27, 2019 08:58

Rohan Kumar added 2 commits March 28, 2019 10:21

Make eva-pipeline use Simplified Annotation of variation-commons

5ebacd5

Minor README fix to correct markdown for Variant Brownser URL

c5e6bff

sundarvenkata-EBI reviewed Apr 1, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate variation-commons into eva-pipeline #150

Integrate variation-commons into eva-pipeline #150

108krohan commented Mar 24, 2019 •

edited

Loading

sundarvenkata-EBI Apr 1, 2019

108krohan Apr 1, 2019 •

edited

Loading

108krohan Apr 1, 2019

108krohan Apr 3, 2019

sundarvenkata-EBI Apr 1, 2019 •

edited

Loading

108krohan Apr 1, 2019

108krohan Apr 2, 2019 •

edited

Loading

sundarvenkata-EBI Apr 1, 2019 •

edited

Loading

108krohan Apr 1, 2019

sundarvenkata-EBI Apr 1, 2019

108krohan Apr 1, 2019

sundarvenkata-EBI Apr 1, 2019

108krohan Apr 1, 2019

sundarvenkata-EBI Apr 1, 2019

108krohan Apr 1, 2019

Integrate variation-commons into eva-pipeline #150

Are you sure you want to change the base?

Integrate variation-commons into eva-pipeline #150

Conversation

108krohan commented Mar 24, 2019 • edited Loading

Phase 1 (Viability Test)

Phase 2

Phase 3

Phase 4

Phase 5

Phase 6

Choose a reason for hiding this comment

108krohan Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundarvenkata-EBI Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

108krohan Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

sundarvenkata-EBI Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

108krohan commented Mar 24, 2019 •

edited

Loading

108krohan Apr 1, 2019 •

edited

Loading

sundarvenkata-EBI Apr 1, 2019 •

edited

Loading

108krohan Apr 2, 2019 •

edited

Loading

sundarvenkata-EBI Apr 1, 2019 •

edited

Loading