Redbiom initial code #2282

antgonza · 2017-09-13T12:48:37Z

#2278 needs to be merged first.

Extra from adding redbiom:

I rm qiita_pet/templates/analysis_waiting.html cause it's not used anywhere
show_loading was duplicated so just left one and made it work on all other pages

codecov-io · 2017-09-13T14:40:04Z

Codecov Report

Merging #2282 into dev will increase coverage by 0.03%.
The diff coverage is 97.2%.

@@            Coverage Diff             @@
##              dev    #2282      +/-   ##
==========================================
+ Coverage   93.78%   93.81%   +0.03%     
==========================================
  Files         161      163       +2     
  Lines       18249    18427     +178     
==========================================
+ Hits        17115    17288     +173     
- Misses       1134     1139       +5

Impacted Files	Coverage Δ
qiita_pet/webserver.py	`100% <100%> (ø)`	⬆️
qiita_db/test/test_util.py	`99.62% <100%> (+0.01%)`	⬆️
qiita_db/util.py	`96.83% <100%> (+0.15%)`	⬆️
qiita_pet/handlers/qiita_redbiom.py	`94.59% <94.59%> (ø)`
qiita_pet/test/test_qiita_redbiom.py	`98.41% <98.41%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42dd9a9...079f4eb. Read the comment docs.

…edbiom-code

wasade · 2017-09-13T16:48:06Z

qiita_pet/handlers/qiita_redbiom.py

+
+                if message == '':
+                    config = redbiom.get_config()
+                    get = redbiom._requests.make_get(config)


this is a private module, it should not be necessary to call this method

wasade · 2017-09-13T16:49:43Z

qiita_pet/handlers/qiita_redbiom.py

+                        'Not a valid search: "%s", your query is too small '
+                        '(too few letters), try a longer query' % query)
+
+                if message == '':


can you describe in english what the goal of this block is? it looks like the intention is to establish a dict mapping a study, to the artifacts contained in that study that overlap with requested samples? if yes, wouldn't it be easier to just query the qiita API for that?

I guess I'm also a bit confused as to why the artifact IDs are necessary? I'm concerned with the subsequent code as it depends on the structure of redbiom IDs which we do not make guarantees about. If this is necessary (which I think can be handled by cross referencing with the Qiita API), can you please open an issue so that we can construct an API endpoint to retrieve the tags from a set of redbiom IDs?

right! got it now, this was my first interaction with redbiom and you can tell. updating and simplifying, should also address the next comments.

wasade · 2017-09-13T16:58:24Z

qiita_pet/handlers/qiita_redbiom.py

+                    for idx in redbiom.util.ids_from(query, True, 'feature',
+                                                     ctx):
+                        aid, sid = idx.split('_', 1)
+                        sid = sid.split('.', 1)[0]


The sample IDs in qiita are not prefixed with the qiita study id?

wasade · 2017-09-13T16:59:14Z

qiita_pet/handlers/qiita_redbiom.py

+                                                     ctx):
+                        aid, sid = idx.split('_', 1)
+                        sid = sid.split('.', 1)[0]
+                        study_artifacts[sid].append(aid)


Okay, understanding this more. An API method should be exposed in redbiom which yields a the set of observed tags for a given iterable of redbiom IDs.

yup, but for the time being, this works.

antgonza

@wasade, thanks!

antgonza · 2017-09-13T19:15:26Z

qiita_pet/handlers/qiita_redbiom.py

+                        'Not a valid search: "%s", your query is too small '
+                        '(too few letters), try a longer query' % query)
+
+                if message == '':


right! got it now, this was my first interaction with redbiom and you can tell. updating and simplifying, should also address the next comments.

antgonza · 2017-09-13T19:26:02Z

qiita_pet/handlers/qiita_redbiom.py

+                                                     ctx):
+                        aid, sid = idx.split('_', 1)
+                        sid = sid.split('.', 1)[0]
+                        study_artifacts[sid].append(aid)


yup, but for the time being, this works.

josenavas

Few comments

josenavas · 2017-09-14T15:31:37Z

qiita_db/util.py

+            del info["pi_email"]
+            del info["pi_name"]
+
+            infolist.append({


I think if you do this:

info['status'] = qdb.study.Study(info['study_id']).status infolist.append(info)

you get the same result right? It looks like you're creating a dictionary by using the same key values that info has. If the problem is that info has more keys than you need, I think the solution is to delete those keys (as you are doing a couple of lines above).

josenavas · 2017-09-14T15:39:08Z

qiita_pet/handlers/qiita_redbiom.py

+                    message = (
+                        'Not a valid search: "%s", are you sure this is a '
+                        'valid metadata %s?' % (
+                            query, 'value' if search_on == 'metadata' else


The if on this line is redundant (you are sure that search_on == 'metadata' -> see line 43

josenavas · 2017-09-14T15:40:31Z

qiita_pet/handlers/qiita_redbiom.py

+                    message = (
+                        'Not a valid search: "%s", your query is too small '
+                        '(too few letters), try a longer query' % query)
+                if message == '':


Can you use if not message like in line 40, for consistency?

Alternatively, you can use try/except/else:

In [1]: def f(s): ...: try: ...: if s == 'a': ...: raise ValueError('Error!') ...: except ValueError: ...: print "Error happened" ...: else: ...: print "No error" ...: In [2]: f('a') Error happened In [3]: f('b') No error

josenavas · 2017-09-14T15:58:20Z

qiita_pet/handlers/qiita_redbiom.py

+        self.render('redbiom.html')
+
+    @execute_as_transaction
+    def _redbiom_search(self, query, search_on, callback):


I had some trouble following this function, basically because this function is performing 3 different operations, depending on the value of search_on. I would create smaller functions that perform the single operation, so the code is easier to read. Something along this lines:

def _redbiom_metadata_search(self, query, contexts): ... return message, study_artifacts def _redbiom_feature_search(self, query, contexts): ... return message, study_artifacts def _redbiom_taxon_search(self, query, contexts): ... return message, study_artifacts

Then, this function can be simplified as shown below. I know the function is incomplete here but I think this exemplifies the structure that I'm proposing:

def _redbiom_search(self, query, search_on, callback): search_f = {'metadata': self._redbiom_metadata_search, 'feature': _redbiom_feature_search, 'taxon': _redbiom_taxon_search} try: df = readbiom.summarize.contexts() except ConnectionError: message = '...' else: contexts = df.ContextName.values message, study_artifacts = search_f(query, contexts) ...

josenavas · 2017-09-14T15:59:34Z

qiita_pet/handlers/qiita_redbiom.py

+                message = ('Incorrect search by: you can use metadata, '
+                           'features or taxon and you passed: %s' % search_on)
+
+            if message == '':


use if not message

josenavas · 2017-09-14T16:02:38Z

qiita_pet/handlers/qiita_redbiom.py

+    @coroutine
+    @execute_as_transaction
+    def post(self, search):
+        search = self.get_argument('search', None)


It looks like search and search_on are not really optional parameters.
I think the correct thing to do here is:

search = self.get_argument('search') search_on = self.get_argument('search_on')

and let tornado handle if the arguments are not passed. It will return a 400 error message which is the correct code given that the request is incorrect.

josenavas · 2017-09-14T16:03:31Z

qiita_pet/static/js/qiita.js

@@ -115,6 +140,36 @@ function show_hide_process_list() {
  }
 }

+/*


Documentation doesn't fit function signature. Can you update?

josenavas · 2017-09-14T16:05:30Z

qiita_pet/static/js/qiita.js

+  $(button).addClass("btn-info");
+  bootstrapAlert('We are adding ' + aids.length + ' artifact(s) to the analysis. This ' +
+                 'might take some time based on the number of samples on each artifact.', "warning", 10000);
+  $.get('/artifact/samples/', {ids:aids})


Samples are added using a get? That is confusing... get should be use for read-only operations.

that's not actually adding the samples, it's requesting the sample names from the artifacts to add. The addition is actually done in this line: qiita_websocket.send('sel', data['data']);

Thanks for the explanation!

josenavas · 2017-09-14T16:10:50Z

qiita_pet/templates/redbiom.html

+            {% if current_user is not None %}
+              return '<div class="container" style="max-width: 5em;">'+
+                       '<div class="row justify-content-md-center">' +
+                         '<div class="col-md-1 text-center details-control">&nbsp;</div>' +


It looks like these two blocks are the same except for this line? You can then do:

return '<div class="container" style="max-width: 5em;">'+ '<div class="row justify-content-md-center">' + {% if current_user is not None %} '<div class="col-md-1 text-center details-control"> </div>' + {% end %} '<div class="col-md-1 text-center">' + data.length + '</div>' + '</div>' + '</div>';

josenavas · 2017-09-14T16:12:49Z

qiita_pet/templates/redbiom.html

+
+{%block content%}
+  <small>
+    Redbiom only searches on public data and the data is updated nightly. Note that you will only be able to expand and add artifacts to analyses if you are signed into Qiita.


Is this accurate? @wasade will it be possible to update the redis db every night?

We have not touched load mechanisms since June, and there is work to do there, but it should be feasible

yes, that's a discussion for next week's meeting.

antgonza

Thanks @josenavas

antgonza · 2017-09-14T17:17:48Z

qiita_db/util.py

+            del info["pi_email"]
+            del info["pi_name"]
+
+            infolist.append({


antgonza · 2017-09-14T17:18:25Z

qiita_pet/handlers/qiita_redbiom.py

+        self.render('redbiom.html')
+
+    @execute_as_transaction
+    def _redbiom_search(self, query, search_on, callback):


antgonza · 2017-09-14T17:53:44Z

qiita_pet/handlers/qiita_redbiom.py

+                    message = (
+                        'Not a valid search: "%s", your query is too small '
+                        '(too few letters), try a longer query' % query)
+                if message == '':


antgonza · 2017-09-14T17:56:27Z

qiita_pet/handlers/qiita_redbiom.py

+    @coroutine
+    @execute_as_transaction
+    def post(self, search):
+        search = self.get_argument('search', None)


antgonza · 2017-09-14T17:57:46Z

qiita_pet/static/js/qiita.js

@@ -115,6 +140,36 @@ function show_hide_process_list() {
  }
 }

+/*


antgonza · 2017-09-14T18:01:42Z

qiita_pet/static/js/qiita.js

+  $(button).addClass("btn-info");
+  bootstrapAlert('We are adding ' + aids.length + ' artifact(s) to the analysis. This ' +
+                 'might take some time based on the number of samples on each artifact.', "warning", 10000);
+  $.get('/artifact/samples/', {ids:aids})


that's not actually adding the samples, it's requesting the sample names from the artifacts to add. The addition is actually done in this line: qiita_websocket.send('sel', data['data']);

antgonza · 2017-09-14T18:03:51Z

qiita_pet/templates/redbiom.html

+            {% if current_user is not None %}
+              return '<div class="container" style="max-width: 5em;">'+
+                       '<div class="row justify-content-md-center">' +
+                         '<div class="col-md-1 text-center details-control">&nbsp;</div>' +


antgonza · 2017-09-14T18:04:11Z

qiita_pet/templates/redbiom.html

+
+{%block content%}
+  <small>
+    Redbiom only searches on public data and the data is updated nightly. Note that you will only be able to expand and add artifacts to analyses if you are signed into Qiita.


yes, that's a discussion for next week's meeting.

josenavas · 2017-09-14T18:20:39Z

qiita_pet/static/js/qiita.js

+  $(button).addClass("btn-info");
+  bootstrapAlert('We are adding ' + aids.length + ' artifact(s) to the analysis. This ' +
+                 'might take some time based on the number of samples on each artifact.', "warning", 10000);
+  $.get('/artifact/samples/', {ids:aids})


Thanks for the explanation!

antgonza added 4 commits September 11, 2017 15:33

init commit

5e99089

cleaning code for initial review

8ab4066

install latest redbiom

d3590f6

fix test

ab397c1

Merge branch 'dev' of https://github.com/biocore/qiita into initial-r…

eef49b1

…edbiom-code

antgonza mentioned this pull request Sep 13, 2017

Adding Redbiom to master #2131

Closed

9 tasks

wasade reviewed Sep 13, 2017

View reviewed changes

addressing @wasade comments and adding other tests

e482bce

antgonza commented Sep 13, 2017

View reviewed changes

flake8

25ee54f

josenavas suggested changes Sep 14, 2017

View reviewed changes

addressing @josenavas comments

079f4eb

antgonza commented Sep 14, 2017

View reviewed changes

josenavas approved these changes Sep 14, 2017

View reviewed changes

josenavas merged commit 9e1c1b5 into qiita-spots:dev Sep 14, 2017

@@ @@ -115,6 +140,36 @@ function show_hide_process_list() { @@
                 }
               }
+              /*

Redbiom initial code #2282

Redbiom initial code #2282

Uh oh!

Conversation

antgonza commented Sep 13, 2017

Uh oh!

codecov-io commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antgonza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-io commented Sep 13, 2017 •

edited

Loading