Initial commit of annotation service. #7

Jessime · 2018-09-18T00:41:19Z

The pipeline has had a successful end-to-end run of annotating variants
and inserting them into BQ. Many components are inflexible, ignore edge
cases, or are otherwise brittle. Efforts have been made to document
current issues with TODO comments.

The pipeline has had a successful end-to-end run of annotating variants and inserting them into BQ. Many components are inflexible, ignore edge cases, or are otherwise brittle. Efforts have been made to document current issues with TODO comments.

bashir2

Thanks again Jessime for this prototype. I have made a bunch of comments, mostly at a high level without getting into minor issues. I think we can check in this code with enough warnings in the README file; another option is to leave it as an open PR for someone to take over in future (like the gcp-variant-transforms counterpart PR). Which way do you prefer?

bashir2 · 2018-09-20T18:26:35Z

service/example_client.py

+import sys
+import uuid
+import requests
+import apache_beam as beam


Does this file belong to variant-annotation repo? I would have expected a pure client code here, i.e., clients_tests above (e.g., to annotate a single VCF file using the server) not something that uses Beam. If you want to keep it this way maybe it should be moved elsewhere like gcp-variant-transforms.

I was thinking of this file as my small integration test for the server, but I agree that it probably makes more sense to move to gcp-variant-transforms.

