Source format selection #111

maxvonhippel · 2017-05-16T21:08:20Z

In regard to issue 75: added optional source format selection.

No change in behavior for BIOMV210Format .biom files, but now BIOMV100Format .biom files are allowed if you specify the format as BIOMV100Format.

REST API updated to include the key-value pair in the relevant JSON objects. Note that if the value is null, I put in a null (None) value rather than skipping the pair altogether. If this is not the desired behavior I can easily change the code accordingly.

…ifacts

…ource format, and tested on my machine and found that behaviour is as expected both with and without supplied format for BIOMV100Format biom input

maxvonhippel · 2017-05-17T00:40:46Z

I used sample files from the tutorials to test with. With V100 .biom files, Q2Studio still gives an error message if you do not supply a source format parameter, like this:

However, you now have the ability to supply the source format, allowing non V210 .biom files to be processed, like this:

... and the artifact is successfully added to the table:

And V210 .biom files still work as they did before, with or without the optional source format parameter.

Adding a V210 .biom with the source format:

And then adding one without supplying the source format, thus demonstrating that it is optional:

maxvonhippel · 2017-05-17T00:49:21Z

Also, the plugins appear to work with the V100 demo file. I just tried rarefying it, with an arbitrary depth value, as follows:

And it looks like it worked:

I don't think anything is broken with the existing V210 support functionality, and this demo suggests that other source formats should now be equally supported. I'm happy to perform additional testing if there are any specific features/plugins/demo files/etc. you would like checked 👍 .

ebolyen

Functionality works, but the API is exposing more than it really needs to and it doesn't exactly work. Just remove a couple things and this is good to go.

ebolyen · 2017-05-17T18:53:13Z

app/js/containers/ArtifactGenerator.js

@@ -24,7 +24,7 @@ const mapDispatchToProps = dispatch => ({
        const fd = new FormData(e.target);
        const data = {};
        for (const [key, value] of fd) {
-            if (value === '' || value === undefined) {
+            if (key !== 'source_format' && (value === '' || value === undefined)) {


Good enough hack for now 👍

So it was important that the other values besides source_format be passed as '' if they are not entered in order to trigger @fail_elegantly in the backend python code, so I ended up modifying this part of the code to first check if the key is source_format and the value is '', in which case we set the value to null, and then to check if that is not the case if value is undefined etc, and then finally to set the value as was done before. This should make the code more readable here (and maybe a bit less hack-y looking) and also fix the problem you pointed out in workspace.py with source_format not being null by default.

Let me know if you don't like this style and I am happy to change it. Thanks!

ebolyen · 2017-05-17T18:55:08Z

q2studio/api/workspace.py

    return {
        'name': name,
        'uuid': metadata.uuid,
        'type': metadata.type,
-        'uri': url_for(route, uuid=metadata.uuid)
+        'uri': url_for(route, uuid=metadata.uuid),
+        'source_format': source_format


There's nothing to connect this to on the UI, so I would drop source_format (also it should be metadata.format anyhow because the metadata already has that info).

Makes sense, will do!

ebolyen · 2017-05-17T18:56:10Z

q2studio/api/workspace.py

@@ -75,8 +76,12 @@ def get_artifacts():
 @fail_gracefully
 def create_artifact():
    request_body = request.get_json()
+    _format = None
+    if request_body['source_format']:
+        _format = request_body['source_format']


This code makes the payload seem ambiguous. We should be able to rely on the source_format key to exist as null (and thus will be converted to None) so you shouldn't really need this block.

Ok. I'm not completely sure that we are able to rely on it to be null, but I agree that we should be able to. I'll check if it works as it should already and if it doesn't I'll follow the code back and figure out what's happening.

ebolyen · 2017-05-17T19:06:28Z

q2studio/api/workspace.py

-    return jsonify({'uuid': metadata.uuid, 'type': metadata.type})
+    return jsonify({'uuid': metadata.uuid,
+                    'type': metadata.type,
+                    'source_format': metadata.source_format})


Should be metadata.format, but again there's nothing on the client-side to draw this information, so it doesn't matter much. I would remove.

ebolyen · 2017-05-17T19:06:34Z

q2studio/api/workspace.py

-    return jsonify({'uuid': metadata.uuid, 'type': metadata.type})
+    return jsonify({'uuid': metadata.uuid,
+                    'type': metadata.type,
+                    'source_format': metadata.source_format})


ebolyen · 2017-05-17T19:06:47Z

q2studio/api/workspace.py

@@ -9,7 +9,7 @@
 import os
 import glob

-from flask import Blueprint, jsonify, request, abort, url_for\
+from flask import Blueprint, jsonify, request, abort, url_for


maxvonhippel · 2017-05-17T20:55:46Z

Thanks!

maxvonhippel added 6 commits May 16, 2017 12:48

got UI hooked up

a044434

made the source_format optional in the frontend

8128c64

testing source_format feature

170128e

first attempt at implementing REST API with source_format-enabled art…

d5bbabc

…ifacts

tested, appears to be working

13d1f5e

fix tests removing unnecesarry and actually not even useful test of s…

f99790e

…ource format, and tested on my machine and found that behaviour is as expected both with and without supplied format for BIOMV100Format biom input

ebolyen suggested changes May 17, 2017

View reviewed changes

responded to code review

a39ae41

ebolyen approved these changes May 17, 2017

View reviewed changes

ebolyen merged commit 945d2fb into qiime2:master May 17, 2017

maxvonhippel deleted the source-format-selection branch May 17, 2017 20:55

jairideout mentioned this pull request May 17, 2017

ENH: Specify source format during importing #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source format selection #111

Source format selection #111

maxvonhippel commented May 16, 2017 •

edited

Loading

maxvonhippel commented May 17, 2017

maxvonhippel commented May 17, 2017 •

edited

Loading

ebolyen left a comment

ebolyen May 17, 2017

maxvonhippel May 17, 2017

ebolyen May 17, 2017

maxvonhippel May 17, 2017

ebolyen May 17, 2017

maxvonhippel May 17, 2017

ebolyen May 17, 2017

ebolyen May 17, 2017

ebolyen May 17, 2017

maxvonhippel commented May 17, 2017

Source format selection #111

Source format selection #111

Conversation

maxvonhippel commented May 16, 2017 • edited Loading

maxvonhippel commented May 17, 2017

maxvonhippel commented May 17, 2017 • edited Loading

ebolyen 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

maxvonhippel commented May 17, 2017

maxvonhippel commented May 16, 2017 •

edited

Loading

maxvonhippel commented May 17, 2017 •

edited

Loading