From 1d173a280f72a1394d411489dd0c07f51e663f82 Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Mon, 5 Oct 2026 15:53:37 +0200 Subject: [PATCH] fix: make computing feature lookup deterministic History replay ran the built-in feature when a plugin feature shared its name. Assisted-by: Claude Opus 5.5 --- datalab/gui/processor/base.py | 98 ++++++++++++++----- datalab/gui/processor/image.py | 5 - datalab/gui/processor/signal.py | 20 ---- .../common/feature_identity_unit_test.py | 97 ++++++++++++++++++ .../common/history_model_unit_test.py | 1 + doc/release_notes/release_1.04.md | 6 ++ 6 files changed, 175 insertions(+), 52 deletions(-) create mode 100644 datalab/tests/features/common/feature_identity_unit_test.py diff --git a/datalab/gui/processor/base.py b/datalab/gui/processor/base.py index c069f4c9b..7edda1f8e 100644 --- a/datalab/gui/processor/base.py +++ b/datalab/gui/processor/base.py @@ -860,6 +860,15 @@ def name(self) -> str: ) return self.function.__name__ + @property + def feature_id(self) -> str: + """Return the identifier persisted in processing metadata and History. + + Built-in feature identifiers are computation function names, which must be + unique within a processor. + """ + return self.name + @property def action_title(self) -> str: """Return the action title of the computing feature.""" @@ -1769,7 +1778,7 @@ def _compute_1_to_1_subroutine( # Store processing metadata for interactive re-processing pp = ProcessingParameters( - func_name=name, + func_name=self.get_feature_id(func), pattern="1-to-1", param=param, source_uuid=get_uuid(obj), @@ -1915,7 +1924,7 @@ def compute_1_to_1( draft = copy.deepcopy(param) preview_results = [] - feature = self.computing_registry.get(func.__name__) + feature = self.computing_registry.get(func) allowed = preview_enabled and (feature is None or feature.preview_enabled) if not edit_processing_parameters( draft, @@ -1948,7 +1957,10 @@ def compute_1_to_1( self.PARAM_DEFAULTS[type(param).__name__] = copy.deepcopy(param) plugin_origin = self._get_plugin_origin_for(func) pp = build_processing_parameters( - func.__name__, "1-to-1", param=param, plugin_origin=plugin_origin + self.get_feature_id(func), + "1-to-1", + param=param, + plugin_origin=plugin_origin, ) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or func.__name__, @@ -2004,14 +2016,15 @@ def compute_multiple_1_to_1( return if len(funcs) != len(params): raise ValueError("Number of functions must match number of parameters") + func_names = [self.get_feature_id(func) for func in funcs] pp = build_processing_parameters( - funcs[0].__name__ if funcs else "", "multiple-1-to-1" + func_names[0] if func_names else "", "multiple-1-to-1" ) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or "compute_multiple_1_to_1", pp, panel_str=self.panel.PANEL_STR_ID, - func_names=[f.__name__ for f in funcs], + func_names=func_names, params=params if any(p is not None for p in params) else None, plugin_origin=(self._get_plugin_origin_for(funcs[0]) if funcs else None), ) @@ -2052,7 +2065,7 @@ def compute_1_to_n( group = gds.DataSetGroup(params, title=_("Parameters")) if not group.edit(parent=self.mainwindow): return - pp = build_processing_parameters(func.__name__, "1-to-n") + pp = build_processing_parameters(self.get_feature_id(func), "1-to-n") action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or func.__name__, pp, @@ -2119,7 +2132,8 @@ def compute_1_to_0( return None current_obj = self.panel.objview.get_current_object() title = func.__name__ if title is None else title - pp_history = build_processing_parameters(func.__name__, "1-to-0", param=param) + feature_id = self.get_feature_id(func) + pp_history = build_processing_parameters(feature_id, "1-to-0", param=param) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title, pp_history, @@ -2160,7 +2174,7 @@ def persist_result(result=result, obj=obj) -> bool: adapter = create_adapter(result) adapter.add_to(obj, param) pp = ProcessingParameters( - func_name=func.__name__, + func_name=feature_id, pattern="1-to-0", param=param, source_uuid=get_uuid(obj), @@ -2251,8 +2265,9 @@ def compute_n_to_1( objmodel = self.panel.objmodel pairwise = is_pairwise_mode() if pairwise is None else pairwise name = func.__name__ + feature_id = self.get_feature_id(func) - pp_history = build_processing_parameters(name, "n-to-1", param=param) + pp_history = build_processing_parameters(feature_id, "n-to-1", param=param) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or name, pp_history, @@ -2342,7 +2357,7 @@ def compute_n_to_1( # Store lightweight processing metadata (non-interactive) proc_params = ProcessingParameters( - func_name=name, + func_name=feature_id, pattern="n-to-1", param=param, source_uuids=[get_uuid(obj) for obj in src_objs_pair], @@ -2434,7 +2449,7 @@ def compute_n_to_1( # Store lightweight processing metadata (non-interactive) proc_params = ProcessingParameters( - func_name=name, + func_name=feature_id, pattern="n-to-1", param=param, source_uuids=[get_uuid(obj) for obj in src_obj_list], @@ -2521,6 +2536,7 @@ def compute_2_to_1( # pylint: disable=too-many-return-statements objmodel = self.panel.objmodel pairwise = is_pairwise_mode() if pairwise is None else pairwise name = func.__name__ + feature_id = self.get_feature_id(func) if obj2 is None: objs2 = [] @@ -2556,9 +2572,7 @@ def compute_2_to_1( # pylint: disable=too-many-return-statements if objs2 is None: return - pp_history = build_processing_parameters( - func.__name__, "2-to-1", param=param - ) + pp_history = build_processing_parameters(feature_id, "2-to-1", param=param) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or func.__name__, pp_history, @@ -2637,7 +2651,7 @@ def compute_2_to_1( # pylint: disable=too-many-return-statements # Store lightweight processing metadata (non-interactive) proc_params = ProcessingParameters( - func_name=name, + func_name=feature_id, pattern="2-to-1", param=param, source_uuids=[ @@ -2673,9 +2687,7 @@ def compute_2_to_1( # pylint: disable=too-many-return-statements return obj2 = objs2[0] - pp_history = build_processing_parameters( - func.__name__, "2-to-1", param=param - ) + pp_history = build_processing_parameters(feature_id, "2-to-1", param=param) action = self.mainwindow.historypanel.add_compute_entry_from_pp( title or func.__name__, pp_history, @@ -2768,7 +2780,7 @@ def compute_2_to_1( # pylint: disable=too-many-return-statements # Store lightweight processing metadata (non-interactive) proc_params = ProcessingParameters( - func_name=name, + func_name=feature_id, pattern="2-to-1", param=param, source_uuids=[ @@ -3009,6 +3021,19 @@ def _get_plugin_origin_for(self, func: Callable) -> dict[str, Any] | None: return feature.plugin_origin return _detect_plugin_origin(func) + def get_feature_id(self, func: Callable) -> str: + """Return the identifier persisted for ``func``. + + Args: + func: Computation function. + + Returns: + Identifier of the registered feature, or the function name if ``func`` + is not registered. + """ + feature = self.computing_registry.get(func) + return func.__name__ if feature is None else feature.feature_id + def get_feature( self, function_or_name: Callable | str, @@ -3017,10 +3042,15 @@ def get_feature( ) -> ComputingFeature: """Get a computing feature by name or function. + When several features share the same identifier, built-in features are + preferred unless ``plugin_origin`` is given, in which case features from + the same plugin module are preferred. + Args: - function_or_name: Name of the feature or the function itself. - plugin_origin: Optional plugin origin descriptor used to enrich the - :class:`FeatureNotFoundError` raised when the feature is unknown. + function_or_name: Identifier of the feature or the function itself. + plugin_origin: Optional plugin origin descriptor used to select the + plugin feature and to enrich the :class:`FeatureNotFoundError` raised + when the feature is unknown. paramclass_name: Optional name of the required parameter class, also used to enrich the error message. @@ -3032,12 +3062,26 @@ def get_feature( exception subclasses :class:`ValueError` to preserve backward compatibility with existing callers. """ - try: + if isinstance(function_or_name, str): + candidates = [ + feature + for feature in self.computing_registry.values() + if feature.feature_id == function_or_name + ] + if plugin_origin is None: + preferred = [f for f in candidates if f.plugin_origin is None] + else: + module = plugin_origin.get("module") + preferred = [ + f + for f in candidates + if f.plugin_origin is not None + and f.plugin_origin.get("module") == module + ] + if preferred or candidates: + return (preferred or candidates)[0] + elif function_or_name in self.computing_registry: return self.computing_registry[function_or_name] - except KeyError: - for _func, feature in self.computing_registry.items(): - if feature.name == function_or_name: - return feature raise FeatureNotFoundError( str(function_or_name), plugin_origin=plugin_origin, diff --git a/datalab/gui/processor/image.py b/datalab/gui/processor/image.py index 748529435..201207458 100644 --- a/datalab/gui/processor/image.py +++ b/datalab/gui/processor/image.py @@ -269,11 +269,6 @@ def register_processing(self) -> None: " • z' = a0 + a1*z + a2*z^2 + ..." ), ) - self.register_1_to_1( - sipi.transpose, - _("Swap X/Y axes"), - icon_name="swap_x_y.svg", - ) # Level adjustment self.register_1_to_1( sipi.normalize, diff --git a/datalab/gui/processor/signal.py b/datalab/gui/processor/signal.py index b0d2e4dbb..e56e6018c 100644 --- a/datalab/gui/processor/signal.py +++ b/datalab/gui/processor/signal.py @@ -544,26 +544,6 @@ def register_processing(self) -> None: _("Hadamard variance"), paramclass=sigima.params.AllanVarianceParam, ) - self.register_1_to_1( - sips.modified_allan_variance, - _("Modified Allan variance"), - paramclass=sigima.params.AllanVarianceParam, - ) - self.register_1_to_1( - sips.hadamard_variance, - _("Hadamard variance"), - paramclass=sigima.params.AllanVarianceParam, - ) - self.register_1_to_1( - sips.modified_allan_variance, - _("Modified Allan variance"), - paramclass=sigima.params.AllanVarianceParam, - ) - self.register_1_to_1( - sips.hadamard_variance, - _("Hadamard variance"), - paramclass=sigima.params.AllanVarianceParam, - ) self.register_1_to_1( sips.total_variance, _("Total variance"), diff --git a/datalab/tests/features/common/feature_identity_unit_test.py b/datalab/tests/features/common/feature_identity_unit_test.py new file mode 100644 index 000000000..82404eb1f --- /dev/null +++ b/datalab/tests/features/common/feature_identity_unit_test.py @@ -0,0 +1,97 @@ +# Copyright (c) DataLab Platform Developers, BSD 3-Clause license, see LICENSE file. + +"""Computing feature identity unit tests.""" + +from __future__ import annotations + +import dataclasses +from collections import Counter + +import numpy as np +import sigima.params +import sigima.proc.signal as sips +from sigima.objects import SignalObj, create_signal +from sigima.proc.decorator import computation_function + +from datalab.gui.processor.base import BaseProcessor, ComputingFeature +from datalab.tests import datalab_test_app_context + +PLUGIN_ORIGIN = { + "plugin_class": "IdentityTestPlugin", + "module": "datalab_identity_test_plugin", + "directory": None, + "version": None, +} + + +@computation_function() +def normalize(src: SignalObj) -> SignalObj: + """Plugin computation sharing a built-in feature identifier.""" + return src.copy() + + +def test_builtin_feature_ids_are_unique(monkeypatch) -> None: + """Built-in registrations never reuse an identifier within a panel.""" + registrations: list[tuple[str, str]] = [] + add_feature = BaseProcessor.add_feature + + def record_feature(self: BaseProcessor, feature: ComputingFeature) -> None: + add_feature(self, feature) + if feature.plugin_origin is None: + registrations.append((self.panel.PANEL_STR_ID, feature.feature_id)) + + monkeypatch.setattr(BaseProcessor, "add_feature", record_feature) + with datalab_test_app_context(console=False): + pass + assert registrations + counts = Counter(registrations) + assert [key for key, count in counts.items() if count > 1] == [] + + +def test_get_feature_prefers_builtin_or_matching_plugin() -> None: + """Identifier lookup does not depend on registration order.""" + with datalab_test_app_context(console=False) as win: + processor = win.signalpanel.processor + plugin_feature = ComputingFeature( + "1_to_1", + function=normalize, + title="Plugin normalization", + plugin_origin=dict(PLUGIN_ORIGIN), + ) + processor.computing_registry = { + normalize: plugin_feature, + **processor.computing_registry, + } + + assert processor.get_feature("normalize").function is sips.normalize + feature = processor.get_feature("normalize", plugin_origin=PLUGIN_ORIGIN) + assert feature is plugin_feature + assert processor.get_feature(normalize) is plugin_feature + assert processor.get_feature_id(normalize) == "normalize" + + +def test_compute_1_to_1_honours_registered_preview_flag(monkeypatch) -> None: + """Direct 1-to-1 calls respect the registered preview capability.""" + allowed_values: list[bool] = [] + + def cancel_edit(draft, function, sources, parent, allowed, results, **kwargs): + allowed_values.append(allowed) + return False + + monkeypatch.setattr( + "datalab.widgets.processingpreview.edit_processing_parameters", cancel_edit + ) + with datalab_test_app_context(console=False) as win: + panel = win.signalpanel + processor = panel.processor + feature = processor.get_feature(sips.moving_average) + processor.add_feature(dataclasses.replace(feature, preview_enabled=False)) + source = create_signal("Source", np.arange(10.0), np.arange(10.0)) + panel.add_object(source) + panel.objview.select_objects([source]) + processor.compute_1_to_1( + sips.moving_average, + paramclass=sigima.params.MovingAverageParam, + edit=True, + ) + assert allowed_values == [False] diff --git a/datalab/tests/features/common/history_model_unit_test.py b/datalab/tests/features/common/history_model_unit_test.py index 6fcbd5d66..0fb25a3f6 100644 --- a/datalab/tests/features/common/history_model_unit_test.py +++ b/datalab/tests/features/common/history_model_unit_test.py @@ -252,6 +252,7 @@ def test_compute_n_to_1_uses_provided_history_title() -> None: ), mainwindow=SimpleNamespace(historypanel=history_panel), _get_plugin_origin_for=Mock(return_value=None), + get_feature_id=lambda func: func.__name__, ) def average(_objects: list[object]) -> None: diff --git a/doc/release_notes/release_1.04.md b/doc/release_notes/release_1.04.md index c446abbd6..13bb580c9 100644 --- a/doc/release_notes/release_1.04.md +++ b/doc/release_notes/release_1.04.md @@ -23,3 +23,9 @@ * Existing PlotPy annotations remain readable and are converted only after an annotation edit is accepted; simply opening a workspace or cancelling the editor leaves its data unchanged * Annotation identifiers, lock state, custom metadata and extension data are preserved across edits, while unknown third-party payloads are retained without modification * DataLab now requires Sigima ≥ 1.3.0 and SigimaX ≥ 1.1.0 + +### 🛠️ Bug Fixes ### + +**Plugins and History:** + +* Replaying a History action or re-processing a result created by a plugin feature that shares its name with a built-in feature now runs the plugin feature instead of the built-in one