From 31d0f8f9966f424c58a8acfa471bfbef211a95dc Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 31 Jul 2023 12:55:40 -0400 Subject: [PATCH 01/12] first step on fixing param mapping --- src/biokbase/narrative/app_util.py | 153 +++++++++++++++++++++++------ 1 file changed, 124 insertions(+), 29 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 5c95a31f8e..4bdd89ca54 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -1,9 +1,12 @@ import json import re +from typing import Optional import biokbase.narrative.clients as clients from biokbase.narrative.system import system_variable +import biokbase.narrative.upa as upa + """ Some utility functions for running KBase Apps or Methods or whatever they are this week. """ @@ -727,7 +730,7 @@ def resolve_ref_if_typed(value, spec_param): return value -def transform_param_value(transform_type, value, spec_param): +def transform_param_value(transform_type: Optional[str], value: Any, spec_param: Optional[dict]) -> Any: """ Transforms an input according to the rules given in NarrativeMethodStore.ServiceMethodInputMapping @@ -738,7 +741,59 @@ def transform_param_value(transform_type, value, spec_param): (4.) none or None - doesn't transform. Returns a transformed (or not) value. + + New rules and logic, esp for objects being sent. + 1. Test if current transform applies. (None is a valid transform) + A. Check if input is an object - valid transforms are ref, resolved-ref, list, list, None + B. If not object, int, list, and None are allowed. + 2. If object and not output field, apply transform as follows: + A. None -> returns only object name + B. ref -> returns workspace_name/object_name + C. resolved-ref -> returns UPA + D. (not in docs yet) upa -> returns UPA + E. any of the above can be applied in list + + This function will attempt to transform value according to the above rules. If value looks like a ref (ws_name/obj_name) + and transform_type is None, then obj_name will be returned. Likewise, if resolved-ref is given, and value looks like + an UPA already, then the already resolved-ref will be returned. + + Previously, if the param type was an input object, it was assumed that the value was the object name. + That, honestly, shouldn't have been the case. + + There are also a few never-used transform types that aren't documented. This code comes from the Before Times, + before any kind of documentation and modern design strategies, so I guess that's expected. + + Parameters: + transform_type - str/None - should be one of the following, if not None: + * string + * int + * ref + * resolved-ref + * list where X is any of the above + value - anything or None. Parameter values are expected, by the KBase app stack, to generally be + either a singleton or a list of singletons. In practice, they're usually strings, ints, floats, None, + or a list of those. + spec_param - either None or a spec parameter dictionary as defined by SpecManager.app_params. That is: + { + optional = boolean, + is_constant = boolean, + value = (whatever, optional), + type = [text|int|float|list|textsubdata], + is_output = boolean, + short_hint = string, + description = string, + allowed_values = list (optional), + } """ + is_input_object = False + if ( + spec_param is not None + and spec_param["type"] == "text" + and not spec_param["is_output"] + and len(spec_param["allowed_values"]) + ): + is_input_object = True + if ( transform_type is None and spec_param is not None @@ -749,38 +804,39 @@ def transform_param_value(transform_type, value, spec_param): if ( transform_type is None or transform_type == "none" - or transform_type == "object-name" ): return value - elif transform_type == "ref" or transform_type == "unresolved-ref": - # make unresolved workspace ref (like 'ws-name/obj-name') - if (value is not None) and ("/" not in value): - value = system_variable("workspace") + "/" + value - return value - - elif transform_type == "resolved-ref": - # make a workspace ref - if value is not None: - value = resolve_ref(system_variable("workspace"), value) - return value - - elif transform_type == "future-default": - # let's guess base on spec_param - if spec_param is None: - return value - else: - if value is not None: - value = resolve_ref_if_typed(value, spec_param) - return value + transform_type = transform_type.lower() + + # Here's the issue. + # We're getting values that can be either a string, a ref, or an UPA. + # Now, when we get transform_type = "ref"/"unresolved-ref", that + # should be transformed to a ws/obj ref + # if transform_type == "ref" or transform_type == "unresolved-ref": + # # make unresolved workspace ref (like 'ws-name/obj-name') + # if value is not None and "/" not in value: + # value = system_variable("workspace") + "/" + value + # return value + + # if transform_type == "resolved-ref": + # # make a workspace UPA + # if value is not None: + # value = resolve_ref(system_variable("workspace"), value) + # return value + + if transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] or is_input_object: + if isinstance(value, list): + return [transform_object_value(transform_type, v) for v in value] + return transform_object_value(transform_type, value) - elif transform_type == "int": + if transform_type == "int": # make it an integer, OR 0. if value is None or len(str(value).strip()) == 0: return None return int(value) - elif transform_type == "string": + if transform_type == "string": if value is None: return value elif isinstance(value, list): @@ -790,16 +846,55 @@ def transform_param_value(transform_type, value, spec_param): else: return str(value) - elif transform_type.startswith("list<") and transform_type.endswith(">"): + if transform_type.startswith("list<") and transform_type.endswith(">"): # make it a list of transformed types. list_type = transform_type[5:-1] if isinstance(value, list): - ret = [] - for pos in range(0, len(value)): - ret.append(transform_param_value(list_type, value[pos], None)) - return ret + return [transform_param_value(list_type, v, None) for v in value] else: return [transform_param_value(list_type, value, None)] else: raise ValueError("Unsupported Transformation type: " + transform_type) + +def transform_object_value(transform_type: Optional[str], value: Optional[str]) -> str: + """ + Cases: + transform = ref or unresolved-ref: + - should return wsname / object name + transform = upa or resolved-ref: + - should return UPA + transform = None: + - should return object name + + value can be either object name, ref, upa, or ref-path + can tell by testing with UPA api + + If we can't find any object info on the value, just return the value as-is + """ + + if value is None: + return None + + # 1. get object info + is_upa = upa.is_upa(value) + is_ref = upa.is_ref(value) + search_ref = value + if not is_upa and not is_ref: + search_ref = f"{system_variable('workspace')/{value}}" + try: + obj_info = clients.get("workspace").get_object_info3({"objects": [{"ref": search_ref}]}) + except Exception as e: + # TODO logging + return value + + if transform_type == "ref" or transform_type == "unresolved_ref": + if is_ref: + return value + obj = obj_info["infos"][0] + return f"{obj[7]}/{obj[1]}" + if transform_type == "resolved-ref" or transform_type == "upa": + if is_upa: + return value + return obj_info["paths"][0] + return obj_info[1] From 8296154840a91e9d330a77629be939e31f62fd4b Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 31 Jul 2023 13:51:11 -0400 Subject: [PATCH 02/12] add translation fix for PTV-1810 backend --- src/biokbase/narrative/app_util.py | 15 ++++++++------- src/biokbase/narrative/tests/test_app_util.py | 6 ++++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 4bdd89ca54..44100ee282 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -1,6 +1,6 @@ import json import re -from typing import Optional +from typing import Any, Optional import biokbase.narrative.clients as clients from biokbase.narrative.system import system_variable @@ -881,14 +881,15 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) is_ref = upa.is_ref(value) search_ref = value if not is_upa and not is_ref: - search_ref = f"{system_variable('workspace')/{value}}" + search_ref = f"{system_variable('workspace')}/{value}" try: obj_info = clients.get("workspace").get_object_info3({"objects": [{"ref": search_ref}]}) - except Exception as e: - # TODO logging + except Exception as err: + print(err) + print(f"Error while searching for object ref '{search_ref}'") return value - if transform_type == "ref" or transform_type == "unresolved_ref": + if transform_type == "ref" or transform_type == "unresolved-ref": if is_ref: return value obj = obj_info["infos"][0] @@ -896,5 +897,5 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) if transform_type == "resolved-ref" or transform_type == "upa": if is_upa: return value - return obj_info["paths"][0] - return obj_info[1] + return ";".join(obj_info["paths"][0]) + return value diff --git a/src/biokbase/narrative/tests/test_app_util.py b/src/biokbase/narrative/tests/test_app_util.py index d039741356..a1781c98ef 100644 --- a/src/biokbase/narrative/tests/test_app_util.py +++ b/src/biokbase/narrative/tests/test_app_util.py @@ -392,13 +392,15 @@ def test_transform_param_value_textsubdata(value, expected): (None, None), ("foo/bar", "foo/bar"), ("1/2/3", "1/2/3"), - ("name", workspace + "/name") + ("Sbicolor2", "wjriehl:1490995018528/Sbicolor2") ] +@mock.patch("biokbase.narrative.app_util.clients.get", get_mock_client) @pytest.mark.parametrize("value,expected", ref_cases) def test_transform_param_value_simple_ref(value, expected, workspace_name): workspace_name(workspace) for tf_type in ["ref", "unresolved-ref"]: - assert transform_param_value(tf_type, value, None) == expected + tf_value = transform_param_value(tf_type, value, None) + assert tf_value == expected mock_upa = "18836/5/1" upa_cases = [ From 162b393d101be31748d13563311aa7937f436da7 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 31 Jul 2023 13:52:48 -0400 Subject: [PATCH 03/12] run flake8/black --- src/biokbase/narrative/app_util.py | 21 +- src/biokbase/narrative/tests/test_app_util.py | 328 ++++++++---------- 2 files changed, 150 insertions(+), 199 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 44100ee282..25554ba0dd 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -13,6 +13,7 @@ app_version_tags = ["release", "beta", "dev"] + def check_tag(tag, raise_exception=False): """ Checks if the given tag is one of "release", "beta", or "dev". @@ -28,7 +29,6 @@ def check_tag(tag, raise_exception=False): return tag_exists - def map_inputs_from_job(job_inputs, app_spec): """ Unmaps the actual list of job inputs back to the @@ -730,7 +730,9 @@ def resolve_ref_if_typed(value, spec_param): return value -def transform_param_value(transform_type: Optional[str], value: Any, spec_param: Optional[dict]) -> Any: +def transform_param_value( + transform_type: Optional[str], value: Any, spec_param: Optional[dict] +) -> Any: """ Transforms an input according to the rules given in NarrativeMethodStore.ServiceMethodInputMapping @@ -801,10 +803,7 @@ def transform_param_value(transform_type: Optional[str], value: Any, spec_param: ): transform_type = "string" - if ( - transform_type is None - or transform_type == "none" - ): + if transform_type is None or transform_type == "none": return value transform_type = transform_type.lower() @@ -825,7 +824,10 @@ def transform_param_value(transform_type: Optional[str], value: Any, spec_param: # value = resolve_ref(system_variable("workspace"), value) # return value - if transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] or is_input_object: + if ( + transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] + or is_input_object + ): if isinstance(value, list): return [transform_object_value(transform_type, v) for v in value] return transform_object_value(transform_type, value) @@ -857,6 +859,7 @@ def transform_param_value(transform_type: Optional[str], value: Any, spec_param: else: raise ValueError("Unsupported Transformation type: " + transform_type) + def transform_object_value(transform_type: Optional[str], value: Optional[str]) -> str: """ Cases: @@ -883,7 +886,9 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) if not is_upa and not is_ref: search_ref = f"{system_variable('workspace')}/{value}" try: - obj_info = clients.get("workspace").get_object_info3({"objects": [{"ref": search_ref}]}) + obj_info = clients.get("workspace").get_object_info3( + {"objects": [{"ref": search_ref}]} + ) except Exception as err: print(err) print(f"Error while searching for object ref '{search_ref}'") diff --git a/src/biokbase/narrative/tests/test_app_util.py b/src/biokbase/narrative/tests/test_app_util.py index a1781c98ef..9cca911938 100644 --- a/src/biokbase/narrative/tests/test_app_util.py +++ b/src/biokbase/narrative/tests/test_app_util.py @@ -12,7 +12,7 @@ get_result_sub_path, map_inputs_from_job, map_outputs_from_state, - transform_param_value + transform_param_value, ) from . import util @@ -21,7 +21,9 @@ user_token = "SOME_TOKEN" config = util.ConfigTests() user_id = config.get("users", "test_user") -user_token = util.read_token_file(config.get_path("token_files", "test_user", from_root=True)) +user_token = util.read_token_file( + config.get_path("token_files", "test_user", from_root=True) +) good_tag = "release" bad_tag = "not_a_tag" @@ -30,59 +32,48 @@ workspace = "valid_workspace" bad_variable = "not a variable" + def test_check_tag_good(): assert check_tag(good_tag) is True + def test_check_tag_bad(): assert check_tag(bad_tag) is False + def test_check_tag_bad_except(): with pytest.raises(ValueError): check_tag(bad_tag, raise_exception=True) + @pytest.fixture() def workspace_name(): def set_ws_name(ws_name): os.environ["KB_WORKSPACE_ID"] = ws_name + yield set_ws_name del os.environ["KB_WORKSPACE_ID"] + get_result_sub_path_cases = [ ( [{"report": "this_is_a_report", "report_ref": "123/456/7"}], [0, "report_ref"], - "123/456/7" - ), - ( - ["foo", "bar", "baz"], - [2], - "baz" + "123/456/7", ), - ( - ["foo", {"bar": "baz"}, "foobar"], - [1, "bar"], - "baz" - ), - ( - ["foo", 0, {"bar": {"baz": [10, 11, 12, 13]}}], - [2, "bar", "baz", 3], - 13 - ), - ( - ["foo"], - [2], - None - ), - ( - {"foo": "bar"}, - ["baz"], - None - ) + (["foo", "bar", "baz"], [2], "baz"), + (["foo", {"bar": "baz"}, "foobar"], [1, "bar"], "baz"), + (["foo", 0, {"bar": {"baz": [10, 11, 12, 13]}}], [2, "bar", "baz", 3], 13), + (["foo"], [2], None), + ({"foo": "bar"}, ["baz"], None), ] + + @pytest.mark.parametrize("result,path,expected", get_result_sub_path_cases) def test_get_result_sub_path(result, path, expected): assert get_result_sub_path(result, path) == expected + def test_map_inputs_from_job(): inputs = [ "input1", @@ -138,13 +129,12 @@ def test_map_outputs_from_state_simple(workspace_name): workspace_name(workspace) app_spec = { "parameters": [], - "behavior": { - "output_mapping": [{"narrative_system_variable": "workspace"}] - }, + "behavior": {"output_mapping": [{"narrative_system_variable": "workspace"}]}, } expected = ("kbaseDefaultNarrativeOutput", workspace) assert map_outputs_from_state(None, None, app_spec) == expected + def test_map_outputs_from_state(workspace_name): workspace_name(workspace) app_spec = { @@ -175,6 +165,7 @@ def test_map_outputs_from_state(workspace_name): ) assert map_outputs_from_state(state, params, app_spec) == expected + def test_map_outputs_from_state_bad_spec(workspace_name): workspace_name(workspace) app_spec = {"not": "really"} @@ -184,17 +175,16 @@ def test_map_outputs_from_state_bad_spec(workspace_name): map_outputs_from_state(state, params, app_spec) - # app_param tests base_app_param = { - 'id': 'my_param', - 'ui_name': 'My Param', - 'short_hint': 'Short Info', - 'description': 'Longer description', - 'allow_multiple': 0, - 'optional': 0, - 'ui_class': 'input', - 'default_values': [''], + "id": "my_param", + "ui_name": "My Param", + "short_hint": "Short Info", + "description": "Longer description", + "allow_multiple": 0, + "optional": 0, + "ui_class": "input", + "default_values": [""], } base_expect = { "id": base_app_param["id"], @@ -206,136 +196,80 @@ def test_map_outputs_from_state_bad_spec(workspace_name): "allow_multiple": False, "default": None, } -app_param_cases = [( - "text", {}, {} -), ( - "text", - { - "text_options": { - "is_output_name": 1 - } - }, - { - "is_output": True - } -), ( - "text", - { - "optional": 1, - }, - { - "optional": True - } -), ( - "text", - { - "allow_multiple": 1, - "default_values": ["foo", "", "bar"] - }, - { - "allow_multiple": True, - "default": ["foo", "bar"] - } -), ( - "text", - { - "text_options": { - "validate_as": "int", - "min_int": -1, - "max_int": 1000 - } - }, - { - "type": "int", - "min_val": -1, - "max_val": 1000 - } -), ( - "text", - { - "text_options": { - "validate_as": "float", - "min_float": -1.2, - "max_float": 1000.4 - } - }, - { - "type": "float", - "min_val": -1.2, - "max_val": 1000.4 - } -), ( - "dropdown", - { - "dropdown_options": { - "options": [ - {"display": "a", "value": "b"}, - {"display": "c", "value": "d"} - ] - } - }, - { - "allowed_values": ["b", "d"] - } -), ( - "checkbox", - { - "checkbox_options": { - "checked_value": 1, - "unchecked_value": 0 - } - }, - { - "checkbox_map": [1, 0], - "allowed_values": [True, False] - } -), ( - "checkbox", - {}, - { - "allowed_values": [True, False] - } -), ( - "checkbox", - { - "checkbox_options": { - "checked_value": 2, - "unchecked_value": 1, - "kinda_checked_value": 0 - } - }, - { - "allowed_values": [True, False] - } -), ( - "text", - { - "text_options": { - "valid_ws_types": [] - } - }, - {} -), ( - "text", - { - "text_options": { - "valid_ws_types": ["KBaseGenomes.Genome", "FooBar.Baz"] - } - }, - { - "allowed_types": ["KBaseGenomes.Genome", "FooBar.Baz"] - } -), ( - "text", - { - "text_options": { - "regex_constraint": "/\\d+/" - } - }, - { - "regex_constraint": "/\\d+/" - } -)] +app_param_cases = [ + ("text", {}, {}), + ("text", {"text_options": {"is_output_name": 1}}, {"is_output": True}), + ( + "text", + { + "optional": 1, + }, + {"optional": True}, + ), + ( + "text", + {"allow_multiple": 1, "default_values": ["foo", "", "bar"]}, + {"allow_multiple": True, "default": ["foo", "bar"]}, + ), + ( + "text", + {"text_options": {"validate_as": "int", "min_int": -1, "max_int": 1000}}, + {"type": "int", "min_val": -1, "max_val": 1000}, + ), + ( + "text", + { + "text_options": { + "validate_as": "float", + "min_float": -1.2, + "max_float": 1000.4, + } + }, + {"type": "float", "min_val": -1.2, "max_val": 1000.4}, + ), + ( + "dropdown", + { + "dropdown_options": { + "options": [ + {"display": "a", "value": "b"}, + {"display": "c", "value": "d"}, + ] + } + }, + {"allowed_values": ["b", "d"]}, + ), + ( + "checkbox", + {"checkbox_options": {"checked_value": 1, "unchecked_value": 0}}, + {"checkbox_map": [1, 0], "allowed_values": [True, False]}, + ), + ("checkbox", {}, {"allowed_values": [True, False]}), + ( + "checkbox", + { + "checkbox_options": { + "checked_value": 2, + "unchecked_value": 1, + "kinda_checked_value": 0, + } + }, + {"allowed_values": [True, False]}, + ), + ("text", {"text_options": {"valid_ws_types": []}}, {}), + ( + "text", + {"text_options": {"valid_ws_types": ["KBaseGenomes.Genome", "FooBar.Baz"]}}, + {"allowed_types": ["KBaseGenomes.Genome", "FooBar.Baz"]}, + ), + ( + "text", + {"text_options": {"regex_constraint": "/\\d+/"}}, + {"regex_constraint": "/\\d+/"}, + ), +] + + @pytest.mark.parametrize("field_type,spec_add,expect_add", app_param_cases) def test_app_param(field_type, spec_add, expect_add): spec_param = copy.deepcopy(base_app_param) @@ -350,50 +284,59 @@ def test_app_param(field_type, spec_add, expect_add): # transform_param_value tests transform_param_value_simple_cases = [ - ( "string", None, None, None ), - ( "string", "foo", None, "foo" ), - ( "string", 123, None, "123" ), - ( "string", ["a", "b", "c"], None, "a,b,c"), - ( "string", {"a": "b", "c": "d"}, None, "a=b,c=d"), - ( "string", [], None, "" ), - ( "string", {}, None, "" ), - ( "int", "1", None, 1 ), - ( "int", None, None, None ), - ( "int", "", None, None ), - ( "list", [1, 2, 3], None, ["1", "2", "3"] ), - ( "list", ["1", "2", "3"], None, [1, 2, 3] ), - ( "list", "asdf", None, ["asdf"] ), - ( "list", "1", None, [1] ) + ("string", None, None, None), + ("string", "foo", None, "foo"), + ("string", 123, None, "123"), + ("string", ["a", "b", "c"], None, "a,b,c"), + ("string", {"a": "b", "c": "d"}, None, "a=b,c=d"), + ("string", [], None, ""), + ("string", {}, None, ""), + ("int", "1", None, 1), + ("int", None, None, None), + ("int", "", None, None), + ("list", [1, 2, 3], None, ["1", "2", "3"]), + ("list", ["1", "2", "3"], None, [1, 2, 3]), + ("list", "asdf", None, ["asdf"]), + ("list", "1", None, [1]), ] -@pytest.mark.parametrize("transform_type,value,spec_param,expected", transform_param_value_simple_cases) + + +@pytest.mark.parametrize( + "transform_type,value,spec_param,expected", transform_param_value_simple_cases +) def test_transform_param_value_simple(transform_type, value, spec_param, expected): assert transform_param_value(transform_type, value, spec_param) == expected + def test_transform_param_value_fail(): ttype = "foobar" with pytest.raises(ValueError, match=f"Unsupported Transformation type: {ttype}"): transform_param_value(ttype, "foo", None) + textsubdata_cases = [ (None, None), ("asdf", "asdf"), (123, "123"), (["1", "2", "3"], "1,2,3"), - ({"a":"b", "c": "d"}, "a=b,c=d") + ({"a": "b", "c": "d"}, "a=b,c=d"), ] + + @pytest.mark.parametrize("value,expected", textsubdata_cases) def test_transform_param_value_textsubdata(value, expected): - spec = { - "type": "textsubdata" - } + spec = {"type": "textsubdata"} assert transform_param_value(None, value, spec) == expected + ref_cases = [ (None, None), ("foo/bar", "foo/bar"), ("1/2/3", "1/2/3"), - ("Sbicolor2", "wjriehl:1490995018528/Sbicolor2") + ("Sbicolor2", "wjriehl:1490995018528/Sbicolor2"), ] + + @mock.patch("biokbase.narrative.app_util.clients.get", get_mock_client) @pytest.mark.parametrize("value,expected", ref_cases) def test_transform_param_value_simple_ref(value, expected, workspace_name): @@ -402,12 +345,15 @@ def test_transform_param_value_simple_ref(value, expected, workspace_name): tf_value = transform_param_value(tf_type, value, None) assert tf_value == expected + mock_upa = "18836/5/1" upa_cases = [ ("some_name", mock_upa), (["multiple", "names"], [mock_upa, mock_upa]), - ("already/ref", mock_upa) + ("already/ref", mock_upa), ] + + @mock.patch("biokbase.narrative.app_util.clients.get", get_mock_client) @pytest.mark.parametrize("value,expected", upa_cases) def test_transform_param_value_resolved_ref(value, expected, workspace_name): From 7f5b420d42a45ba82daf1990b05dca0196fe20b2 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 31 Jul 2023 17:11:02 -0400 Subject: [PATCH 04/12] remove commented code --- src/biokbase/narrative/app_util.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 25554ba0dd..ed65a68549 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -818,12 +818,6 @@ def transform_param_value( # value = system_variable("workspace") + "/" + value # return value - # if transform_type == "resolved-ref": - # # make a workspace UPA - # if value is not None: - # value = resolve_ref(system_variable("workspace"), value) - # return value - if ( transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] or is_input_object From 989e5bcc81a370eff9b8be1cb24caad9a7dbce40 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Tue, 1 Aug 2023 21:28:28 -0400 Subject: [PATCH 05/12] flesh out features and logic for transform_param_value changes --- src/biokbase/narrative/app_util.py | 31 +++++++++---------- src/biokbase/narrative/tests/test_app_util.py | 4 +-- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index ed65a68549..73c2fca1a2 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -787,14 +787,17 @@ def transform_param_value( allowed_values = list (optional), } """ - is_input_object = False + if transform_type is not None: + transform_type = transform_type.lower() + + is_input_object_param = False if ( spec_param is not None and spec_param["type"] == "text" and not spec_param["is_output"] - and len(spec_param["allowed_values"]) + and len(spec_param.get("allowed_types", [])) ): - is_input_object = True + is_input_object_param = True if ( transform_type is None @@ -803,11 +806,9 @@ def transform_param_value( ): transform_type = "string" - if transform_type is None or transform_type == "none": + if not is_input_object_param and (transform_type is None or transform_type == "none"): return value - transform_type = transform_type.lower() - # Here's the issue. # We're getting values that can be either a string, a ref, or an UPA. # Now, when we get transform_type = "ref"/"unresolved-ref", that @@ -820,7 +821,7 @@ def transform_param_value( if ( transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] - or is_input_object + or (is_input_object_param and not transform_type.startswith("list")) ): if isinstance(value, list): return [transform_object_value(transform_type, v) for v in value] @@ -835,20 +836,18 @@ def transform_param_value( if transform_type == "string": if value is None: return value - elif isinstance(value, list): + if isinstance(value, list): return ",".join(value) - elif isinstance(value, dict): - return ",".join([key + "=" + str(value[key]) for key in value]) - else: - return str(value) + if isinstance(value, dict): + return ",".join([f"{key}={value[key]}" for key in value]) + return str(value) if transform_type.startswith("list<") and transform_type.endswith(">"): # make it a list of transformed types. list_type = transform_type[5:-1] if isinstance(value, list): return [transform_param_value(list_type, v, None) for v in value] - else: - return [transform_param_value(list_type, value, None)] + return [transform_param_value(list_type, value, None)] else: raise ValueError("Unsupported Transformation type: " + transform_type) @@ -889,12 +888,12 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) return value if transform_type == "ref" or transform_type == "unresolved-ref": - if is_ref: - return value obj = obj_info["infos"][0] return f"{obj[7]}/{obj[1]}" if transform_type == "resolved-ref" or transform_type == "upa": if is_upa: return value return ";".join(obj_info["paths"][0]) + if transform_type is None: + return obj_info["infos"][0][1] return value diff --git a/src/biokbase/narrative/tests/test_app_util.py b/src/biokbase/narrative/tests/test_app_util.py index 9cca911938..d9353f3133 100644 --- a/src/biokbase/narrative/tests/test_app_util.py +++ b/src/biokbase/narrative/tests/test_app_util.py @@ -331,8 +331,8 @@ def test_transform_param_value_textsubdata(value, expected): ref_cases = [ (None, None), - ("foo/bar", "foo/bar"), - ("1/2/3", "1/2/3"), + ("foo/bar", "wjriehl:1490995018528/Sbicolor2"), + ("1/2/3", "wjriehl:1490995018528/Sbicolor2"), ("Sbicolor2", "wjriehl:1490995018528/Sbicolor2"), ] From 77eb2bef6eb2277e087d0c6c7641404687ad688f Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Tue, 1 Aug 2023 21:37:05 -0400 Subject: [PATCH 06/12] fix another case --- src/biokbase/narrative/app_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 73c2fca1a2..852899786a 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -821,7 +821,7 @@ def transform_param_value( if ( transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] - or (is_input_object_param and not transform_type.startswith("list")) + or (is_input_object_param and transform_type is None) ): if isinstance(value, list): return [transform_object_value(transform_type, v) for v in value] From f62915695e1106eba433e236bc88f2d4670145e1 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 7 Aug 2023 17:05:40 -0400 Subject: [PATCH 07/12] make sure ref-paths update properly --- src/biokbase/narrative/app_util.py | 18 ++-- src/biokbase/narrative/tests/test_app_util.py | 84 ++++++++++++++++++- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 852899786a..b92e7a53b5 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -3,9 +3,8 @@ from typing import Any, Optional import biokbase.narrative.clients as clients -from biokbase.narrative.system import system_variable - import biokbase.narrative.upa as upa +from biokbase.narrative.system import system_variable """ Some utility functions for running KBase Apps or Methods or whatever they are this week. @@ -754,6 +753,7 @@ def transform_param_value( C. resolved-ref -> returns UPA D. (not in docs yet) upa -> returns UPA E. any of the above can be applied in list + 3. Exception: if the input is an UPA path or reference path, it should only get transformed to an UPA path. This function will attempt to transform value according to the above rules. If value looks like a ref (ws_name/obj_name) and transform_type is None, then obj_name will be returned. Likewise, if resolved-ref is given, and value looks like @@ -806,7 +806,9 @@ def transform_param_value( ): transform_type = "string" - if not is_input_object_param and (transform_type is None or transform_type == "none"): + if not is_input_object_param and ( + transform_type is None or transform_type == "none" + ): return value # Here's the issue. @@ -819,9 +821,8 @@ def transform_param_value( # value = system_variable("workspace") + "/" + value # return value - if ( - transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] - or (is_input_object_param and transform_type is None) + if transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] or ( + is_input_object_param and transform_type is None ): if isinstance(value, list): return [transform_object_value(transform_type, v) for v in value] @@ -868,13 +869,14 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) If we can't find any object info on the value, just return the value as-is """ - if value is None: return None # 1. get object info is_upa = upa.is_upa(value) is_ref = upa.is_ref(value) + is_path = (is_upa or is_ref) and ";" in value + search_ref = value if not is_upa and not is_ref: search_ref = f"{system_variable('workspace')}/{value}" @@ -887,6 +889,8 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) print(f"Error while searching for object ref '{search_ref}'") return value + if is_path: + return ";".join(obj_info["paths"][0]) if transform_type == "ref" or transform_type == "unresolved-ref": obj = obj_info["infos"][0] return f"{obj[7]}/{obj[1]}" diff --git a/src/biokbase/narrative/tests/test_app_util.py b/src/biokbase/narrative/tests/test_app_util.py index d9353f3133..cba066c383 100644 --- a/src/biokbase/narrative/tests/test_app_util.py +++ b/src/biokbase/narrative/tests/test_app_util.py @@ -3,9 +3,10 @@ """ import copy import os -import pytest from unittest import mock +import pytest + from biokbase.narrative.app_util import ( app_param, check_tag, @@ -14,6 +15,7 @@ map_outputs_from_state, transform_param_value, ) +from biokbase.narrative.upa import is_upa from . import util from .narrative_mock.mockclients import get_mock_client @@ -329,11 +331,12 @@ def test_transform_param_value_textsubdata(value, expected): assert transform_param_value(None, value, spec) == expected +mock_obj_ref = "wjriehl:1490995018528/Sbicolor2" ref_cases = [ (None, None), - ("foo/bar", "wjriehl:1490995018528/Sbicolor2"), - ("1/2/3", "wjriehl:1490995018528/Sbicolor2"), - ("Sbicolor2", "wjriehl:1490995018528/Sbicolor2"), + ("foo/bar", mock_obj_ref), + ("1/2/3", mock_obj_ref), + ("Sbicolor2", mock_obj_ref), ] @@ -359,3 +362,76 @@ def test_transform_param_value_simple_ref(value, expected, workspace_name): def test_transform_param_value_resolved_ref(value, expected, workspace_name): workspace_name(workspace) assert transform_param_value("resolved-ref", value, None) == expected + + +class RefChainWorkspace: + def __init__(self): + pass + + def get_object_info3(self, params): + """ + Makes quite a few assumptions about input, as it's used for a specific test. + 1. params = {"objects": [{"ref": ref}, {"ref": ref}, ...]} + 2. ref is a ;-separated chain of either wsname/objid or upas, not a mix. + 3. we don't really care about names or object info here in responses + + # TODO something like this should go into the MockClient workspace call + """ + + def make_obj_info(wsid, objid, ver): + return [ + objid, + "objname", + "Object.Type-1.0", + "date", + ver, + "user_name", + wsid, + "SomeWorkspace", + "checksum", + 12345, + None, + ] + + obj_infos = [] + obj_paths = [] + for obj_ident in params["objects"]: + ref_chain = obj_ident["ref"].split(";") + obj_path = [] + cur_ws = 1 + cur_obj = 1 + cur_ver = 1 + for ref in ref_chain: + if is_upa(ref): + obj_path.append(ref) + else: + obj_path.append(f"{cur_ws}/{cur_obj}/{cur_ver}") + cur_ws += 1 + cur_obj += 1 + cur_ver += 1 + if is_upa(obj_ident["ref"]): + obj_info = make_obj_info(*(ref_chain[-1].split("/"))) + else: + obj_info = make_obj_info(cur_ws, cur_obj, cur_ver) + obj_infos.append(obj_info) + obj_paths.append(obj_path) + return {"infos": obj_infos, "paths": obj_paths} + + +def get_ref_path_mock_ws(name="workspace"): + return RefChainWorkspace() + + +@mock.patch("biokbase.narrative.app_util.clients.get", get_ref_path_mock_ws) +def test_transform_param_value_upa_path(): + upa_path = "123/456/789;234/345/456" + for tf_type in ["ref", "unresolved-ref", "upa", "resolved-ref"]: + assert transform_param_value(tf_type, upa_path, None) == upa_path + + +@mock.patch("biokbase.narrative.app_util.clients.get", get_ref_path_mock_ws) +def test_transform_param_value_ref_path(): + ref_path = "some_ws/some_obj;some_other_ws/some_other_obj" + expected_upa = "1/1/1;2/2/2" + for tf_type in ["ref", "unresolved-ref", "upa", "resolved-ref"]: + assert transform_param_value(tf_type, ref_path, None) == expected_upa From dbc188ceedf9decc8208bdc146b7fb480dd410a9 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 7 Aug 2023 17:11:44 -0400 Subject: [PATCH 08/12] update release notes, comments --- RELEASE_NOTES.md | 2 +- src/biokbase/narrative/app_util.py | 43 ++++++++++++------------------ 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 3f2790a1b9..0c27e68184 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,7 +7,7 @@ This is built on the Jupyter Notebook v6.4.12 and IPython 8.5.0 (more notes will ## Unreleased - PTV-1875 - fix public data paging issue by removing paging from workspace data sources - UIP-28 - update Google Analytics tags to GA4 properties -- PTV-1810 - address object name display issues in the View Configure tab of app cells. +- PTV-1810 - address object name display issues in the View Configure tab of app cells. This now saves all app inputs as UPAs in the cell. It also includes an update to input transforms to properly convert from UPAs <-> names or references as appropriate before starting the app. - PTV-1877 - fix app descriptions to replace the documentation link for the upload / download guide - PTV-1878 - fix some failing front end unit tests diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index b92e7a53b5..c87f59c90e 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -743,9 +743,10 @@ def transform_param_value( Returns a transformed (or not) value. - New rules and logic, esp for objects being sent. + Rules and logic, esp for objects being sent. 1. Test if current transform applies. (None is a valid transform) - A. Check if input is an object - valid transforms are ref, resolved-ref, list, list, None + A. Check if input is an object - valid transforms are ref, resolved-ref, list, + list, None B. If not object, int, list, and None are allowed. 2. If object and not output field, apply transform as follows: A. None -> returns only object name @@ -753,17 +754,13 @@ def transform_param_value( C. resolved-ref -> returns UPA D. (not in docs yet) upa -> returns UPA E. any of the above can be applied in list - 3. Exception: if the input is an UPA path or reference path, it should only get transformed to an UPA path. + 3. Exception: if the input is an UPA path or reference path, it should only get transformed + to an UPA path. - This function will attempt to transform value according to the above rules. If value looks like a ref (ws_name/obj_name) - and transform_type is None, then obj_name will be returned. Likewise, if resolved-ref is given, and value looks like - an UPA already, then the already resolved-ref will be returned. - - Previously, if the param type was an input object, it was assumed that the value was the object name. - That, honestly, shouldn't have been the case. - - There are also a few never-used transform types that aren't documented. This code comes from the Before Times, - before any kind of documentation and modern design strategies, so I guess that's expected. + This function will attempt to transform value according to the above rules. If value looks + like a ref (ws_name/obj_name) and transform_type is None, then obj_name will be returned. + Likewise, if resolved-ref is given, and value looks like an UPA already, then the already + resolved-ref will be returned. Parameters: transform_type - str/None - should be one of the following, if not None: @@ -771,11 +768,13 @@ def transform_param_value( * int * ref * resolved-ref + * upa * list where X is any of the above - value - anything or None. Parameter values are expected, by the KBase app stack, to generally be - either a singleton or a list of singletons. In practice, they're usually strings, ints, floats, None, - or a list of those. - spec_param - either None or a spec parameter dictionary as defined by SpecManager.app_params. That is: + value - anything or None. Parameter values are expected, by the KBase app stack, to + generally be either a singleton or a list of singletons. In practice, they're usually + strings, ints, floats, None, or a list of those. + spec_param - either None or a spec parameter dictionary as defined by + SpecManager.app_params. That is: { optional = boolean, is_constant = boolean, @@ -811,16 +810,6 @@ def transform_param_value( ): return value - # Here's the issue. - # We're getting values that can be either a string, a ref, or an UPA. - # Now, when we get transform_type = "ref"/"unresolved-ref", that - # should be transformed to a ws/obj ref - # if transform_type == "ref" or transform_type == "unresolved-ref": - # # make unresolved workspace ref (like 'ws-name/obj-name') - # if value is not None and "/" not in value: - # value = system_variable("workspace") + "/" + value - # return value - if transform_type in ["ref", "unresolved-ref", "resolved-ref", "upa"] or ( is_input_object_param and transform_type is None ): @@ -863,6 +852,8 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) - should return UPA transform = None: - should return object name + Note that if it is a reference path, it was always get returned as an UPA-path + for the best compatibility. value can be either object name, ref, upa, or ref-path can tell by testing with UPA api From 2cfc45e4fd96cda784def0b4f051908fb79f144b Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Tue, 15 Aug 2023 16:57:14 -0400 Subject: [PATCH 09/12] adjustments to tests, transform_param_value function --- src/biokbase/narrative/app_util.py | 26 ++++++++++------- .../tests/narrative_mock/mockclients.py | 14 +++++++-- .../narrative/tests/test_appmanager.py | 29 +++++++++---------- 3 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index c87f59c90e..7b4ee29cca 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -95,7 +95,7 @@ def _untransform(transform_type, value): if slash == -1: return value else: - return value[slash + 1 :] + return value[slash + 1:] else: return value @@ -115,7 +115,8 @@ def app_param(p): "type": string "is_output": boolean "allow_multiple": boolean - "allowed_values": present if type == "dropdown" or "checkbox", a list of allowed dropdown values + "allowed_values": present if type == "dropdown" or "checkbox", a list of allowed dropdown + values "default": None, singleton, or list if "allow_multiple" is True "checkbox_map": [checked_value, unchecked_value] if type == checkbox The following only apply if the "text_options" field is present in the original spec @@ -261,7 +262,8 @@ def get_result_sub_path(result, path): So it recursively works through. look at path[pos] - that's a 0, so get the 0th element of result (the dict that's there.) - since result is a list (it almost always is, to start with, since that's what run_job returns), we get: + since result is a list (it almost always is, to start with, since that's what run_job + returns), we get: result[path[0]] = {'report': 'asdf', 'report_ref': 'xyz'} path = ['0', 'report_ref'] pos = 1 @@ -687,12 +689,8 @@ def resolve_single_ref(workspace, value): for path_item in path_items: if len(path_item.split("/")) > 3: raise ValueError( - "Object reference {} has too many slashes - should be workspace/object/version(optional)".format( - value - ) + f"Object reference {value} has too many slashes - should be ws/object/version" ) - # return (ws_ref, 'Data reference named {} does not have the right format - # - should be workspace/object/version(optional)') info = clients.get("workspace").get_object_info_new( {"objects": [{"ref": value}]} )[0] @@ -870,6 +868,10 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) search_ref = value if not is_upa and not is_ref: + if transform_type is None: + # it's already "transformed" - nothing to do, so don't waste time + # fetching it. + return value search_ref = f"{system_variable('workspace')}/{value}" try: obj_info = clients.get("workspace").get_object_info3( @@ -877,8 +879,12 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) ) except Exception as err: print(err) - print(f"Error while searching for object ref '{search_ref}'") - return value + transform = transform_type + if transform is None: + transform = "object name" + raise ValueError( + f"Unable to find object reference '{search_ref}' to transform as {transform}" + ) if is_path: return ";".join(obj_info["paths"][0]) diff --git a/src/biokbase/narrative/tests/narrative_mock/mockclients.py b/src/biokbase/narrative/tests/narrative_mock/mockclients.py index b7a2341438..e86e912af8 100644 --- a/src/biokbase/narrative/tests/narrative_mock/mockclients.py +++ b/src/biokbase/narrative/tests/narrative_mock/mockclients.py @@ -232,9 +232,19 @@ def get_object_info3(self, params): None, ] ] - paths = [["18836/5/1"]] + upa = "18836/5/1" num_objects = len(params.get("objects", [0])) - return {"infos": infos * num_objects, "paths": paths * num_objects} + paths = [] + for obj_ident in params.get("objects", []): + ref_path = [] + if "ref" in obj_ident and ";" in obj_ident["ref"]: + num_steps = len(obj_ident["ref"].split(";")) + for step in range(num_steps - 1): + i = step + 1 + ref_path.append(f"{i}/{i}/{i}") + ref_path.append(upa) + paths.append(ref_path) + return {"infos": infos * num_objects, "paths": paths} def list_objects(self, params): ws_ids = params["ids"] diff --git a/src/biokbase/narrative/tests/test_appmanager.py b/src/biokbase/narrative/tests/test_appmanager.py index b07a8f2517..5b83c232b5 100644 --- a/src/biokbase/narrative/tests/test_appmanager.py +++ b/src/biokbase/narrative/tests/test_appmanager.py @@ -1072,8 +1072,9 @@ def test_input_mapping(self): ref_path = ( ws_name + "/MyReadsSet; " + ws_name + "/rhodobacterium.art.q10.PE.reads" ) + # ref_paths get mocked as 1/1/1;2/2/2;...N/N/N;18836/5/1 ret = app_util.transform_param_value("resolved-ref", ref_path, None) - self.assertEqual(ret, ws_name + "/MyReadsSet;18836/5/1") + self.assertEqual(ret, "1/1/1;18836/5/1") @mock.patch(CLIENTS_AM_SM, get_mock_client) def test_generate_input(self): @@ -1096,19 +1097,19 @@ def test_transform_input_good(self): ws_name = self.public_ws test_data = [ { - "value": "input_value", + "value": READS_OBJ_1, "type": "ref", - "expected": ws_name + "/" + "input_value", + "expected": ws_name + "/" + READS_OBJ_1, }, { - "value": ws_name + "/input_value", + "value": ws_name + "/" + READS_OBJ_1, "type": "ref", - "expected": ws_name + "/" + "input_value", + "expected": ws_name + "/" + READS_OBJ_1, }, { - "value": "input_value", + "value": READS_OBJ_1, "type": "unresolved-ref", - "expected": ws_name + "/" + "input_value", + "expected": ws_name + "/" + READS_OBJ_1, }, { "value": READS_OBJ_1, @@ -1123,9 +1124,9 @@ def test_transform_input_good(self): {"value": None, "type": "int", "expected": None}, {"value": "5", "type": "int", "expected": 5}, { - "value": ["a", "b", "c"], + "value": [READS_OBJ_1, READS_OBJ_2], "type": "list", - "expected": [ws_name + "/a", ws_name + "/b", ws_name + "/c"], + "expected": [ws_name + "/" + READS_OBJ_1, ws_name + "/" + READS_OBJ_2], }, { "value": [ @@ -1135,15 +1136,13 @@ def test_transform_input_good(self): "type": "list", "expected": ["11635/9/1", "11635/10/1"], }, - {"value": "foo", "type": "list", "expected": [ws_name + "/foo"]}, - {"value": ["1", "2", 3], "type": "list", "expected": [1, 2, 3]}, - {"value": "bar", "type": None, "expected": "bar"}, { "value": READS_OBJ_1, - "type": "future-default", - "spec": {"is_output": 0, "allowed_types": ["Some.KnownType"]}, - "expected": "11635/9/1", + "type": "list", + "expected": [ws_name + "/" + READS_OBJ_1], }, + {"value": ["1", "2", 3], "type": "list", "expected": [1, 2, 3]}, + {"value": "bar", "type": None, "expected": "bar"}, {"value": [123, 456], "type": None, "expected": [123, 456]}, {"value": 123, "type": "string", "expected": "123"}, { From d55acb159151129bf7f68d3846bcf2b12e10f38f Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 21 Aug 2023 15:45:36 -0400 Subject: [PATCH 10/12] fix tests and test data --- src/biokbase/narrative/app_util.py | 27 ++++++++++++++----- .../tests/data/ee2_job_test_data.json | 4 +-- .../narrative/tests/data/response_data.json | 20 +++++++------- src/biokbase/narrative/tests/test_app_util.py | 6 +++-- src/biokbase/narrative/tests/test_jobcomm.py | 7 +++++ 5 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 7b4ee29cca..93356e531f 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -858,6 +858,7 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) If we can't find any object info on the value, just return the value as-is """ + print(f"transform: {transform_type} value: {value}") if value is None: return None @@ -866,12 +867,25 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) is_ref = upa.is_ref(value) is_path = (is_upa or is_ref) and ";" in value + # simple cases: + # 1. if is_upa and we want resolved-ref or upa, return the value + # 2. if is_ref and not is_upa and we want ref or unresolved-ref, return the value + # 3. if is neither upa or ref and transform is None, then we want the string, so return that + # 4. Otherwise, look up the object and return what's desired from there. + + if is_upa and transform_type in ["upa", "resolved-ref"]: + print(f"returning {value}") + return value + if is_ref and not is_upa and transform_type in ["ref", "unresolved-ref"]: + print(f"returning {value}") + return value + if not is_upa and not is_ref and transform_type is None: + print(f"returning {value}") + return value + + search_ref = value if not is_upa and not is_ref: - if transform_type is None: - # it's already "transformed" - nothing to do, so don't waste time - # fetching it. - return value search_ref = f"{system_variable('workspace')}/{value}" try: obj_info = clients.get("workspace").get_object_info3( @@ -891,9 +905,8 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) if transform_type == "ref" or transform_type == "unresolved-ref": obj = obj_info["infos"][0] return f"{obj[7]}/{obj[1]}" - if transform_type == "resolved-ref" or transform_type == "upa": - if is_upa: - return value + if transform_type in ["resolved-ref", "upa"]: + print(f"returning at end: {';'.join(obj_info['paths'][0])}") return ";".join(obj_info["paths"][0]) if transform_type is None: return obj_info["infos"][0][1] diff --git a/src/biokbase/narrative/tests/data/ee2_job_test_data.json b/src/biokbase/narrative/tests/data/ee2_job_test_data.json index 94db803aeb..44d4b9223b 100644 --- a/src/biokbase/narrative/tests/data/ee2_job_test_data.json +++ b/src/biokbase/narrative/tests/data/ee2_job_test_data.json @@ -292,7 +292,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/2/12", "report_name": "kb_sra_upload_report_af468a02-4278-40af-9536-0bdef1f050db", "report_ref": "62425/18/1", "report_window_line_height": "16", @@ -512,7 +512,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", diff --git a/src/biokbase/narrative/tests/data/response_data.json b/src/biokbase/narrative/tests/data/response_data.json index d5a85ff521..ddd42028aa 100644 --- a/src/biokbase/narrative/tests/data/response_data.json +++ b/src/biokbase/narrative/tests/data/response_data.json @@ -401,7 +401,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/2/12", "report_name": "kb_sra_upload_report_af468a02-4278-40af-9536-0bdef1f050db", "report_ref": "62425/18/1", "report_window_line_height": "16", @@ -414,7 +414,7 @@ "outputWidgetInfo": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/2/12", "report_name": "kb_sra_upload_report_af468a02-4278-40af-9536-0bdef1f050db", "report_ref": "62425/18/1", "report_window_line_height": "16", @@ -502,7 +502,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", @@ -515,7 +515,7 @@ "outputWidgetInfo": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", @@ -773,7 +773,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/2/12", "report_name": "kb_sra_upload_report_af468a02-4278-40af-9536-0bdef1f050db", "report_ref": "62425/18/1", "report_window_line_height": "16", @@ -786,7 +786,7 @@ "outputWidgetInfo": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/2/12", "report_name": "kb_sra_upload_report_af468a02-4278-40af-9536-0bdef1f050db", "report_ref": "62425/18/1", "report_window_line_height": "16", @@ -913,7 +913,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", @@ -926,7 +926,7 @@ "outputWidgetInfo": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", @@ -1065,7 +1065,7 @@ "widget_info": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", @@ -1078,7 +1078,7 @@ "outputWidgetInfo": { "name": "no-display", "params": { - "obj_ref": "18836/5/1", + "obj_ref": "62425/17/1", "report_name": "kb_sra_upload_report_4ffc6219-0260-4f1d-8b4d-5083e0595150", "report_ref": "62425/19/1", "report_window_line_height": "16", diff --git a/src/biokbase/narrative/tests/test_app_util.py b/src/biokbase/narrative/tests/test_app_util.py index cba066c383..c53aa19ac7 100644 --- a/src/biokbase/narrative/tests/test_app_util.py +++ b/src/biokbase/narrative/tests/test_app_util.py @@ -334,7 +334,7 @@ def test_transform_param_value_textsubdata(value, expected): mock_obj_ref = "wjriehl:1490995018528/Sbicolor2" ref_cases = [ (None, None), - ("foo/bar", mock_obj_ref), + ("foo/bar", "foo/bar"), ("1/2/3", mock_obj_ref), ("Sbicolor2", mock_obj_ref), ] @@ -432,6 +432,8 @@ def test_transform_param_value_upa_path(): @mock.patch("biokbase.narrative.app_util.clients.get", get_ref_path_mock_ws) def test_transform_param_value_ref_path(): ref_path = "some_ws/some_obj;some_other_ws/some_other_obj" + for tf_type in ["ref", "unresolved-ref"]: + assert transform_param_value(tf_type, ref_path, None) == ref_path expected_upa = "1/1/1;2/2/2" - for tf_type in ["ref", "unresolved-ref", "upa", "resolved-ref"]: + for tf_type in ["upa", "resolved-ref"]: assert transform_param_value(tf_type, ref_path, None) == expected_upa diff --git a/src/biokbase/narrative/tests/test_jobcomm.py b/src/biokbase/narrative/tests/test_jobcomm.py index ce0fc1fea9..b7135d7176 100644 --- a/src/biokbase/narrative/tests/test_jobcomm.py +++ b/src/biokbase/narrative/tests/test_jobcomm.py @@ -876,11 +876,18 @@ def test_cancel_jobs__job_id_list__failure(self): @mock.patch(CLIENTS, get_mock_client) def check_retry_jobs(self, job_args, job_id_list): + import json req_dict = make_comm_msg(RETRY, job_args, False) expected = { job_id: ALL_RESPONSE_DATA[RETRY][job_id] for job_id in job_id_list if job_id } + # print("request") + # print(json.dumps(req_dict, indent=4)) retry_data = self.jc._handle_comm_message(req_dict) + # print("reply") + # print(json.dumps(retry_data, indent=4)) + # print("expected") + # print(json.dumps(expected, indent=4)) self.assertEqual(expected, retry_data) retry_msg = self.jc._comm.pop_message() self.assertEqual( From 963a8dec82fb14cb0451aa1ab52e5dc3deb55bb3 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Thu, 24 Aug 2023 08:46:52 -0400 Subject: [PATCH 11/12] remove print statements --- src/biokbase/narrative/app_util.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/biokbase/narrative/app_util.py b/src/biokbase/narrative/app_util.py index 93356e531f..d8b2bd8536 100644 --- a/src/biokbase/narrative/app_util.py +++ b/src/biokbase/narrative/app_util.py @@ -858,7 +858,6 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) If we can't find any object info on the value, just return the value as-is """ - print(f"transform: {transform_type} value: {value}") if value is None: return None @@ -874,13 +873,10 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) # 4. Otherwise, look up the object and return what's desired from there. if is_upa and transform_type in ["upa", "resolved-ref"]: - print(f"returning {value}") return value if is_ref and not is_upa and transform_type in ["ref", "unresolved-ref"]: - print(f"returning {value}") return value if not is_upa and not is_ref and transform_type is None: - print(f"returning {value}") return value @@ -891,8 +887,7 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) obj_info = clients.get("workspace").get_object_info3( {"objects": [{"ref": search_ref}]} ) - except Exception as err: - print(err) + except Exception: transform = transform_type if transform is None: transform = "object name" @@ -902,11 +897,10 @@ def transform_object_value(transform_type: Optional[str], value: Optional[str]) if is_path: return ";".join(obj_info["paths"][0]) - if transform_type == "ref" or transform_type == "unresolved-ref": + if transform_type in ["ref", "unresolved-ref"]: obj = obj_info["infos"][0] return f"{obj[7]}/{obj[1]}" if transform_type in ["resolved-ref", "upa"]: - print(f"returning at end: {';'.join(obj_info['paths'][0])}") return ";".join(obj_info["paths"][0]) if transform_type is None: return obj_info["infos"][0][1] From d869e99ca10442ce6982437d078bab1a30894344 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Thu, 24 Aug 2023 08:55:45 -0400 Subject: [PATCH 12/12] remove print statements --- src/biokbase/narrative/tests/test_jobcomm.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/biokbase/narrative/tests/test_jobcomm.py b/src/biokbase/narrative/tests/test_jobcomm.py index b7135d7176..ce0fc1fea9 100644 --- a/src/biokbase/narrative/tests/test_jobcomm.py +++ b/src/biokbase/narrative/tests/test_jobcomm.py @@ -876,18 +876,11 @@ def test_cancel_jobs__job_id_list__failure(self): @mock.patch(CLIENTS, get_mock_client) def check_retry_jobs(self, job_args, job_id_list): - import json req_dict = make_comm_msg(RETRY, job_args, False) expected = { job_id: ALL_RESPONSE_DATA[RETRY][job_id] for job_id in job_id_list if job_id } - # print("request") - # print(json.dumps(req_dict, indent=4)) retry_data = self.jc._handle_comm_message(req_dict) - # print("reply") - # print(json.dumps(retry_data, indent=4)) - # print("expected") - # print(json.dumps(expected, indent=4)) self.assertEqual(expected, retry_data) retry_msg = self.jc._comm.pop_message() self.assertEqual(