From c9832dd88c4b85873f31a7ae0c4dd1bdf88369b2 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 16 Dec 2024 14:51:26 +0100 Subject: [PATCH 1/6] Add test case for optional unspecified text parameter which is accessed after scheduling. Caused the failure in https://github.com/galaxyproject/galaxy/issues/19328. --- ...nal_text_param_rescheduling.gxwf-tests.yml | 12 ++++++++ .../optional_text_param_rescheduling.gxwf.yml | 30 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf-tests.yml create mode 100644 lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf.yml diff --git a/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf-tests.yml b/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf-tests.yml new file mode 100644 index 000000000000..c531a2b803bb --- /dev/null +++ b/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf-tests.yml @@ -0,0 +1,12 @@ +- doc: | + Test that scheduling succeeds even if step needs rescheduling + job: + optional_text: + type: raw + value: null + outputs: + out: + class: File + asserts: + - that: has_text + text: "null" diff --git a/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf.yml b/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf.yml new file mode 100644 index 000000000000..dbf5a4b57b64 --- /dev/null +++ b/lib/galaxy_test/workflow/optional_text_param_rescheduling.gxwf.yml @@ -0,0 +1,30 @@ +class: GalaxyWorkflow +inputs: + optional_text: + type: text + optional: true + when: + type: boolean + default: true +outputs: + out: + outputSource: gx_text_optional/inputs_json +steps: + pick_value: + tool_id: pick_value + tool_state: + style_cond: + pick_style: first + type_cond: + param_type: boolean + pick_from: + - value: true + gx_text_optional: + tool_id: gx_text_optional + # the when expression requires pick_value to execute, which recapitulates https://github.com/galaxyproject/galaxy/issues/19328 + when: $(inputs.when) + in: + parameter: + source: optional_text + when: + source: pick_value/boolean_param From 20d904ec621746dd69d25b400b44044a2235e687 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 18 Dec 2024 10:39:29 +0100 Subject: [PATCH 2/6] Add type annotation for workflow_step --- lib/galaxy/model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index edb721c1d30e..e5057dd6bee4 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -9420,7 +9420,7 @@ class WorkflowInvocationStep(Base, Dictifiable, Serializable): ) action: Mapped[Optional[bytes]] = mapped_column(MutableJSONType) - workflow_step = relationship("WorkflowStep") + workflow_step: Mapped[WorkflowStep] = relationship("WorkflowStep") job: Mapped[Optional["Job"]] = relationship(back_populates="workflow_invocation_step", uselist=False) implicit_collection_jobs = relationship("ImplicitCollectionJobs", uselist=False) output_dataset_collections = relationship( From f4ee34f507c70f72fe2e79ce6facdefa8b0764b3 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 18 Dec 2024 10:51:30 +0100 Subject: [PATCH 3/6] Record NO_REPLACEMENT as step output for unspecified value --- lib/galaxy/model/__init__.py | 2 +- lib/galaxy/tools/parameters/__init__.py | 19 ++++++++++-- lib/galaxy/tools/parameters/basic.py | 14 +++++++-- lib/galaxy/tools/parameters/workflow_utils.py | 9 ++++++ lib/galaxy/workflow/modules.py | 26 ++++++++-------- lib/galaxy/workflow/refactor/execute.py | 6 ++-- lib/galaxy/workflow/run.py | 30 +++++++++---------- lib/galaxy/workflow/run_request.py | 4 ++- .../workflow/default_values.gxwf-tests.yml | 3 +- .../default_values_optional.gxwf-tests.yml | 3 +- test/unit/workflows/test_modules.py | 3 +- 11 files changed, 75 insertions(+), 44 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index e5057dd6bee4..ead37ac4e1a7 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -8108,7 +8108,7 @@ class WorkflowStep(Base, RepresentById, UsesCreateAndUpdateTime): tool_errors: Mapped[Optional[bytes]] = mapped_column(JSONType) position: Mapped[Optional[bytes]] = mapped_column(MutableJSONType) config: Mapped[Optional[bytes]] = mapped_column(JSONType) - order_index: Mapped[Optional[int]] + order_index: Mapped[int] when_expression: Mapped[Optional[bytes]] = mapped_column(JSONType) uuid: Mapped[Optional[Union[UUID, str]]] = mapped_column(UUIDType) label: Mapped[Optional[str]] = mapped_column(Unicode(255)) diff --git a/lib/galaxy/tools/parameters/__init__.py b/lib/galaxy/tools/parameters/__init__.py index ee3a8709817d..04c968910b45 100644 --- a/lib/galaxy/tools/parameters/__init__.py +++ b/lib/galaxy/tools/parameters/__init__.py @@ -31,6 +31,7 @@ ) from .workflow_utils import ( is_runtime_value, + NO_REPLACEMENT, runtime_to_json, ) from .wrapped import flat_to_nested_state @@ -180,8 +181,22 @@ def callback_helper(input, input_values, name_prefix, label_prefix, parent_prefi replace = new_value != no_replacement_value if replace: input_values[input.name] = new_value - elif replace_optional_connections and is_runtime_value(value) and hasattr(input, "value"): - input_values[input.name] = input.value + elif replace_optional_connections: + # Only used in workflow context + has_default = hasattr(input, "value") + if new_value is value is NO_REPLACEMENT: + # NO_REPLACEMENT means value was connected but left unspecified + if has_default: + # Use default if we have one + input_values[input.name] = input.value + else: + # Should fail if input is not optional and does not have default value + # Effectively however depends on parameter implementation. + # We might want to raise an exception here, instead of depending on a tool parameter value error. + input_values[input.name] = None + + elif is_runtime_value(value) and has_default: + input_values[input.name] = input.value def get_current_case(input, input_values): test_parameter = input.test_param diff --git a/lib/galaxy/tools/parameters/basic.py b/lib/galaxy/tools/parameters/basic.py index 34b325a0d123..1767c7e4e145 100644 --- a/lib/galaxy/tools/parameters/basic.py +++ b/lib/galaxy/tools/parameters/basic.py @@ -49,7 +49,10 @@ ParameterParseException, text_input_is_optional, ) -from galaxy.tools.parameters.workflow_utils import workflow_building_modes +from galaxy.tools.parameters.workflow_utils import ( + NO_REPLACEMENT, + workflow_building_modes, +) from galaxy.util import ( sanitize_param, string_as_bool, @@ -247,6 +250,8 @@ def to_python(self, value, app): def value_to_basic(self, value, app, use_security=False): if is_runtime_value(value): return runtime_to_json(value) + elif value == NO_REPLACEMENT: + return {"__class__": "NoReplacement"} return self.to_json(value, app, use_security) def value_from_basic(self, value, app, ignore_errors=False): @@ -255,8 +260,11 @@ def value_from_basic(self, value, app, ignore_errors=False): if isinstance(self, HiddenToolParameter): raise ParameterValueError(message_suffix="Runtime Parameter not valid", parameter_name=self.name) return runtime_to_object(value) - elif isinstance(value, MutableMapping) and value.get("__class__") == "UnvalidatedValue": - return value["value"] + elif isinstance(value, MutableMapping): + if value.get("__class__") == "UnvalidatedValue": + return value["value"] + elif value.get("__class__") == "NoReplacement": + return NO_REPLACEMENT # Delegate to the 'to_python' method if ignore_errors: try: diff --git a/lib/galaxy/tools/parameters/workflow_utils.py b/lib/galaxy/tools/parameters/workflow_utils.py index 73fcb688dfa1..178f08a773a6 100644 --- a/lib/galaxy/tools/parameters/workflow_utils.py +++ b/lib/galaxy/tools/parameters/workflow_utils.py @@ -1,6 +1,15 @@ from collections.abc import MutableMapping +class NoReplacement: + + def __str__(self): + return "NO_REPLACEMENT singleton" + + +NO_REPLACEMENT = NoReplacement() + + class workflow_building_modes: DISABLED = False ENABLED = True diff --git a/lib/galaxy/workflow/modules.py b/lib/galaxy/workflow/modules.py index 23602ffaef0f..0ed5c5113004 100644 --- a/lib/galaxy/workflow/modules.py +++ b/lib/galaxy/workflow/modules.py @@ -96,6 +96,8 @@ from galaxy.tools.parameters.workflow_utils import ( ConnectedValue, is_runtime_value, + NO_REPLACEMENT, + NoReplacement, runtime_to_json, workflow_building_modes, ) @@ -129,14 +131,6 @@ POSSIBLE_PARAMETER_TYPES: Tuple[INPUT_PARAMETER_TYPES] = get_args(INPUT_PARAMETER_TYPES) -class NoReplacement: - def __str__(self): - return "NO_REPLACEMENT singleton" - - -NO_REPLACEMENT = NoReplacement() - - class ConditionalStepWhen(BooleanToolParameter): pass @@ -954,7 +948,7 @@ class InputModule(WorkflowModule): def get_runtime_state(self): state = DefaultToolState() - state.inputs = dict(input=None) + state.inputs = dict(input=NO_REPLACEMENT) return state def get_all_inputs(self, data_only=False, connectable_only=False): @@ -966,7 +960,7 @@ def execute( invocation = invocation_step.workflow_invocation step = invocation_step.workflow_step input_value = step.state.inputs["input"] - if input_value is None: + if input_value is NO_REPLACEMENT: default_value = step.get_input_default_value(NO_REPLACEMENT) if default_value is not NO_REPLACEMENT: input_value = raw_to_galaxy(trans.app, trans.history, default_value) @@ -993,7 +987,7 @@ def execute( # everything should come in from the API and this can be eliminated. if not invocation.has_input_for_step(step.id): content = next(iter(step_outputs.values())) - if content: + if content and content is not NO_REPLACEMENT: invocation.add_input(content, step.id) progress.set_outputs_for_input(invocation_step, step_outputs) return None @@ -1582,7 +1576,7 @@ def _parameter_def_list_to_options(parameter_value): def get_runtime_state(self): state = DefaultToolState() - state.inputs = dict(input=None) + state.inputs = dict(input=NO_REPLACEMENT) return state def get_all_outputs(self, data_only=False): @@ -1609,7 +1603,7 @@ def execute( input_value = progress.inputs_by_step_id[step.id] else: input_value = step.state.inputs["input"] - if input_value is None: + if input_value is NO_REPLACEMENT: default_value = step.get_input_default_value(NO_REPLACEMENT) # TODO: look at parameter type and infer if value should be a dictionary # instead. Guessing only field parameter types in CWL branch would have @@ -2266,7 +2260,11 @@ def decode_runtime_state(self, step, runtime_state): ) def execute( - self, trans, progress: "WorkflowProgress", invocation_step, use_cached_job: bool = False + self, + trans, + progress: "WorkflowProgress", + invocation_step: "WorkflowInvocationStep", + use_cached_job: bool = False, ) -> Optional[bool]: invocation = invocation_step.workflow_invocation step = invocation_step.workflow_step diff --git a/lib/galaxy/workflow/refactor/execute.py b/lib/galaxy/workflow/refactor/execute.py index 264fa843d830..1e4c22c52bc7 100644 --- a/lib/galaxy/workflow/refactor/execute.py +++ b/lib/galaxy/workflow/refactor/execute.py @@ -9,6 +9,7 @@ from galaxy.tools.parameters.basic import contains_workflow_parameter from galaxy.tools.parameters.workflow_utils import ( ConnectedValue, + NO_REPLACEMENT, runtime_to_json, ) from .schema import ( @@ -41,10 +42,7 @@ UpgradeSubworkflowAction, UpgradeToolAction, ) -from ..modules import ( - InputParameterModule, - NO_REPLACEMENT, -) +from ..modules import InputParameterModule log = logging.getLogger(__name__) diff --git a/lib/galaxy/workflow/run.py b/lib/galaxy/workflow/run.py index d634f678999a..5210a336d4d9 100644 --- a/lib/galaxy/workflow/run.py +++ b/lib/galaxy/workflow/run.py @@ -1,5 +1,6 @@ import logging import uuid +from collections.abc import MutableMapping from typing import ( Any, Dict, @@ -37,6 +38,10 @@ WarningReason, ) from galaxy.tools.parameters.basic import raw_to_galaxy +from galaxy.tools.parameters.workflow_utils import ( + NO_REPLACEMENT, + NoReplacement, +) from galaxy.tools.parameters.wrapped import nested_key_to_path from galaxy.util import ExecutionTimer from galaxy.workflow import modules @@ -432,11 +437,11 @@ def remaining_steps( def replacement_for_input(self, trans, step: "WorkflowStep", input_dict: Dict[str, Any]): replacement: Union[ - modules.NoReplacement, + NoReplacement, model.DatasetCollectionInstance, List[model.DatasetCollectionInstance], HistoryItem, - ] = modules.NO_REPLACEMENT + ] = NO_REPLACEMENT prefixed_name = input_dict["name"] multiple = input_dict["multiple"] is_data = input_dict["input_type"] in ["dataset", "dataset_collection"] @@ -494,6 +499,8 @@ def replacement_for_connection(self, connection: "WorkflowStepConnection", is_da dependent_workflow_step_id=output_step_id, ) ) + if isinstance(replacement, MutableMapping) and replacement.get("__class__") == "NoReplacement": + return NO_REPLACEMENT if isinstance(replacement, model.HistoryDatasetCollectionAssociation): if not replacement.collection.populated: if not replacement.waiting_for_elements: @@ -574,19 +581,8 @@ def set_outputs_for_input( if self.inputs_by_step_id: step_id = step.id if step_id not in self.inputs_by_step_id and "output" not in outputs: - default_value = step.get_input_default_value(modules.NO_REPLACEMENT) - if default_value is not modules.NO_REPLACEMENT: - outputs["output"] = default_value - else: - log.error(f"{step.log_str()} not found in inputs_step_id {self.inputs_by_step_id}") - raise modules.FailWorkflowEvaluation( - why=InvocationFailureOutputNotFound( - reason=FailureReason.output_not_found, - workflow_step_id=invocation_step.workflow_step_id, - output_name="output", - dependent_workflow_step_id=invocation_step.workflow_step_id, - ) - ) + default_value = step.get_input_default_value(NO_REPLACEMENT) + outputs["output"] = default_value elif step_id in self.inputs_by_step_id: if self.inputs_by_step_id[step_id] is not None or "output" not in outputs: outputs["output"] = self.inputs_by_step_id[step_id] @@ -620,7 +616,7 @@ def set_step_outputs( # Add this non-data, non workflow-output output to the workflow outputs. # This is required for recovering the output in the next scheduling iteration, # and should be replaced with a WorkflowInvocationStepOutputValue ASAP. - if not workflow_outputs_by_name.get(output_name) and not output_object == modules.NO_REPLACEMENT: + if not workflow_outputs_by_name.get(output_name) and output_object is not NO_REPLACEMENT: workflow_output = model.WorkflowOutput(step, output_name=output_name) step.workflow_outputs.append(workflow_output) for workflow_output in step.workflow_outputs: @@ -645,6 +641,8 @@ def set_step_outputs( ) def _record_workflow_output(self, step: "WorkflowStep", workflow_output: "WorkflowOutput", output: Any) -> None: + if output is NO_REPLACEMENT: + output = {"__class__": "NoReplacement"} self.workflow_invocation.add_output(workflow_output, step, output) def mark_step_outputs_delayed(self, step: "WorkflowStep", why: Optional[str] = None) -> None: diff --git a/lib/galaxy/workflow/run_request.py b/lib/galaxy/workflow/run_request.py index baeb5492dd6d..94b47feeec31 100644 --- a/lib/galaxy/workflow/run_request.py +++ b/lib/galaxy/workflow/run_request.py @@ -30,6 +30,7 @@ from galaxy.tool_util.parameters import DataRequestUri from galaxy.tools.parameters.basic import ParameterValueError from galaxy.tools.parameters.meta import expand_workflow_inputs +from galaxy.tools.parameters.workflow_utils import NO_REPLACEMENT from galaxy.workflow.modules import WorkflowModuleInjector from galaxy.workflow.resources import get_resource_mapper_function @@ -599,7 +600,8 @@ def add_parameter(name: str, value: str, type: WorkflowRequestInputParameter.typ type=param_types.REPLACEMENT_PARAMETERS, ) for step_id, content in run_config.inputs.items(): - workflow_invocation.add_input(content, step_id) + if content is not NO_REPLACEMENT: + workflow_invocation.add_input(content, step_id) for step_id, param_dict in run_config.param_map.items(): add_parameter( name=str(step_id), diff --git a/lib/galaxy_test/workflow/default_values.gxwf-tests.yml b/lib/galaxy_test/workflow/default_values.gxwf-tests.yml index 92d0ad6e03c5..f5f7de07fee7 100644 --- a/lib/galaxy_test/workflow/default_values.gxwf-tests.yml +++ b/lib/galaxy_test/workflow/default_values.gxwf-tests.yml @@ -8,7 +8,8 @@ - that: has_text text: "1" - doc: | - Test that null is replaced with default value (follows https://www.commonwl.org/v1.2/Workflow.html#WorkflowInputParameter) + Test that explicit null is not replaced and fails + expect_failure: true job: required_int_with_default: type: raw diff --git a/lib/galaxy_test/workflow/default_values_optional.gxwf-tests.yml b/lib/galaxy_test/workflow/default_values_optional.gxwf-tests.yml index 7af1432006cb..9cd1d0abc328 100644 --- a/lib/galaxy_test/workflow/default_values_optional.gxwf-tests.yml +++ b/lib/galaxy_test/workflow/default_values_optional.gxwf-tests.yml @@ -8,7 +8,8 @@ - that: has_text text: "1" - doc: | - Test that null is replaced with default value (follows https://www.commonwl.org/v1.2/Workflow.html#WorkflowInputParameter) + Test that explicit null is not replaced and fails + expect_failure: true job: optional_int_with_default: type: raw diff --git a/test/unit/workflows/test_modules.py b/test/unit/workflows/test_modules.py index 8052c6b11950..d2e40b88138f 100644 --- a/test/unit/workflows/test_modules.py +++ b/test/unit/workflows/test_modules.py @@ -14,6 +14,7 @@ from galaxy import model from galaxy.managers.workflows import WorkflowContentsManager +from galaxy.tools.parameters.workflow_utils import NO_REPLACEMENT from galaxy.util import bunch from galaxy.workflow import modules from .workflow_support import ( @@ -57,7 +58,7 @@ def test_data_input_compute_runtime_state_default(): state, errors = module.compute_runtime_state(module.trans, module.test_step) assert not errors assert "input" in state.inputs - assert state.inputs["input"] is None + assert state.inputs["input"] is NO_REPLACEMENT def test_data_input_compute_runtime_state_args(): From 2d4270715cc2b94ee102c29f5fb816a9abc0d55b Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 18 Dec 2024 14:27:58 +0100 Subject: [PATCH 4/6] Fix various type hints --- lib/galaxy/model/__init__.py | 2 +- lib/galaxy/model/store/ro_crate_utils.py | 2 ++ lib/galaxy/workflow/modules.py | 5 +++-- lib/galaxy/workflow/run.py | 2 ++ 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index ead37ac4e1a7..14e72d3b1ead 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -8695,7 +8695,7 @@ class WorkflowInvocation(Base, UsesCreateAndUpdateTime, Dictifiable, Serializabl state: Mapped[Optional[str]] = mapped_column(TrimmedString(64), index=True) scheduler: Mapped[Optional[str]] = mapped_column(TrimmedString(255), index=True) handler: Mapped[Optional[str]] = mapped_column(TrimmedString(255), index=True) - uuid: Mapped[Optional[Union[UUID, str]]] = mapped_column(UUIDType()) + uuid: Mapped[Optional[Union[UUID]]] = mapped_column(UUIDType()) history_id: Mapped[Optional[int]] = mapped_column(ForeignKey("history.id"), index=True) history = relationship("History", back_populates="workflow_invocations") diff --git a/lib/galaxy/model/store/ro_crate_utils.py b/lib/galaxy/model/store/ro_crate_utils.py index f3592ac04ab9..b6b583a57a77 100644 --- a/lib/galaxy/model/store/ro_crate_utils.py +++ b/lib/galaxy/model/store/ro_crate_utils.py @@ -358,6 +358,7 @@ def _add_step_parameter_pv(self, step: WorkflowInvocationStep, crate: ROCrate): def _add_step_parameter_fp(self, step: WorkflowInvocationStep, crate: ROCrate): param_id = step.workflow_step.label + assert step.workflow_step.tool_inputs param_type = step.workflow_step.tool_inputs["parameter_type"] return crate.add( ContextEntity( @@ -375,6 +376,7 @@ def _add_step_parameter_fp(self, step: WorkflowInvocationStep, crate: ROCrate): def _add_step_tool_pv(self, step: WorkflowInvocationStep, tool_input: str, crate: ROCrate): param_id = tool_input + assert step.workflow_step.tool_inputs return crate.add( ContextEntity( crate, diff --git a/lib/galaxy/workflow/modules.py b/lib/galaxy/workflow/modules.py index 0ed5c5113004..48498adf78ad 100644 --- a/lib/galaxy/workflow/modules.py +++ b/lib/galaxy/workflow/modules.py @@ -463,7 +463,7 @@ def update_value(input, context, prefixed_name, **kwargs): return state, step_errors - def encode_runtime_state(self, step, runtime_state): + def encode_runtime_state(self, step, runtime_state: DefaultToolState): """Takes the computed runtime state and serializes it during run request creation.""" return runtime_state.encode(Bunch(inputs=self.get_runtime_inputs(step)), self.trans.app) @@ -2274,6 +2274,7 @@ def execute( message = f"Specified tool [{tool.id}] in step {step.order_index + 1} is not workflow-compatible." raise exceptions.MessageException(message) tool_state = step.state + assert tool_state is not None tool_inputs = tool.inputs.copy() # Not strictly needed - but keep Tool state clean by stripping runtime # metadata parameters from it. @@ -2402,7 +2403,7 @@ def callback(input, prefixed_name: str, **kwargs): mapping_params=mapping_params, history=invocation.history, collection_info=collection_info, - workflow_invocation_uuid=invocation.uuid.hex, + workflow_invocation_uuid=invocation.uuid.hex if invocation.uuid else None, invocation_step=invocation_step, max_num_jobs=max_num_jobs, validate_outputs=validate_outputs, diff --git a/lib/galaxy/workflow/run.py b/lib/galaxy/workflow/run.py index 5210a336d4d9..0e4217b16b77 100644 --- a/lib/galaxy/workflow/run.py +++ b/lib/galaxy/workflow/run.py @@ -339,6 +339,7 @@ def __check_implicitly_dependent_step(self, output_id: int, step_id: int): ) def _invoke_step(self, invocation_step: WorkflowInvocationStep) -> Optional[bool]: + assert invocation_step.workflow_step.module incomplete_or_none = invocation_step.workflow_step.module.execute( self.trans, self.progress, @@ -735,6 +736,7 @@ def raw_to_galaxy(self, value: dict): return raw_to_galaxy(self.module_injector.trans.app, self.module_injector.trans.history, value) def _recover_mapping(self, step_invocation: WorkflowInvocationStep) -> None: + assert step_invocation.workflow_step.module try: step_invocation.workflow_step.module.recover_mapping(step_invocation, self) except modules.DelayedWorkflowEvaluation as de: From 133e7f5c70b0332b245ee6caee5bfbd0cbdc47d1 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 18 Dec 2024 20:23:46 +0100 Subject: [PATCH 5/6] Fix `test_run_with_optional_data_unspecified_survives_delayed_step` --- lib/galaxy/tools/parameters/__init__.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/galaxy/tools/parameters/__init__.py b/lib/galaxy/tools/parameters/__init__.py index 04c968910b45..7281067f9a47 100644 --- a/lib/galaxy/tools/parameters/__init__.py +++ b/lib/galaxy/tools/parameters/__init__.py @@ -175,6 +175,17 @@ def callback_helper(input, input_values, name_prefix, label_prefix, parent_prefi if input.name not in input_values: args["error"] = f"No value found for '{args.get('prefixed_label')}'." new_value = callback(**args) + + # is this good enough ? feels very ugh + if new_value == [no_replacement_value]: + # Single unspecified value in multiple="true" input with a single null input, pretend it's a singular value + new_value = no_replacement_value + if isinstance(new_value, list): + # Maybe mixed input, I guess tool defaults don't really make sense here ? + # Would e.g. be default dataset in multiple="true" input, you wouldn't expect the default to be inserted + # if other inputs are connected and provided. + new_value = [item if not item == no_replacement_value else None for item in new_value] + if no_replacement_value is REPLACE_ON_TRUTHY: replace = bool(new_value) else: From 7a7836e70ae168ffee9451c01142a27a2d19b420 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 19 Dec 2024 12:30:46 +0100 Subject: [PATCH 6/6] Don't replace default values too early We should probably only do this once we're ready to execute the step, not when injecting modules and building the tool state. --- lib/galaxy/model/__init__.py | 5 +++-- lib/galaxy/tools/parameters/__init__.py | 5 +---- lib/galaxy/workflow/modules.py | 25 ++++++++++++++++--------- lib/galaxy/workflow/run.py | 16 ++++++++++------ 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 14e72d3b1ead..387d9cc409f3 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -8206,17 +8206,18 @@ def setup_inputs_by_name(self): # Ensure input_connections has already been set. # Make connection information available on each step by input name. - inputs_by_name = {} + inputs_by_name: Dict[str, Any] = {} for step_input in self.inputs: input_name = step_input.name assert input_name not in inputs_by_name inputs_by_name[input_name] = step_input self._inputs_by_name = inputs_by_name + return inputs_by_name @property def inputs_by_name(self): if self._inputs_by_name is None: - self.setup_inputs_by_name() + return self.setup_inputs_by_name() return self._inputs_by_name def get_input(self, input_name): diff --git a/lib/galaxy/tools/parameters/__init__.py b/lib/galaxy/tools/parameters/__init__.py index 7281067f9a47..fc1ca3ab23a7 100644 --- a/lib/galaxy/tools/parameters/__init__.py +++ b/lib/galaxy/tools/parameters/__init__.py @@ -195,7 +195,7 @@ def callback_helper(input, input_values, name_prefix, label_prefix, parent_prefi elif replace_optional_connections: # Only used in workflow context has_default = hasattr(input, "value") - if new_value is value is NO_REPLACEMENT: + if new_value is value is NO_REPLACEMENT or is_runtime_value(value): # NO_REPLACEMENT means value was connected but left unspecified if has_default: # Use default if we have one @@ -206,9 +206,6 @@ def callback_helper(input, input_values, name_prefix, label_prefix, parent_prefi # We might want to raise an exception here, instead of depending on a tool parameter value error. input_values[input.name] = None - elif is_runtime_value(value) and has_default: - input_values[input.name] = input.value - def get_current_case(input, input_values): test_parameter = input.test_param test_parameter_name = test_parameter.name diff --git a/lib/galaxy/workflow/modules.py b/lib/galaxy/workflow/modules.py index 48498adf78ad..02eafbc02cf7 100644 --- a/lib/galaxy/workflow/modules.py +++ b/lib/galaxy/workflow/modules.py @@ -410,14 +410,11 @@ def get_runtime_inputs(self, step, connections: Optional[Iterable[WorkflowStepCo """ return {} - def compute_runtime_state(self, trans, step=None, step_updates=None): + def compute_runtime_state(self, trans, step=None, step_updates=None, replace_default_values=False): """Determine the runtime state (potentially different from self.state which describes configuration state). This (again unlike self.state) is currently always a `DefaultToolState` object. - If `step` is not `None`, it will be used to search for default values - defined in workflow input steps. - If `step_updates` is `None`, this is likely for rendering the run form for instance and no runtime properties are available and state must be solely determined by the default runtime state described by the step. @@ -426,6 +423,8 @@ def compute_runtime_state(self, trans, step=None, step_updates=None): supplied by the workflow runner. """ state = self.get_runtime_state() + if replace_default_values and step: + state.inputs = step.state.inputs step_errors = {} if step is not None: @@ -435,8 +434,11 @@ def update_value(input, context, prefixed_name, **kwargs): if step_input is None: return NO_REPLACEMENT - if step_input.default_value_set: - return step_input.default_value + if replace_default_values and step_input.default_value_set: + input_value = step_input.default_value + if isinstance(input, BaseDataToolParameter): + input_value = raw_to_galaxy(trans.app, trans.history, input_value) + return input_value return NO_REPLACEMENT @@ -2227,13 +2229,14 @@ def get_runtime_state(self): def get_runtime_inputs(self, step, connections: Optional[Iterable[WorkflowStepConnection]] = None): return self.get_inputs() - def compute_runtime_state(self, trans, step=None, step_updates=None): + def compute_runtime_state(self, trans, step=None, step_updates=None, replace_default_values=False): # Warning: This method destructively modifies existing step state. if self.tool: step_errors = {} state = self.state - self.runtime_post_job_actions = {} - state, step_errors = super().compute_runtime_state(trans, step, step_updates) + state, step_errors = super().compute_runtime_state( + trans, step, step_updates, replace_default_values=replace_default_values + ) if step_updates: self.runtime_post_job_actions = step_updates.get(RUNTIME_POST_JOB_ACTIONS_KEY, {}) step_metadata_runtime_state = self.__step_meta_runtime_state() @@ -2273,6 +2276,10 @@ def execute( # TODO: why do we even create an invocation, seems like something we could check on submit? message = f"Specified tool [{tool.id}] in step {step.order_index + 1} is not workflow-compatible." raise exceptions.MessageException(message) + self.state, _ = self.compute_runtime_state( + trans, step, step_updates=progress.param_map.get(step.id), replace_default_values=True + ) + step.state = self.state tool_state = step.state assert tool_state is not None tool_inputs = tool.inputs.copy() diff --git a/lib/galaxy/workflow/run.py b/lib/galaxy/workflow/run.py index 0e4217b16b77..6d52a7f6a0cf 100644 --- a/lib/galaxy/workflow/run.py +++ b/lib/galaxy/workflow/run.py @@ -39,6 +39,7 @@ ) from galaxy.tools.parameters.basic import raw_to_galaxy from galaxy.tools.parameters.workflow_utils import ( + is_runtime_value, NO_REPLACEMENT, NoReplacement, ) @@ -462,15 +463,18 @@ def replacement_for_input(self, trans, step: "WorkflowStep", input_dict: Dict[st replacement = temp else: replacement = self.replacement_for_connection(connection[0], is_data=is_data) - elif step.state and (state_input := get_path(step.state.inputs, nested_key_to_path(prefixed_name), None)): + elif ( + step.state + and (state_input := get_path(step.state.inputs, nested_key_to_path(prefixed_name), None)) + and not is_runtime_value(state_input) + ): # workflow submitted with step parameters populates state directly # via populate_module_and_state replacement = state_input - else: - for step_input in step.inputs: - if step_input.name == prefixed_name and step_input.default_value_set: - if is_data: - replacement = raw_to_galaxy(trans.app, trans.history, step_input.default_value) + elif (step_input := step.inputs_by_name.get(prefixed_name)) and step_input.default_value_set: + replacement = step_input.default_value + if is_data: + replacement = raw_to_galaxy(trans.app, trans.history, step_input.default_value) return replacement def replacement_for_connection(self, connection: "WorkflowStepConnection", is_data: bool = True):