TASK-1229 - UPA handler API #1199

briehl · 2017-10-31T23:57:55Z

This is mainly to prototype a solution for some parts of the copy/sharing problems. This first step will introduce an API for dealing with UPAs in the narrative. I think the following steps should be carefully done and tested before merging:

Add an UPA API in Javascript
Test it
Add an UPA API in Python
Test it

… Narrative cells.

coveralls · 2017-11-01T00:04:31Z

Coverage decreased (-0.006%) to 12.212% when pulling fd43c9a on briehl:copy-sharing-problem into f6f57e3 on kbase:develop.

coveralls · 2017-11-02T23:06:06Z

Coverage decreased (-0.001%) to 12.216% when pulling 2cc03da on briehl:copy-sharing-problem into f6f57e3 on kbase:develop.

coveralls · 2017-11-03T00:27:10Z

Coverage decreased (-0.001%) to 12.216% when pulling 58db396 on briehl:copy-sharing-problem into f6f57e3 on kbase:develop.

coveralls · 2017-11-03T21:20:15Z

Changes Unknown when pulling e5557ff on briehl:copy-sharing-problem into ** on kbase:develop**.

coveralls · 2017-11-04T00:21:11Z

Coverage increased (+0.04%) to 12.254% when pulling d9dec65 on briehl:copy-sharing-problem into ca1a111 on kbase:develop.

… redundant.

coveralls · 2017-11-07T01:59:36Z

Coverage increased (+0.08%) to 12.292% when pulling 99007c5 on briehl:copy-sharing-problem into a6b9c0b on kbase:develop.

MrCreosote · 2017-11-07T18:30:43Z

Caveats: I'm not qualified to review anything except the python changes.

MrCreosote · 2017-11-07T18:37:24Z

src/biokbase/narrative/tests/test_upa_api.py

+            "1/2/a",
+            "123/456/7/",
+            "1/2/3;",
+            "x/y/z"


Try some upas with ws & obj names, or just names alone, or just numbers alone, with and without refpaths.

MrCreosote · 2017-11-07T18:37:41Z

src/biokbase/narrative/tests/test_upa_api.py

+
+        self.bad_serials = list()
+        for bad_upa in self.bad_upas:
+            self.bad_serials.append(external_tag + bad_upa)


What about bracket serializations?

MrCreosote · 2017-11-07T18:41:06Z

src/biokbase/narrative/upa.py

