Migrate variant browser to postgres #528

talavis · 2019-03-22T13:07:23Z

Describe the pull request:

Pull request long description:

Migration of the variant browser from mongo to postgres. Also includes a lot of bug fixes in both backend and frontend. A lot of browser tests are also added.

Changes made:

A lot.

…many other things

…_rsid

…t_minimal_representation as it seems unused

…lems

…unctions running

…. Most hacks can be removed once database is fixed

viklund

I've gone over most of the python code. None of the JS so far.

Apart from the specific comments I have left I have a few general ones.

The beacon style dataset reference parsing. Wouldn't it be better to put that in a function and call that in all handlers that get the dataset name. That way we are both future proof and it's easier to find all the places to fix the beacon-version parsing bugs.
There's a lot of [x for x in db.query.dicts()], esp. in the lookups module, is this really necessary? In one or two cases maybe, but I feel that the conversion from peewee objects into dicts should be made as late as possible. Maybe this requires a larger refactor than what we should do at the moment, but I would like to make sure.
This is even more general. Should the exception handling happen close to where the exceptions happen or should the lookups just let them pass and then we deal with them in the handlers. The latter approach would (possibly) reduce the amount of exception handling. I'm not super convinced either way, but as it is now the exceptions are handled in both the lookup and the handler (although, there exceptions are the case when there's a None return from the underlying lookup function).

For example GetGene in browser_handlers.py. Line 89 checks wheter gene in None, which only happens in case get_gene in lookups.py (line 196) gets an exception. To me this looks like we are doing double work.

backend/beacon.py

backend/db.py

backend/handlers.py

backend/modules/browser/browser_handlers.py

viklund · 2019-03-29T13:33:29Z

backend/modules/browser/lookups.py

+                                         (db.Coverage.pos <= end_pos) &
+                                         (db.Coverage.chrom == chrom) &
+                                         (db.Coverage.dataset_version == dataset_version.id))
+                                  .dicts())]


Why can't you just return the db.Coverage...where() witihout .dicts() and without a list-comprehension?

The list comprehension is needed; otherwise it only returns a SelectQuery object that needs to be executed on the database.

viklund · 2019-03-29T13:41:29Z

backend/modules/browser/lookups.py

+        return db.Gene.select().where((db.Gene.gene_id == gene_id) &
+                                      (db.Gene.reference_set == ref_set)).dicts().get()
+    except db.Gene.DoesNotExist:
+        return None


Can't this be in just one try: with two except clauses instead, would be a bit clearer to me at least.

viklund · 2019-03-29T13:47:38Z

backend/modules/browser/lookups.py

+                                        (db.Gene.start <= stop_pos) &
+                                        (db.Gene.stop >= start_pos) &
+                                        (db.Gene.chrom == chrom)).dicts()
+    return [gene for gene in gene_query]


Again, I'm not really sure you need to do the dicts() and list comprehension here.

viklund · 2019-03-29T13:55:10Z

backend/modules/browser/lookups.py

+    if variant['rsid']:
+        if not str(variant['rsid']).startswith('rs'):
+            variant['rsid'] = 'rs{}'.format(variant['rsid'])
+    return variant


Maybe this method should instead be:

variant = get_raw_variant(dataset, pos, chrom, ref, alt, ds_version) if variant and 'rsid' in variant and not str(variant['rsid']).startswith('rs'): variant['rsid'] = f"{rs{variant['rsid']}" return variant

(the main point was the updated ifs, not the f-string)

frontend/src/js/controller.browserController.js

viklund

Went through the js as well.

frontend/src/js/controller.browserController.js

frontend/templates/ng-templates/browser-region.html

viklund · 2019-03-29T15:02:44Z

frontend/templates/ng-templates/browser-region.html

-                <p>Your region is too large. Please submit a region of at most 100 kb.</p>
-                <p>If you require larger regions, please see our <a href="/dataset/{{ ctrl.dataset.shortName }}/download">Download</a> page for raw data.</p>
-            </div>
+    <div ng-if='ctrl.region.statusText=="The region is too large"'>


I'm not sure about comparing with a string in this way here. Isn't there a statusCode for too large region?

I'm using 400 (bad request) for too large region. 400 is also used for problems with parsing the region (which in my opinion also fits bad request). Thus the need to check the actual text. But maybe I should change it to ===?

frontend/templates/ng-templates/browser-region.html

talavis · 2019-03-29T17:09:25Z

Fine by me. Though there are ~20 functions using dataset.
Apparently the .dicts() call performs the select. I got used that I need I either had to use it as an iterator (thus the for part) or perform a second call with execute. As for using dicts, that's only done because the exac browser was doing it. I've been considering to change to peewee objects, but chose to mimic exac for this initial migration.
I've been trying to keep the exception handling close to where it occurs, but I'm open to do it mainly in the handlers instead.

…the same reference set

viklund

Very good. Just a small comment about the parse_beacon_dataset.

backend/db.py

…s no rsid

talavis added 30 commits March 20, 2019 15:56

Autocomplete working with pgsql

49eb56a

skeleton for get_variant_list

010b761

first function passing test