(I'll leave it in for now, but feel free to move it in the future.)

bashir2 · 2018-09-20T18:27:26Z

service/client_tests.py

@@ -0,0 +1,98 @@
+import sys


Please make sure you add the header doc strings before, especially the Apache licence related lines (and a short description for each module).

bashir2 · 2018-09-21T13:51:05Z

service/client_tests.py

+        except ValueError:
+            raise ValueError(response.text)
+
+if __name__ == '__main__':


nit: Please add a usage text and print it when there are not two arguments.

bashir2 · 2018-09-21T13:55:18Z

service/example_client.py

+    with beam.Pipeline(options=options) as p:
+        results = (p | beam.io.ReadFromText(infile)
+                     | beam.ParDo(KeyGenFn())
+                     | beam.GroupByKey()


We should avoid this GroupByKey; maybe it is easier to remove this module from this repo for now and discuss this issue in the other PR in gcp-variant-transforms repo.

bashir2 · 2018-09-21T13:56:43Z

service/example_client.py

+            'staging_location': 'gs://jessime_test_bucket/staging',
+            'temp_location': 'gs://jessime_test_bucket/temp',
+            'job_name': 'vep-as-a-service8',
+            'setup_file': './setup.py',


Where is setup.py?

Here's a specific reason why it makes more sense to keep this in gcp-variant-transforms. This setup.py file refers to Variant Transforms main setup.py file.

bashir2 · 2018-09-21T14:55:59Z

service/server/main.py

+    print 'VEP CACHE setup.'
+
+
+#Global calls after `setup_vep_cache` is defined but before `app` decorators


Why these need to be placed before app decorators? Can we protect this block by an if block such that this code is not executed when the module is imported? If not, is it possible to move these lines to a separate module that depends on this? I am asking because for example writing unit-tests in the current form is problematic.

If you place app = Flask(__name__) after app decorators, you'll get a NameError, since app isn't defined yet.

It should be fine to put the setup_vep_cache() behind if __name__ == '__main__'. But app = Flask(__name__) might cause issues (with Gunicorn, for example, though I would have to investigate more).

bashir2 · 2018-09-21T15:32:48Z

service/README.md

@@ -0,0 +1,3 @@
+# Annotation Server
+
+## TODO: add descriptions of architecture, development, deployment, etc.


For now, please add a note that this is a prototype that is not productionized yet but has been shown to scale well with Variant Transforms and add a link to your other PR: googlegenomics/gcp-variant-transforms#361

Done. I've also added a few lines about how to launch the server.

bashir2 · 2018-09-21T15:59:37Z

service/server/main.py

+    vep_file = 'vep_cache_homo_sapiens_GRCh38_91.tar.gz'
+    gcs_vep_file = os.path.join(gcs_dir, vep_file)
+    local_vep_file = os.path.join(local_cache, vep_file)
+    check_call('gsutil cp {} {}'.format(gcs_vep_file, local_cache).split())


Please add a TODO for handling errors.

bashir2 · 2018-09-21T16:06:14Z

service/server/main.py

+    return header_names, annotation_lines
+
+
+def vep_tab_to_json(response):


So it seems response is both input and output for this function, correct? If yes, can you separate it and do not mutate the input. Also it would be more readable that any response that is not over the network be renamed to something more descriptive, e.g., annotation_response or maybe annotation_stdout? I thought that should make understanding the code easier.

Yep, I already have a TODO to not mutate response. I've added another to make the name more descriptive.

bashir2 · 2018-09-21T16:07:25Z

service/server/main.py

+    """Updates response dict to contain annotations grouped by their variants
+
+    Args:
+        response: Dict containing JSON response to send to client


Please add an example for both input and output.

Done. Will have to modify slightly when refactoring to avoid mutation.

Jessime

I only addressed the points I had time to address and made fixes that affected documentation but not code.

Looking forward to seeing this in production eventually!

Jessime · 2018-09-21T18:43:11Z

service/README.md

@@ -0,0 +1,3 @@
+# Annotation Server
+
+## TODO: add descriptions of architecture, development, deployment, etc.


Done. I've also added a few lines about how to launch the server.

Jessime · 2018-09-21T18:46:56Z

service/example_client.py

+import sys
+import uuid
+import requests
+import apache_beam as beam


I was thinking of this file as my small integration test for the server, but I agree that it probably makes more sense to move to gcp-variant-transforms.

(I'll leave it in for now, but feel free to move it in the future.)

Jessime · 2018-09-21T18:48:25Z

service/example_client.py

+            'staging_location': 'gs://jessime_test_bucket/staging',
+            'temp_location': 'gs://jessime_test_bucket/temp',
+            'job_name': 'vep-as-a-service8',
+            'setup_file': './setup.py',


Here's a specific reason why it makes more sense to keep this in gcp-variant-transforms. This setup.py file refers to Variant Transforms main setup.py file.

Jessime · 2018-09-21T18:59:53Z

service/server/Dockerfile

+RUN perl INSTALL.pl \
+    --AUTO a \
+    --NO_UPDATE
+WORKDIR ..


Absolutely. I initially started using both the Dockerfile and the run_vep.sh, but was running into issues. I unfortunately don't remember the specifics, but I was having trouble getting those to run on GAE with Flask, the Dockerfile in particular. The two lines that took the most effort to get right were:

FROM gcr.io/google_appengine/python

CMD gunicorn -b :$PORT -t 600 main:app

I eventually gave up trying to modify the existing files (going "top-down", if you will), and decided to go "bottom-up" by building off of GPC minimalist examples. My intention was to continue building up functionality, using the original files as endpoints and guides.

Jessime · 2018-09-21T19:02:37Z

service/server/Dockerfile

+ENV VEP_SCRIPT ensembl-vep/vep
+ENV ASSEMBLY GRCh38
+
+# Set virtualenv environment variables. This is equivalent to running


Added a couple of deleted lines back.

Jessime · 2018-09-21T19:10:47Z

service/server/main.py

+    vep_file = 'vep_cache_homo_sapiens_GRCh38_91.tar.gz'
+    gcs_vep_file = os.path.join(gcs_dir, vep_file)
+    local_vep_file = os.path.join(local_cache, vep_file)
+    check_call('gsutil cp {} {}'.format(gcs_vep_file, local_cache).split())


Jessime · 2018-09-21T19:31:41Z

service/server/main.py

+    return header_names, annotation_lines
+
+
+def vep_tab_to_json(response):


Yep, I already have a TODO to not mutate response. I've added another to make the name more descriptive.

Jessime · 2018-09-21T20:03:42Z

service/server/main.py

+    """Updates response dict to contain annotations grouped by their variants
+
+    Args:
+        response: Dict containing JSON response to send to client


Done. Will have to modify slightly when refactoring to avoid mutation.

Jessime · 2018-09-21T20:33:55Z

service/client_tests.py

@@ -0,0 +1,98 @@
+import sys


Jessime · 2018-09-21T20:38:41Z

service/server/main.py

+    print 'VEP CACHE setup.'
+
+
+#Global calls after `setup_vep_cache` is defined but before `app` decorators


If you place app = Flask(__name__) after app decorators, you'll get a NameError, since app isn't defined yet.

It should be fine to put the setup_vep_cache() behind if __name__ == '__main__'. But app = Flask(__name__) might cause issues (with Gunicorn, for example, though I would have to investigate more).

Initial commit of annotation service.

597a7a6

The pipeline has had a successful end-to-end run of annotating variants and inserting them into BQ. Many components are inflexible, ignore edge cases, or are otherwise brittle. Efforts have been made to document current issues with TODO comments.

Jessime requested a review from bashir2 September 18, 2018 13:59

bashir2 reviewed Sep 21, 2018

View reviewed changes

Documentation update based on review comments.

97cf7ad

Jessime commented Sep 21, 2018

View reviewed changes

bashir2 mentioned this pull request Oct 25, 2018

Annotation as a Service #5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit of annotation service. #7

Initial commit of annotation service. #7

Jessime commented Sep 18, 2018

bashir2 left a comment

bashir2 Sep 20, 2018

Jessime Sep 21, 2018

bashir2 Sep 20, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

bashir2 Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

bashir2 Sep 21, 2018

Jessime Sep 21, 2018

Jessime left a comment

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

Jessime Sep 21, 2018

		print 'VEP CACHE setup.'


		#Global calls after `setup_vep_cache` is defined but before `app` decorators

		@@ -0,0 +1,3 @@
		# Annotation Server

		## TODO: add descriptions of architecture, development, deployment, etc.

		return header_names, annotation_lines


		def vep_tab_to_json(response):

Initial commit of annotation service. #7

Are you sure you want to change the base?

Initial commit of annotation service. #7

Conversation

Jessime commented Sep 18, 2018

bashir2 left a 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

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

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

Jessime left a 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

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