Fix 2307 v1 #2314

nathandunn · 2019-11-07T21:45:22Z

Replaces my bad merge in #2310

get tests to pass by uploading new data
delete old sequences
add new sequence if not there
change length of sequence if not there

nathandunn · 2019-11-07T22:56:45Z

@abretaud I'll continue to take a look, but feel free to push here, as well.

nathandunn · 2019-11-10T22:13:22Z

@abretaud I added a change to allow sequence to be writeable, which was causing errors in some of my changes.

I guess the thing I'm a little unsure of whether you want to allow deletion / update of Sequence / Scaffold objects in the code, or just the addition of them. If you allow deletion, you have to delete all of the annotations associated with them (or explicitly check and disallow them). If you allow updates, then this code should work. My intuition is that we simply don't allow alteration of existing sequences, just addition of new scaffolds. From the original problem description, it sounded like you only wanted to be able to add them.

I think the current issue now is that some of the tests fail (4) because the organism is created with a genomic FASTA sequence and then updated for a refSeq.json. Because the original keeps the FASTA sequence it gets confused, so I think it ends up creating a second sequence.

Anyway, my recommendation:

I'll check in some fixes that will make tests more consistent (e.g., ordering sequences when they come back and fix a flag
@abretaud rewrite tests to do updates for genomic data only or refSeq.json data only (https://github.com/GMOD/Apollo/pull/2314/files#diff-dd23bc0329e522c1d6a07295b706dc1fR1295)
@abretaud decides if he wants to allow removals, or updates of sequences, or simply additions only (I am voting for the latter) and then :
- @nathandunn will rewrite portions to accomadate that
@nathandunn will clean up

abretaud · 2019-11-13T17:00:22Z

@abretaud rewrite tests to do updates for genomic data only or refSeq.json data only

Hum, it should be fasta for all test data I'm using, it's the dataset_x_files dirs in https://github.com/galaxy-genome-annotation/python-apollo/tree/2c916d295776c6a57dd015464dd7b1b30eb79b7f/test-data
I can have a look at it tomorrow maybe

@abretaud decides if he wants to allow removals, or updates of sequences, or simply additions only (I am voting for the latter)

additions -> yes, we need it for sure
removals -> yes, I don't want to keep sequences that were removed in the genome sequence. So I guess removing annotations/alterations on it would make sense
updates -> I know it sounds scary, but there are valid cases where you want to do it (e.g. you created an organism, you realise there is an error in one scaffold sequence that no one has worked on => you just want to update the sequence, and forget about the old one). I guess it's ok to delete annotations/alterations too as they will probably not make sense if the sequence changed.

There is always the no_reload_sequences param when you want to be sure not to lose anything

So it would look like this:

foreach(old seqs):
    if the sequence is no longer ni the new fasta:
        delete the sequence + annotations/alterations

foreach(new seq):
    if the sequence is already in db:
        if the length is the same
            keep it
        else:
            update it
            delete annotations/alterations
    else:
        add the new sequence to the db

abretaud · 2019-11-14T19:08:50Z

Some progress:

added an optional env var to the docker image to ease debugging
2 tests were failing just because I forgot to set the option I was testing (damn!)
Still 2 failing tests in remote mode, when expecting apollo to reload the sequences, I get this in the logs:

2019-11-14 18:56:06,353 [http-nio-8080-exec-6] DEBUG apollo.OrganismController  - Updating organism info null
Genome FASTa seq/genome.fasta
is a genome fasta
done is a genome fasta
should be laoding a data file here null , /home/abretaud/git/python-apollo/apollo_shared_dir/org2, /home/abretaud/git/python-apollo/apollo_shared_dir/org2 , false

It seems like organismDataFile is null there while it should not: do you see a reason why? Something in the handling of uploaded file (I think it is sent properly by python-apollo)?

nathandunn · 2019-11-14T20:50:35Z

@abretaud I think the problem is that things are being posted potentially wrong.

I have a piece of code here:

Apollo/grails-app/controllers/org/bbop/apollo/OrganismController.groovy

Lines 1306 to 1308 in e5dfdde

    
           if (!request instanceof ShiroHttpServletRequest) { 
        
             organismDataFile = request.getFile(FeatureStringEnum.ORGANISM_DATA.value) 
        
           }

That checks for a regular shiro request instead of a form post type:

If I remove the check I get:

No signature of method: org.apache.shiro.web.servlet.ShiroHttpServletRequest.getFile()

Note that I don't have that check in other parts of my code, but I think this method gets used in other ways, so its probably still applicable?

Maybe the autoconvert_to_json flag being passed in on the post is getting ignored so its not setting the header to ?
https://github.com/galaxy-genome-annotation/python-apollo/blob/master/apollo/remote/__init__.py#L170

The flag should probably be multipart/form-data? I'm not sure . . it looks like addOrganismWithSequence works just fine. I'm wondering if somehow its getting passed differently somehow.

nathandunn · 2019-11-14T21:10:48Z

@abretaud / @erasche The default communication I use (at least on the GWT-side) is:

        builder.setHeader("Content-type", "application/x-www-form-urlencoded");
        builder.setHeader("Accept", "application/json");

I noticed you don't actually seem to test the addOrganismWithSequence as far as I can tell, so that piece of code doesn't get tested.

I added this just to confirm my suspicions:

diff --git a/grails-app/controllers/org/bbop/apollo/OrganismController.groovy b/grails-app/controllers/org/bbop/apollo/OrganismController.groovy
index 9759777bb..155450754 100644
--- a/grails-app/controllers/org/bbop/apollo/OrganismController.groovy
+++ b/grails-app/controllers/org/bbop/apollo/OrganismController.groovy
@@ -254,6 +254,14 @@ class OrganismController {
   def addOrganismWithSequence() {
 
 
+    if (!request instanceof ShiroHttpServletRequest) {
+      println "ADD ORG request is not a shiro one ${request}"
+//      organismDataFile = request.getFile(FeatureStringEnum.ORGANISM_DATA.value)
+    }
+    else{
+      println "ADD ORG IS A shiro request ${request}"
+    }
+
     JSONObject returnObject = new JSONObject()
     JSONObject requestObject = permissionService.handleInput(request, params)
     log.info "Adding organism with SEQUENCE ${requestObject as String}"
@@ -1304,8 +1312,13 @@ class OrganismController {
 
         CommonsMultipartFile organismDataFile = null
         if (!request instanceof ShiroHttpServletRequest) {
+          println "request is not a shiro one ${request}"
           organismDataFile = request.getFile(FeatureStringEnum.ORGANISM_DATA.value)
         }
+        else{
+          println "IS A shiro request ${request}"
+        }
+//        CommonsMultipartFile organismDataFile = request.getFile(FeatureStringEnum.ORGANISM_DATA.value)
         String foundBlatdb = null
         if (organismDataFile) {
           File archiveFile = new File(organismDataFile.getOriginalFilename())

abretaud · 2019-11-18T14:41:30Z

To be sure I sniffed the http traffic with wireshark: addOrganismWithSequence and updateOrganismInfo posted content look very similar, and both have the multipart/form-data content type (added automatically by python requests module)
So I guess the problem is probably on the grails/Apollo side

nathandunn · 2019-11-18T16:44:04Z

Do their headers and other content being sent look similar?

…

On Nov 18, 2019, at 6:41 AM, Anthony Bretaudeau ***@***.***> wrote: To be sure I sniffed the http traffic with wireshark: addOrganismWithSequence and updateOrganismInfo posted content look very similar, and both have the multipart/form-data content type (added automatically by python requests module) So I guess the problem is probably on the grails/Apollo side — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2314?email_source=notifications&email_token=AAFXNKTLSRQNC7FQ5V4V6O3QUKSRZA5CNFSM4JKOI342YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEKUZHQ#issuecomment-555043998>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXNKQMXVSAA2J57PDKGJTQUKSRZANCNFSM4JKOI34Q>.

abretaud · 2019-11-18T16:54:23Z

Yep they do!

A wireshark screenshot when adding an organism:

And when updating an organism:

nathandunn · 2019-11-18T17:59:48Z

That is strange . . okay . . I'll take a look. Probably tomorrow for me, though. This is very helpful.

nathandunn · 2019-11-19T19:53:22Z

@abretaud So in some cases form data is being sent over and in some cases it is not.

So for example here:

ERROR: test_update_organism (test.organism_test.OrganismTest)

root: INFO: curl http://localhost:8080/apollo/organism/updateOrganismInfo -H 'Content-Type: application/json' -d '"{\"username\": \"admin@local.host\", \"password\": \"password\", \"name\": \"temp_org\", \"species\": \"updatedspecies\", \"publicMode\": false, \"noReloadSequences\": false, \"directory\": \"/Users/nathandunn/repositories/python-apollo/apollo_shared_dir/org1\", \"blatdb\": \"/Users/nathandunn/repositories/python-apollo/apollo_shared_dir/org1/seq/genome.2bit\", \"genus\": \"updatedgenus\", \"id\": 1000}"'
urllib3.connectionpool: DEBUG: Starting new HTTP connection (1): localhost:8080
urllib3.connectionpool: DEBUG: http://localhost:8080 "POST /apollo/organism/updateOrganismInfo HTTP/1.1" 200 None

and here:

ERROR: test_update_organism_changedseq (test.organism_test.OrganismTest)
root: INFO: curl http://localhost:8080/apollo/organism/updateOrganismInfo -H 'Content-Type: application/json' -d '"{\"username\": \"admin@local.host\", \"password\": \"password\", \"name\": \"temp_org\", \"species\": \"updatedspecies\", \"publicMode\": false, \"noReloadSequences\": false, \"directory\": \"/Users/nathandunn/repositories/python-apollo/apollo_shared_dir/org_update_changedseq\", \"blatdb\": \"/Users/nathandunn/repositories/python-apollo/apollo_shared_dir/org1/seq/genome.2bit\", \"genus\": \"updatedgenus\", \"id\": 1004}"'
urllib3.connectionpool: DEBUG: Starting new HTTP connection (1): localhost:8080
urllib3.connectionpool: DEBUG: http://localhost:8080 "POST /apollo/organism/updateOrganismInfo HTTP/1.1" 200 None

But in some cases it does get sent through as multi-part form data.

So, I think there are two methods for calling updateOrganism as well. One in the organism/__init__.py file and one int the remote/__init__.py file. Sometimes the response should be posted as JSON (no multipart data) and the rest of the time it should be fine without.

So, my guess is that its calling the wrong thing in the wrong place. I'll have to explore it.

nathandunn · 2019-11-20T03:00:59Z

@abretaud The problem was that the updateOrganismInfo in some instances call was updating a common directory to multiple organisms as per design. However, doesn't make for a great test. I rewrote that part. There was also a part with the changing the decoding check, but I'm not sure if it actually did anything (it didn't break anything).

nathandunn · 2019-11-20T03:27:32Z

@abretaud I am going to merge this in for now in order to simplify the testing, but we can add more for sure

abretaud and others added 7 commits November 7, 2019 13:19

add a todo

d3bc376

more awkward pirouettes...

a1dab7c

solve chicken and egg problem

ced1474

try (just a little bit harder)

a365c01

try to implement #2307

10a29e9

try to reduce docker image size

d825bc8

fixes #2307

83d1a58

nathandunn added this to In progress in 2.5.0 via automation Nov 7, 2019

nathandunn mentioned this pull request Nov 7, 2019

Update organism: reloading only SOME sequences #2310

Closed

abretaud and others added 5 commits November 8, 2019 07:26

fix

880fb25

fix

1182e1b

making sequence no longer read-only

1b6f881

fixed the order of the organism being fixed

d0bb805

reversed the reloading flag

53d8432

nathandunn and others added 3 commits November 13, 2019 11:50

merged in develop

8e43ef4

Merge branch 'develop' into fix-2307-v1

ea87360

add env var to enable more debug in docker mode

e5dfdde

nathandunn mentioned this pull request Nov 14, 2019

Update organism: reloading only SOME sequences galaxy-genome-annotation/python-apollo#22

Merged

minor updates

e884f18

fixed the multipart form check

b2ddb1c

nathandunn merged commit 5c36e8d into develop Nov 20, 2019

2.5.0 automation moved this from In progress to Done Nov 20, 2019

cmdcolin deleted the fix-2307-v1 branch December 7, 2023 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2307 v1 #2314

Fix 2307 v1 #2314

nathandunn commented Nov 7, 2019 •

edited

nathandunn commented Nov 7, 2019

nathandunn commented Nov 10, 2019 •

edited

abretaud commented Nov 13, 2019

abretaud commented Nov 14, 2019

nathandunn commented Nov 14, 2019

nathandunn commented Nov 14, 2019

abretaud commented Nov 18, 2019

nathandunn commented Nov 18, 2019 via email

abretaud commented Nov 18, 2019

nathandunn commented Nov 18, 2019

nathandunn commented Nov 19, 2019

nathandunn commented Nov 20, 2019

nathandunn commented Nov 20, 2019

Fix 2307 v1 #2314

Fix 2307 v1 #2314

Conversation

nathandunn commented Nov 7, 2019 • edited

nathandunn commented Nov 7, 2019

nathandunn commented Nov 10, 2019 • edited

abretaud commented Nov 13, 2019

abretaud commented Nov 14, 2019

nathandunn commented Nov 14, 2019

nathandunn commented Nov 14, 2019

abretaud commented Nov 18, 2019

nathandunn commented Nov 18, 2019 via email

abretaud commented Nov 18, 2019

nathandunn commented Nov 18, 2019

nathandunn commented Nov 19, 2019

nathandunn commented Nov 20, 2019

nathandunn commented Nov 20, 2019

nathandunn commented Nov 7, 2019 •

edited

nathandunn commented Nov 10, 2019 •

edited