Merge pull request #23084 from guerler/toolrequests.002

Align async tool requests with sync execution

Author: jmchilton

lib/galaxy/managers/hdas.py

@@ -184,6 +184,7 @@ def materialize(
             file_sources=self.app.file_sources,
             sa_session=session,
             user_context=user_context,
+            datatypes_registry=self.app.datatypes_registry,
         )
         if request.source == DatasetSourceType.hda:
             dataset_instance: HistoryDatasetAssociation | LibraryDatasetDatasetAssociation = self.get_accessible(

lib/galaxy/model/deferred.py

@@ -9,8 +9,10 @@
 from sqlalchemy.orm import Session
 from sqlalchemy.orm.exc import DetachedInstanceError
 
+from galaxy.datatypes.registry import Registry
 from galaxy.datatypes.sniff import (
     convert_function,
+    guess_ext,
     stream_url_to_file,
 )
 from galaxy.exceptions import ObjectAttributeInvalidException
@@ -76,6 +78,7 @@ class DatasetInstanceMaterializer:
     def __init__(
         self,
         attached: bool,
+        datatypes_registry: Registry,
         object_store_populator: ObjectStorePopulator | None = None,
         transient_path_mapper: TransientPathMapper | None = None,
         file_sources: ConfiguredFileSources | None = None,
@@ -91,13 +94,17 @@ def __init__(
         ``user_context`` is forwarded to file source operations so that access
         controls (``requires_roles`` / ``requires_groups``) are enforced when
         materializing from ``gxfiles://`` URIs.
+
+        ``datatypes_registry`` enables content sniffing for deferred datasets whose
+        extension is ``"auto"``.
         """
         self._attached = attached
         self._transient_path_mapper = transient_path_mapper
         self._object_store_populator = object_store_populator
         self._file_sources = file_sources
         self._sa_session = sa_session
         self._user_context = user_context
+        self._datatypes_registry = datatypes_registry
         self._previously_materialized: dict[int, HistoryDatasetAssociation] = {}
 
     def ensure_materialized(
@@ -189,6 +196,7 @@ def ensure_materialized(
             if not replacement_dataset:
                 try:
                     path = self._stream_source(target_source, dataset_instance.datatype, materialized_dataset)
+                    self._sniff_deferred_extension(path, dataset_instance)
                     object_store.update_from_file(materialized_dataset, file_name=path)
                     materialized_dataset.set_size()
                 except Exception as e:
@@ -201,6 +209,7 @@ def ensure_materialized(
             # TODO: take into account transform and ensure we are and are not modifying the file as appropriate.
             try:
                 path = self._stream_source(target_source, dataset_instance.datatype, materialized_dataset)
+                self._sniff_deferred_extension(path, dataset_instance)
                 shutil.move(path, transient_paths.external_filename)
                 materialized_dataset.external_filename = transient_paths.external_filename
             except Exception as e:
@@ -256,6 +265,23 @@ def ensure_materialized(
         self._previously_materialized[dataset_instance.id] = materialized_dataset_instance
         return materialized_dataset_instance
 
+    def _sniff_deferred_extension(
+        self,
+        path: str,
+        dataset_instance: HistoryDatasetAssociation | LibraryDatasetDatasetAssociation,
+    ) -> None:
+        """Resolve ``extension="auto"`` once the deferred source is streamed to ``path``.
+
+        A deferred fetch/upload with no explicit ``ext`` is registered as ``auto`` because
+        the content is not available to sniff at request time. ``path`` is the first point
+        the bytes exist locally, so we sniff here while materializing.
+        """
+        if dataset_instance.extension != "auto":
+            return
+        sniffed = guess_ext(path, self._datatypes_registry.sniff_order)
+        if sniffed and sniffed != "auto":
+            dataset_instance.extension = sniffed
+
     def _stream_source(self, target_source: DatasetSource, datatype, dataset: Dataset) -> str:
         source_uri = target_source.source_uri
         if source_uri is None:
@@ -387,6 +413,7 @@ def _materialize_collection_element(
 
 def materializer_factory(
     attached: bool,
+    datatypes_registry: Registry,
     object_store: ObjectStore | None = None,
     object_store_populator: ObjectStorePopulator | None = None,
     transient_path_mapper: TransientPathMapper | None = None,
@@ -401,6 +428,7 @@ def materializer_factory(
         transient_path_mapper = SimpleTransientPathMapper(transient_directory)
     return DatasetInstanceMaterializer(
         attached,
+        datatypes_registry,
         object_store_populator=object_store_populator,
         transient_path_mapper=transient_path_mapper,
         file_sources=file_sources,

lib/galaxy/tool_util/parameters/case.py

@@ -96,18 +96,23 @@ def legacy_from_string(parameter: ToolParameterT, value: Any | None, warnings: l
     """
     result_value: Any = value
     if isinstance(value, str):
-        if isinstance(parameter, (IntegerParameterModel,)):
-            if WARN_ON_UNTYPED_XML_STRINGS:
-                warnings.append(
-                    f"Implicitly converted {parameter.name} to an integer from a string value, please use 'value_json' to define this test input parameter value instead."
-                )
-            result_value = int(value)
-        elif isinstance(parameter, (FloatParameterModel,)):
-            if WARN_ON_UNTYPED_XML_STRINGS:
-                warnings.append(
-                    f"Implicitly converted {parameter.name} to a floating point number from a string value, please use 'value_json' to define this test input parameter value instead."
-                )
-            result_value = float(value)
+        if isinstance(parameter, (IntegerParameterModel, FloatParameterModel)):
+            # ``value=""`` on a numeric param is the legacy "not set" convention; emit None
+            # rather than raising on int("")/float("").
+            if value == "":
+                result_value = None
+            elif isinstance(parameter, IntegerParameterModel):
+                if WARN_ON_UNTYPED_XML_STRINGS:
+                    warnings.append(
+                        f"Implicitly converted {parameter.name} to an integer from a string value, please use 'value_json' to define this test input parameter value instead."
+                    )
+                result_value = int(value)
+            else:
+                if WARN_ON_UNTYPED_XML_STRINGS:
+                    warnings.append(
+                        f"Implicitly converted {parameter.name} to a floating point number from a string value, please use 'value_json' to define this test input parameter value instead."
+                    )
+                result_value = float(value)
         elif isinstance(parameter, (BooleanParameterModel,)):
             if WARN_ON_UNTYPED_XML_STRINGS:
                 warnings.append(

lib/galaxy/tool_util/parameters/convert.py

@@ -14,6 +14,7 @@
 
 from galaxy.tool_util_models.parameters import (
     BooleanParameterModel,
+    ColorParameterModel,
     ConditionalParameterModel,
     ConditionalWhen,
     create_job_runtime_model,
@@ -491,7 +492,7 @@ def _fill_default_for(tool_state: dict[str, Any], parameter: ToolParameterT) ->
             # see test_tools.py -> expression_null_handling_boolean or test cases for gx_boolean_optional.xml
             tool_state[parameter_name] = parameter.value or False
 
-    if isinstance(parameter, (IntegerParameterModel, FloatParameterModel, HiddenParameterModel)):
+    if isinstance(parameter, (IntegerParameterModel, FloatParameterModel, HiddenParameterModel, ColorParameterModel)):
         if parameter_name not in tool_state:
             tool_state[parameter_name] = parameter.value
     elif isinstance(parameter, GenomeBuildParameterModel):

lib/galaxy/tool_util/parameters/factory.py

@@ -198,11 +198,16 @@ def _from_input_source_galaxy(input_source: InputSource, profile: float) -> Tool
             )
         elif param_type == "color":
             optional = input_source.parse_optional()
+            color_value: str | None = get_color_value(input_source)
+            # A color default must be a valid color or None. The legacy ``value=""``
+            # on an optional color means "unset".
+            if optional and color_value == "":
+                color_value = None
             return ColorParameterModel(
                 type="color",
                 name=input_source.parse_name(),
                 optional=optional,
-                value=get_color_value(input_source),
+                value=color_value,
                 **_common_param_kwargs(input_source),
             )
         elif param_type == "rules":

lib/galaxy/tool_util_models/parameters.py

@@ -327,6 +327,11 @@ def _json_schema_extra_for_validators(validators: Sequence[VT]) -> dict[str, Any
             # Python re.match anchors at start; JSON Schema pattern does not
             if not pattern.startswith("^"):
                 pattern = "^" + pattern
+            # Python ``re`` (used by pydantic's ``pattern``) doesn't recognise POSIX
+            # character classes; defer such patterns to ``statically_validate`` which uses
+            # the ``regex`` module (seurat_plot).
+            if "[:" in pattern and ":]" in pattern:
+                continue
             extra["pattern"] = pattern
             break
     for v in validators:

lib/galaxy/tools/evaluation.py

@@ -309,6 +309,7 @@ def _materialize_objects(
             transient_directory=transient_directory,
             file_sources=self.app.file_sources,
             user_context=user_context,
+            datatypes_registry=self.app.datatypes_registry,
         )
         for key, value in deferred_objects.items():
             if isinstance(value, model.DatasetInstance):

packages/tool_util/pyproject.toml

@@ -22,6 +22,7 @@ dependencies = [
     "packaging",
     "pydantic>=2.7.4",
     "PyYAML",
+    "regex",
     "requests",
     "sortedcontainers",
     "typing-extensions",

packages/tool_util_models/pyproject.toml

@@ -15,6 +15,7 @@ authors = [
 dependencies = [
     "pydantic>=2.7.4",
     "pydantic-extra-types",
+    "regex",
     "typing-extensions",
 ]
 classifiers = [

test/functional/tools/async_posix_regex_validator.xml

@@ -0,0 +1,52 @@
+<tool id="async_posix_regex_validator" name="async_posix_regex_validator" version="1.0.0">
+    <description>async regression: POSIX character-class regex validators are skipped by pydantic and deferred to statically_validate</description>
+    <!--
+      Models seurat_plot test 2. The tool declares a text param with a regex
+      validator that uses POSIX character classes:
+
+          <validator type="regex">^[\w[:punct:]]+$</validator>
+
+      Sync validates via the ``regex`` module, which recognises POSIX classes and
+      accepts values like ``X16cell`` and ``midblast.9``.
+
+      Pydantic's ``pattern`` extra compiles through Python's ``re``, which does
+      NOT recognise POSIX classes and instead treats ``[:punct:]`` as a literal
+      character class of ``:``, ``p``, ``u``, ``n``, ``c``, ``t``. The resulting
+      pattern rejects perfectly legitimate values and the async request build
+      fails at pydantic time.
+
+      The framework fix: skip emitting the ``pattern`` extra when the validator
+      contains a POSIX character class, and defer validation to
+      ``statically_validate`` (which uses the ``regex`` module). Both sync and
+      async now accept the same values on this tool.
+    -->
+    <command><![CDATA[
+echo '$cell1' > '$out_file1'
+    ]]></command>
+    <inputs>
+        <param name="cell1" type="text" value="cell 1">
+            <validator type="regex" message="letters, numbers, or punctuation">^[\w[:punct:]]+$</validator>
+        </param>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" />
+    </outputs>
+    <tests>
+        <test>
+            <param name="cell1" value="X16cell" />
+            <output name="out_file1">
+                <assert_contents>
+                    <has_text text="X16cell" />
+                </assert_contents>
+            </output>
+        </test>
+        <test>
+            <param name="cell1" value="midblast.9" />
+            <output name="out_file1">
+                <assert_contents>
+                    <has_text text="midblast.9" />
+                </assert_contents>
+            </output>
+        </test>
+    </tests>
+</tool>