From 9ecd8931c6f4d059d60cd6df8f9317ed5fc0e7ec Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 8 Aug 2024 20:46:52 -0400 Subject: [PATCH 1/9] Not a problem now that we're not using basic.py abstractions. --- lib/galaxy/tool_util/verify/parse.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/verify/parse.py b/lib/galaxy/tool_util/verify/parse.py index eaec6fce31d1..a210ab5ea840 100644 --- a/lib/galaxy/tool_util/verify/parse.py +++ b/lib/galaxy/tool_util/verify/parse.py @@ -357,7 +357,7 @@ def process_param_value(param_value): if value_for_text is None and param_value == text: value_for_text = opt_value dynamic_options = param.parse_dynamic_options() - if dynamic_options and not input_type == "drill_down": + if dynamic_options: data_table_name = dynamic_options.get_data_table_name() index_file_name = dynamic_options.get_index_file_name() if data_table_name: From ed09ca254f41ae1dc18e948e0672ae65a0750506 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 9 Aug 2024 17:40:37 -0400 Subject: [PATCH 2/9] Handle collection defaults in parameter models... --- lib/galaxy/tool_util/parameters/factory.py | 2 ++ lib/galaxy/tool_util/parameters/models.py | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/parameters/factory.py b/lib/galaxy/tool_util/parameters/factory.py index 8e56712be5a6..2686be7613fc 100644 --- a/lib/galaxy/tool_util/parameters/factory.py +++ b/lib/galaxy/tool_util/parameters/factory.py @@ -135,9 +135,11 @@ def _from_input_source_galaxy(input_source: InputSource) -> ToolParameterT: ) elif param_type == "data_collection": optional = input_source.parse_optional() + default_value = input_source.parse_default() return DataCollectionParameterModel( name=input_source.parse_name(), optional=optional, + value=default_value, ) elif param_type == "select": # Function... example in devteam cummeRbund. diff --git a/lib/galaxy/tool_util/parameters/models.py b/lib/galaxy/tool_util/parameters/models.py index 5d5a2266d895..39bd49f9b2f4 100644 --- a/lib/galaxy/tool_util/parameters/models.py +++ b/lib/galaxy/tool_util/parameters/models.py @@ -322,6 +322,7 @@ class DataCollectionParameterModel(BaseGalaxyToolParameterModelDefinition): parameter_type: Literal["gx_data_collection"] = "gx_data_collection" collection_type: Optional[str] = None extensions: List[str] = ["data"] + value: Optional[Dict[str, Any]] @property def py_type(self) -> Type: @@ -341,7 +342,7 @@ def pydantic_template(self, state_representation: StateRepresentationT) -> Dynam @property def request_requires_value(self) -> bool: - return not self.optional + return not self.optional and self.value is None class HiddenParameterModel(BaseGalaxyToolParameterModelDefinition): From dba435561280e69193876b26c0bfd8eaafd59c96 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 9 Aug 2024 10:18:11 -0400 Subject: [PATCH 3/9] Allow Paths in tool source factory method. --- lib/galaxy/tool_util/parser/factory.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/parser/factory.py b/lib/galaxy/tool_util/parser/factory.py index 6a88f7b9d960..4dda4589e596 100644 --- a/lib/galaxy/tool_util/parser/factory.py +++ b/lib/galaxy/tool_util/parser/factory.py @@ -1,11 +1,13 @@ """Constructors for concrete tool and input source objects.""" import logging +from pathlib import PurePath from typing import ( Callable, Dict, List, Optional, + Union, ) from yaml import safe_load @@ -59,7 +61,7 @@ def build_yaml_tool_source(yaml_string: str) -> YamlToolSource: def get_tool_source( - config_file: Optional[str] = None, + config_file: Optional[Union[str, PurePath]] = None, xml_tree: Optional[ElementTree] = None, enable_beta_formats: bool = True, tool_location_fetcher: Optional[ToolLocationFetcher] = None, @@ -85,6 +87,9 @@ def get_tool_source( tool_location_fetcher = ToolLocationFetcher() assert config_file + if isinstance(config_file, PurePath): + config_file = str(config_file) + config_file = tool_location_fetcher.to_tool_path(config_file) if not enable_beta_formats: tree, macro_paths = load_tool_with_refereces(config_file) From e475003830f8d7f6382d5606dc0636cd36194887 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 8 Aug 2024 21:22:23 -0400 Subject: [PATCH 4/9] Disable unqualified parameters in test cases for new tools. --- lib/galaxy/tool_util/verify/parse.py | 41 +++++++--- .../deprecated/simple_constructs_24_2.xml | 80 +++++++++++++++++++ .../tool_util/test_test_definition_parsing.py | 6 ++ 3 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 test/functional/tools/deprecated/simple_constructs_24_2.xml diff --git a/lib/galaxy/tool_util/verify/parse.py b/lib/galaxy/tool_util/verify/parse.py index a210ab5ea840..a54712b7259e 100644 --- a/lib/galaxy/tool_util/verify/parse.py +++ b/lib/galaxy/tool_util/verify/parse.py @@ -8,6 +8,8 @@ Union, ) +from packaging.version import Version + from galaxy.tool_util.parser.interface import ( InputSource, TestCollectionDef, @@ -139,7 +141,9 @@ def _process_raw_inputs( (| using to nest to new levels) structure and expand dataset information as proceeding to populate self.required_files. """ - parent_context = parent_context or RootParamContext() + profile = tool_source.parse_profile() + allow_legacy_test_case_parameters = Version(profile) <= Version("24.1") + parent_context = parent_context or RootParamContext(allow_unqualified_access=allow_legacy_test_case_parameters) expanded_inputs: ExpandedToolInputs = {} for input_source in input_sources: input_type = input_source.parse_input_type() @@ -274,10 +278,22 @@ def input_sources(tool_source: ToolSource) -> List[InputSource]: class ParamContext: + """Capture the context of a parameter's position within the inputs tree of a tool.""" + + parent_context: AnyParamContext + name: str + # if in a repeat - what position in the repeat + index: Optional[int] + # we've encouraged the use of repeat/conditional tags to capture fully qualified paths + # to parameters in tools. This brings the parameters closer to the API and prevents a + # variety of possible ambiguities. Disable this for newer tools. + allow_unqualified_access: bool + def __init__(self, name: str, parent_context: AnyParamContext, index: Optional[int] = None): self.parent_context = parent_context self.name = name self.index = None if index is None else int(index) + self.allow_unqualified_access = parent_context.allow_unqualified_access def for_state(self) -> str: name = self.name if self.index is None else "%s_%d" % (self.name, self.index) @@ -291,15 +307,18 @@ def __str__(self) -> str: return f"Context[for_state={self.for_state()}]" def param_names(self): - for parent_context_param in self.parent_context.param_names(): + if not self.allow_unqualified_access: + yield self.for_state() + else: + for parent_context_param in self.parent_context.param_names(): + if self.index is not None: + yield "%s|%s_%d" % (parent_context_param, self.name, self.index) + else: + yield f"{parent_context_param}|{self.name}" if self.index is not None: - yield "%s|%s_%d" % (parent_context_param, self.name, self.index) + yield "%s_%d" % (self.name, self.index) else: - yield f"{parent_context_param}|{self.name}" - if self.index is not None: - yield "%s_%d" % (self.name, self.index) - else: - yield self.name + yield self.name def extract_value(self, raw_inputs: ToolSourceTestInputs): for param_name in self.param_names(): @@ -322,8 +341,10 @@ def __raw_param_found(self, param_name: str, raw_inputs: ToolSourceTestInputs): class RootParamContext: - def __init__(self): - pass + allow_unqualified_access: bool + + def __init__(self, allow_unqualified_access: bool): + self.allow_unqualified_access = allow_unqualified_access def for_state(self): return "" diff --git a/test/functional/tools/deprecated/simple_constructs_24_2.xml b/test/functional/tools/deprecated/simple_constructs_24_2.xml new file mode 100644 index 000000000000..ce545d8161d8 --- /dev/null +++ b/test/functional/tools/deprecated/simple_constructs_24_2.xml @@ -0,0 +1,80 @@ + + + '$out_file1' && +echo $booltest >> '$out_file1' && +echo $inttest >> '$out_file1' && +echo $floattest >> '$out_file1' && +echo $radio_select >> '$out_file1' && +echo $check_select >> '$out_file1' && +echo $drop_select >> '$out_file1' && +#if len($files) > 0 + cat '$files[0].file' >> '$out_file1' +#end if + ]]> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/unit/tool_util/test_test_definition_parsing.py b/test/unit/tool_util/test_test_definition_parsing.py index 39d9e9041b29..032bd69d4c76 100644 --- a/test/unit/tool_util/test_test_definition_parsing.py +++ b/test/unit/tool_util/test_test_definition_parsing.py @@ -97,6 +97,12 @@ def test_collection_type_source_parsing(self): test_dicts = self._parse_tests() self._verify_each(test_dicts[0].to_dict(), COLLECTION_TYPE_SOURCE_EXPECTATIONS) + def test_unqualified_access_disabled_in_24_2(self): + self._init_tool_for_path(functional_test_tool_path("deprecated/simple_constructs_24_2.xml")) + test_dicts = self._parse_tests() + test_0 = test_dicts[0].to_dict() + assert "p1|p1use" not in test_0["inputs"] + def test_bigwigtowig_converter(self): # defines if in_packages(): From 7f4ad540be65d7cda3ea914a16b67396caeee98e Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 8 Aug 2024 21:30:28 -0400 Subject: [PATCH 5/9] Disable more test case ambiguities for newer profile tools. --- lib/galaxy/tool_util/verify/parse.py | 58 +++++++++++++++++++--------- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/lib/galaxy/tool_util/verify/parse.py b/lib/galaxy/tool_util/verify/parse.py index a54712b7259e..aa32b5328ffe 100644 --- a/lib/galaxy/tool_util/verify/parse.py +++ b/lib/galaxy/tool_util/verify/parse.py @@ -156,7 +156,7 @@ def _process_raw_inputs( raw_input_dict = case_context.extract_value(raw_inputs) case_value = raw_input_dict["value"] if raw_input_dict else None case_when, case_input_sources = _matching_case_for_value( - tool_source, input_source, test_param_input_source, case_value + tool_source, input_source, test_param_input_source, case_value, allow_legacy_test_case_parameters ) if case_input_sources: for case_input_source in case_input_sources.parse_input_sources(): @@ -180,7 +180,11 @@ def _process_raw_inputs( # an infinite loop - hence the "case_value is not None" # check. processed_value = _process_simple_value( - test_param_input_source, expanded_case_value, required_data_tables, required_loc_files + test_param_input_source, + expanded_case_value, + required_data_tables, + required_loc_files, + allow_legacy_test_case_parameters, ) expanded_inputs[case_context.for_state()] = processed_value elif input_type == "section": @@ -261,7 +265,11 @@ def _process_raw_inputs( processed_value = collection_def else: processed_value = _process_simple_value( - input_source, param_value, required_data_tables, required_loc_files + input_source, + param_value, + required_data_tables, + required_loc_files, + allow_legacy_test_case_parameters, ) expanded_inputs[context.for_state()] = processed_value return expanded_inputs @@ -361,6 +369,7 @@ def _process_simple_value( param_value: Any, required_data_tables: RequiredDataTablesT, required_loc_files: RequiredLocFileT, + allow_legacy_test_case_parameters: bool, ): input_type = param.get("type") if input_type == "select": @@ -371,12 +380,15 @@ def _process_simple_value( def process_param_value(param_value): found_value = False value_for_text = None - static_options = param.parse_static_options() - for text, opt_value, _ in static_options: - if param_value == opt_value: - found_value = True - if value_for_text is None and param_value == text: - value_for_text = opt_value + if allow_legacy_test_case_parameters: + # we used to allow selections based on text - this + # should really be only based on key + static_options = param.parse_static_options() + for text, opt_value, _ in static_options: + if param_value == opt_value: + found_value = True + if value_for_text is None and param_value == text: + value_for_text = opt_value dynamic_options = param.parse_dynamic_options() if dynamic_options: data_table_name = dynamic_options.get_data_table_name() @@ -400,13 +412,19 @@ def process_param_value(param_value): elif input_type == "boolean": # Like above, tests may use the tool define values of simply # true/false. - processed_value = _process_bool_param_value(param, param_value) + processed_value = _process_bool_param_value(param, param_value, allow_legacy_test_case_parameters) else: processed_value = param_value return processed_value -def _matching_case_for_value(tool_source: ToolSource, cond: InputSource, test_param: InputSource, declared_value: Any): +def _matching_case_for_value( + tool_source: ToolSource, + cond: InputSource, + test_param: InputSource, + declared_value: Any, + allow_legacy_test_case_parameters: bool, +): tool_id = tool_source.parse_id() cond_name = cond.parse_name() @@ -418,10 +436,10 @@ def _matching_case_for_value(tool_source: ToolSource, cond: InputSource, test_pa # No explicit value for param in test case, determine from default query_value = boolean_is_checked(test_param) else: - query_value = _process_bool_param_value(test_param, declared_value) + query_value = _process_bool_param_value(test_param, declared_value, allow_legacy_test_case_parameters) def matches_declared_value(case_value): - return _process_bool_param_value(test_param, case_value) == query_value + return _process_bool_param_value(test_param, case_value, allow_legacy_test_case_parameters) == query_value elif test_param_type == "select": static_options = test_param.parse_static_options() @@ -498,21 +516,25 @@ def require_file(name: str, value: str, extra: ExtraFileInfoDictT, required_file return value -def _process_bool_param_value(input_source: InputSource, param_value: Any) -> Any: +def _process_bool_param_value( + input_source: InputSource, param_value: Any, allow_legacy_test_case_parameters: bool +) -> Any: truevalue, falsevalue = boolean_true_and_false_values(input_source) optional = input_source.parse_optional() - return process_bool_param_value(truevalue, falsevalue, optional, param_value) + return process_bool_param_value(truevalue, falsevalue, optional, param_value, allow_legacy_test_case_parameters) -def process_bool_param_value(truevalue: str, falsevalue: str, optional: bool, param_value: Any) -> Any: +def process_bool_param_value( + truevalue: str, falsevalue: str, optional: bool, param_value: Any, allow_legacy_test_case_parameters: bool +) -> Any: was_list = False if isinstance(param_value, list): was_list = True param_value = param_value[0] - if truevalue == param_value: + if allow_legacy_test_case_parameters and truevalue == param_value: processed_value = True - elif falsevalue == param_value: + elif allow_legacy_test_case_parameters and falsevalue == param_value: processed_value = False else: if optional: From cc420a64ae9fd94f9464f0928e1a8fda687ac25b Mon Sep 17 00:00:00 2001 From: John Chilton Date: Sat, 10 Aug 2024 16:35:17 -0400 Subject: [PATCH 6/9] Prevent model parameters from issuing warnings on certain valid XML inputs. --- lib/galaxy/tool_util/parameters/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/parameters/models.py b/lib/galaxy/tool_util/parameters/models.py index 39bd49f9b2f4..77c5f29ca3ca 100644 --- a/lib/galaxy/tool_util/parameters/models.py +++ b/lib/galaxy/tool_util/parameters/models.py @@ -1046,7 +1046,8 @@ def simple_input_models( def create_model_strict(*args, **kwd) -> Type[BaseModel]: - model_config = ConfigDict(extra="forbid") + # proteted_namespaces here prevents tool with model_ parameter names from issueing warnings + model_config = ConfigDict(extra="forbid", protected_namespaces=()) return create_model(*args, __config__=model_config, **kwd) From da220efd23a3bd8cd6aa13bcfd163e58499184fd Mon Sep 17 00:00:00 2001 From: John Chilton Date: Sat, 10 Aug 2024 10:52:55 -0400 Subject: [PATCH 7/9] First pass at workflow step models - linked and unlinked. Work scoped out in 18536. --- .../dev/tool_state_state_classes.plantuml.svg | 29 +- lib/galaxy/tool_util/parameters/__init__.py | 18 +- lib/galaxy/tool_util/parameters/_types.py | 7 +- lib/galaxy/tool_util/parameters/factory.py | 12 +- lib/galaxy/tool_util/parameters/models.py | 153 ++++++++-- lib/galaxy/tool_util/parameters/state.py | 23 +- lib/galaxy/tool_util/parameters/visitor.py | 51 ++++ .../tool_util/parameter_specification.yml | 284 +++++++++++++++++- .../tool_util/test_parameter_specification.py | 52 ++++ 9 files changed, 585 insertions(+), 44 deletions(-) diff --git a/doc/source/dev/tool_state_state_classes.plantuml.svg b/doc/source/dev/tool_state_state_classes.plantuml.svg index b0c086bf18b0..de86dbcd3787 100644 --- a/doc/source/dev/tool_state_state_classes.plantuml.svg +++ b/doc/source/dev/tool_state_state_classes.plantuml.svg @@ -1,14 +1,21 @@ -galaxy.tool_util.parameters.stateToolStatestate_representation: strinput_state: Dict[str, Any]validate(input_models: ToolParameterBundle)_to_base_model(input_models: ToolParameterBundle): Optional[Type[BaseModel]]RequestToolStatestate_representation = "request"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <encoded_id>}.Allow mapping/reduce constructs.RequestInternalToolStatestate_representation = "request_internal"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <decoded_id>}.Allow mapping/reduce constructs.JobInternalToolStatestate_representation = "job_internal"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <decoded_id>}.Mapping constructs expanded out.(Defaults are inserted?)decodeexpandgalaxy.tool_util.parameters.stateToolStatestate_representation: strinput_state: Dict[str, Any]validate(input_models: ToolParameterBundle)_to_base_model(input_models: ToolParameterBundle): Optional[Type[BaseModel]]RequestToolStatestate_representation = "request"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <encoded_id>}.Allow mapping/reduce constructs.RequestInternalToolStatestate_representation = "request_internal"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <decoded_id>}.Allow mapping/reduce constructs.JobInternalToolStatestate_representation = "job_internal"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form{src: "hda", id: <decoded_id>}.Mapping constructs expanded out.(Defaults are inserted?)TestCaseToolStatestate_representation = "test_case"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Object references of the form file name and URIs.Mapping constructs not allowed. WorkflowStepToolStatestate_representation = "workflow_step"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Nearly everything optional except conditional discriminators. WorkflowStepLinkedToolStatestate_representation = "workflow_step_linked"_to_base_model(input_models: ToolParameterBundle): Type[BaseModel]Expect pre-process ``in`` dictionaries and bring in representationof links and defaults and validate them in model. decodeexpandpreprocess_links_and_defaults