+"""
+
+import re
+from app_util import (system_variable)


No need for parens

MrCreosote · 2017-11-07T18:44:45Z

src/biokbase/narrative/upa.py

+    if upa.startswith(ws_id):
+        return upa.replace(ws_id, "[{}]".format(ws_id), 1)
+    else:
+        return external_tag + upa


One thing that's a little worrisome is that this makes it easy for bugs to creep in by devs not being careful about their inputs. I'd almost want to split this into two methods - serialize_upa and serialize_external_upa to ensure the devs know exactly what they're doing when then serialize something.

We talked about this a little bit, so just to record it here - the intent of this API is that it should be used by wrapping widgets and under-the-covers processes. Devs who write viewer widgets or apps will probably never interact with it.

But, you're right, we should be explicit. I'll make the change to split it.

MrCreosote · 2017-11-07T18:46:42Z

src/biokbase/narrative/tests/test_upa_api.py

+    @mock.patch('biokbase.narrative.upa.system_variable', mock_sys_var)
+    def test_serialize_bad(self):
+        for bad_upa in self.bad_upas:
+            with self.assertRaises(ValueError):


Should check the error text is accurate

MrCreosote · 2017-11-07T18:47:14Z

src/biokbase/narrative/tests/test_upa_api.py

+        ]
+        for t in bad_types:
+            with self.assertRaises(ValueError):
+                deserialize(t)


MrCreosote · 2017-11-07T18:47:56Z

src/biokbase/narrative/tests/test_upa_api.py

+            del os.environ['KB_WORKSPACE_ID']
+        with self.assertRaises(RuntimeError):
+            serialize("1/2/3")
+        if tmp is not None:


This should be in a finally block so that the env is always restored

MrCreosote · 2017-11-07T18:48:42Z

src/biokbase/narrative/tests/test_upa_api.py

+            del os.environ['KB_WORKSPACE_ID']
+        with self.assertRaises(RuntimeError):
+            deserialize("[1]/2/3")
+        if tmp is not None:


MrCreosote · 2017-11-07T18:48:58Z

src/biokbase/narrative/tests/test_upa_api.py

+        if 'KB_WORKSPACE_ID' in os.environ:
+            tmp = os.environ.get('KB_WORKSPACE_ID')
+            del os.environ['KB_WORKSPACE_ID']
+        with self.assertRaises(RuntimeError):


Check error text

MrCreosote · 2017-11-07T18:49:03Z

src/biokbase/narrative/tests/test_upa_api.py

+        if 'KB_WORKSPACE_ID' in os.environ:
+            tmp = os.environ.get('KB_WORKSPACE_ID')
+            del os.environ['KB_WORKSPACE_ID']
+        with self.assertRaises(RuntimeError):


coveralls · 2017-11-07T21:03:02Z

Coverage increased (+0.08%) to 12.292% when pulling b0bcb0c on briehl:copy-sharing-problem into a6b9c0b on kbase:develop.

coveralls · 2017-11-07T22:02:07Z

Coverage increased (+0.08%) to 12.292% when pulling 5641eff on briehl:copy-sharing-problem into a6b9c0b on kbase:develop.

coveralls · 2017-11-07T23:24:45Z

Coverage increased (+0.1%) to 12.309% when pulling 1d7f815 on briehl:copy-sharing-problem into a6b9c0b on kbase:develop.

coveralls · 2017-11-08T21:44:33Z

Coverage increased (+0.1%) to 12.309% when pulling 99a3552 on briehl:copy-sharing-problem into a6b9c0b on kbase:develop.

sychan · 2017-11-09T00:48:24Z

@briehl Looks like the kbase-extension/static/kbase/js/api/fileStaging.js file has changed, perhaps due to the new staging service? Otherwise I think its fine

briehl · 2017-11-09T00:50:54Z

Yeah, that file got deleted and replaced with a different one. I'll catch this up with the develop branch and merge.

coveralls · 2017-11-09T00:58:29Z

Coverage increased (+0.1%) to 12.311% when pulling a959379 on briehl:copy-sharing-problem into f6ec7b4 on kbase:develop.

Add a JavaScript API for handling UPAs, mainly for serialization into…

fd43c9a

… Narrative cells.

briehl added 5 commits November 2, 2017 13:57

fix api tests

2c4b898

tweak travis.yml

8facc59

mess with travis caching

6260884

try changing ports!

16183e2

try a different jupyter startup format

2cc03da

tweak test reporter, fix file staging tests

58db396

Merge branch 'develop' into copy-sharing-problem

e5557ff

briehl changed the title ~~[WIP] First steps for copy sharing problem~~ TASK-1229 - UPA handler API Nov 3, 2017

some cleanup, remove get_object_info from upa api - out of scope

d9dec65

briehl added 4 commits November 6, 2017 15:15

update UPA API based on conversations

98a7343

rename api/upaApi -> api/upa. Because we don't need to be redundantly…

89db00b

… redundant.

add Python version of UPA api

0579b4f

Merge branch 'develop' into copy-sharing-problem

99007c5

briehl requested a review from MrCreosote November 7, 2017 01:58

MrCreosote requested changes Nov 7, 2017

View reviewed changes

update UPA api tests

b0bcb0c

add serialize_external function

5641eff

fix up the JS UPA api to match the python

1d7f815

cleanup tests a touch

99a3552

Merge branch 'develop' into copy-sharing-problem

a959379

briehl merged commit 6b4a466 into kbase:develop Nov 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK-1229 - UPA handler API #1199

TASK-1229 - UPA handler API #1199

briehl commented Oct 31, 2017 •

edited

coveralls commented Nov 1, 2017

coveralls commented Nov 2, 2017

coveralls commented Nov 3, 2017

coveralls commented Nov 3, 2017

coveralls commented Nov 4, 2017

coveralls commented Nov 7, 2017

MrCreosote commented Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

MrCreosote Nov 7, 2017

briehl Nov 7, 2017

coveralls commented Nov 7, 2017

coveralls commented Nov 7, 2017

coveralls commented Nov 7, 2017

coveralls commented Nov 8, 2017

sychan commented Nov 9, 2017

briehl commented Nov 9, 2017

coveralls commented Nov 9, 2017

TASK-1229 - UPA handler API #1199

TASK-1229 - UPA handler API #1199

Conversation

briehl commented Oct 31, 2017 • edited

coveralls commented Nov 1, 2017

coveralls commented Nov 2, 2017

coveralls commented Nov 3, 2017

coveralls commented Nov 3, 2017

coveralls commented Nov 4, 2017

coveralls commented Nov 7, 2017

MrCreosote commented Nov 7, 2017

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

coveralls commented Nov 7, 2017

coveralls commented Nov 7, 2017

coveralls commented Nov 7, 2017

coveralls commented Nov 8, 2017

sychan commented Nov 9, 2017

briehl commented Nov 9, 2017

coveralls commented Nov 9, 2017

briehl commented Oct 31, 2017 •

edited