e8c5e7e

transcript retrieval working

b85793e

some get_variant functions fixed

370066c

coverage

1ba5c04

potentially no longer needed function added

5313802

in progress for variants_from_transcript

43bbf71

a bit of test fixing

989d860

multiple updates to tests and code

dd0b5a5

keep on fixing tests and lookups

267d083

more updates, focused on logging, adding version as a parameter, and …

20ef941

…many other things

fixes and tests for get_variants_by_rsid

b3e0305

get_variants_in region migrated, many other fixes in tests and code

8f73f67

added the functionality of get_variants_from_dbsnp to get_variants_by…

4fc466a

…_rsid

awesomebar seems to work

8ae076e

Fix for Google style for docstrings, some other changes

8c83365

starting to migrate the utils code

7b45a8b

first tests

bc67b6e

improved test coverage, also a few small bugs fixed

a2eba10

adding tests and documentation

914442a

tests for get_flags_from_variant(), worst_csq_from_list(); removed ge…

dcc489f

…t_minimal_representation as it seems unused

didn't confirm that genes were retrieved from the correct reference set

d40d7a1

fixes to allow docker testing

7799446

start of conversion of handlers

5f43978

more fixes, especially in GetVariation

8719831

variation page is working, but with a few hacks to get around db prob…

179b878

…lems

new function for retrieving genes by dbid; mixed updates to get all f…

416cfd0

…unctions running

functions migrated from mongodb.py to pgsql.py, bug fixes

dccf4d9

A couple of ugly hacks, but now seems to work for everything but gene…

a882929

…. Most hacks can be removed once database is fixed

pymongo is not used in beacon anymore

781c22e

viklund requested changes Mar 29, 2019

View reviewed changes

viklund reviewed Mar 29, 2019

View reviewed changes

frontend/src/js/controller.browserController.js Outdated Show resolved Hide resolved

viklund requested changes Mar 29, 2019

View reviewed changes

talavis added 8 commits March 29, 2019 18:21

Remove unneeded .dicts()

7ad4d35

variant is visible in URL, no reason to show it on bad load

b72c7f6

make sure location{} is rendered as a code block by sphinx

52b74c1

use variants.loaded to tell if the loading is done

eb57294

make a strict comparison

9eb7b2e

remove whitespace

c65622b

updated db docstrings

61e8234

move dataset beacon-style parsing to its own function

61c7be5

talavis force-pushed the feature/browser-postgres branch from 013e547 to 61c7be5 Compare April 1, 2019 10:00

check if dataset is in beacon style for each dataset request

421c279

talavis force-pushed the feature/browser-postgres branch from af2e9ff to 421c279 Compare April 1, 2019 10:28

refactor ifs for rsid

40b10cf

talavis force-pushed the feature/browser-postgres branch from b779b2b to 40b10cf Compare April 1, 2019 11:14

population frequencies added to variant view for other datasets with …

827109d

…the same reference set

viklund requested changes Apr 1, 2019

View reviewed changes

backend/db.py Outdated Show resolved Hide resolved

talavis force-pushed the feature/browser-postgres branch 2 times, most recently from 9a86973 to 5c17392 Compare April 1, 2019 20:23

move parse_dataset to utils, return a tuple of (dataset, version)

cab3370

talavis force-pushed the feature/browser-postgres branch from 5c17392 to cab3370 Compare April 1, 2019 20:29

talavis added 3 commits April 1, 2019 22:43

tests for parse_dataset

1e9f52d

avoid getting rsNone as rsid

5167214

avoid implying that a variant is not available in dbSNP because it ha…

bee90bd

…s no rsid

viklund approved these changes Apr 2, 2019

View reviewed changes

viklund merged commit 8874f2b into develop Apr 2, 2019

viklund deleted the feature/browser-postgres branch April 2, 2019 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate variant browser to postgres #528

Migrate variant browser to postgres #528

talavis commented Mar 22, 2019

viklund left a comment

viklund Mar 29, 2019

talavis Mar 29, 2019 •

edited

Loading

viklund Mar 29, 2019

viklund Mar 29, 2019

viklund Mar 29, 2019

viklund left a comment

viklund Mar 29, 2019

talavis Mar 29, 2019

talavis commented Mar 29, 2019

viklund left a comment

Migrate variant browser to postgres #528

Migrate variant browser to postgres #528

Conversation

talavis commented Mar 22, 2019

Describe the pull request:

Pull request long description:

Changes made:

viklund left a comment

Choose a reason for hiding this comment

viklund Mar 29, 2019

Choose a reason for hiding this comment

talavis Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

viklund Mar 29, 2019

Choose a reason for hiding this comment

viklund Mar 29, 2019

Choose a reason for hiding this comment

viklund Mar 29, 2019

Choose a reason for hiding this comment

viklund left a comment

Choose a reason for hiding this comment

viklund Mar 29, 2019

Choose a reason for hiding this comment

talavis Mar 29, 2019

Choose a reason for hiding this comment

talavis commented Mar 29, 2019

viklund left a comment

Choose a reason for hiding this comment

talavis Mar 29, 2019 •

edited

Loading