diff --git a/openhcs/config_framework/object_state.py b/openhcs/config_framework/object_state.py index ace33acaf..93779d2e6 100644 --- a/openhcs/config_framework/object_state.py +++ b/openhcs/config_framework/object_state.py @@ -4,9 +4,6 @@ This class holds configuration state independently of UI widgets. Lifecycle: Created when object added to pipeline, persists until removed. PFM attaches to ObjectState when editor opens, detaches when closed. - -ObjectStateRegistry: Singleton registry of all ObjectState instances. -Replaces LiveContextService._active_form_managers as the single source of truth. """ from dataclasses import is_dataclass, fields as dataclass_fields import logging @@ -21,1161 +18,7 @@ logger = logging.getLogger(__name__) -class ObjectStateRegistry: - """Singleton registry of all ObjectState instances. - - Replaces LiveContextService._active_form_managers as the single source of truth - for all configuration state. Keyed by scope_id for efficient lookup. - - Lifecycle ownership: - - PipelineEditor: registers when step added, unregisters when step removed - - ImageBrowser: registers when opened, unregisters when closed - - Config window: registers PipelineConfig/GlobalPipelineConfig - - Thread safety: Not thread-safe (all operations expected on main thread). - """ - _states: Dict[str, 'ObjectState'] = {} # Keys are always strings (None normalized to "") - - # Registration lifecycle callbacks - UI subscribes to sync list items with ObjectState lifecycle - # Callbacks receive (scope_id: str, object_state: ObjectState) - _on_register_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] - _on_unregister_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] - - # Time-travel completion callbacks - UI subscribes to reopen windows for dirty states - # Callbacks receive (dirty_states, triggering_scope) where: - # - dirty_states: list of (scope_id, ObjectState) tuples with unsaved changes - # - triggering_scope: scope_id that triggered the snapshot (may be None) - _on_time_travel_complete_callbacks: List[Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]] = [] - - # History changed callbacks - fired when history is modified (snapshot added or time-travel) - # Used by TimeTravelWidget to stay in sync without polling - _on_history_changed_callbacks: List[Callable[[], None]] = [] - - @classmethod - def add_register_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: - """Subscribe to ObjectState registration events.""" - if callback not in cls._on_register_callbacks: - cls._on_register_callbacks.append(callback) - - @classmethod - def remove_register_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: - """Unsubscribe from ObjectState registration events.""" - if callback in cls._on_register_callbacks: - cls._on_register_callbacks.remove(callback) - - @classmethod - def add_unregister_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: - """Subscribe to ObjectState unregistration events.""" - if callback not in cls._on_unregister_callbacks: - cls._on_unregister_callbacks.append(callback) - - @classmethod - def remove_unregister_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: - """Unsubscribe from ObjectState unregistration events.""" - if callback in cls._on_unregister_callbacks: - cls._on_unregister_callbacks.remove(callback) - - @classmethod - def add_time_travel_complete_callback(cls, callback: Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]) -> None: - """Subscribe to time-travel completion events. - - Callback receives (dirty_states, triggering_scope) where: - - dirty_states: list of (scope_id, ObjectState) tuples with unsaved changes - - triggering_scope: scope_id that triggered the snapshot (may be None) - """ - if callback not in cls._on_time_travel_complete_callbacks: - cls._on_time_travel_complete_callbacks.append(callback) - - @classmethod - def remove_time_travel_complete_callback(cls, callback: Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]) -> None: - """Unsubscribe from time-travel completion events.""" - if callback in cls._on_time_travel_complete_callbacks: - cls._on_time_travel_complete_callbacks.remove(callback) - - @classmethod - def add_history_changed_callback(cls, callback: Callable[[], None]) -> None: - """Subscribe to history change events (snapshot added or time-travel).""" - if callback not in cls._on_history_changed_callbacks: - cls._on_history_changed_callbacks.append(callback) - - @classmethod - def remove_history_changed_callback(cls, callback: Callable[[], None]) -> None: - """Unsubscribe from history change events.""" - if callback in cls._on_history_changed_callbacks: - cls._on_history_changed_callbacks.remove(callback) - - @classmethod - def _fire_history_changed_callbacks(cls) -> None: - """Fire all history changed callbacks.""" - for callback in cls._on_history_changed_callbacks: - try: - callback() - except Exception as e: - logger.warning(f"Error in history_changed callback: {e}") - - @classmethod - def _fire_register_callbacks(cls, scope_key: str, state: 'ObjectState') -> None: - """Fire all registered callbacks for ObjectState registration.""" - for callback in cls._on_register_callbacks: - try: - callback(scope_key, state) - except Exception as e: - logger.warning(f"Error in register callback: {e}") - - @classmethod - def _fire_unregister_callbacks(cls, scope_key: str, state: 'ObjectState') -> None: - """Fire all registered callbacks for ObjectState unregistration.""" - for callback in cls._on_unregister_callbacks: - try: - callback(scope_key, state) - except Exception as e: - logger.warning(f"Error in unregister callback: {e}") - - @classmethod - def _normalize_scope_id(cls, scope_id: Optional[str]) -> str: - """Normalize scope_id: None and "" both represent global scope.""" - return scope_id if scope_id is not None else "" - - @classmethod - def register(cls, state: 'ObjectState', _skip_snapshot: bool = False) -> None: - """Register an ObjectState in the registry. - - Args: - state: The ObjectState to register. - scope_id=None/"" for GlobalPipelineConfig (global scope). - scope_id=plate_path for PipelineConfig. - scope_id=plate_path::step_N for steps. - _skip_snapshot: Internal flag for time-travel (don't record snapshot). - """ - key = cls._normalize_scope_id(state.scope_id) - - if key in cls._states: - logger.warning(f"Overwriting existing ObjectState for scope: {key}") - - cls._states[key] = state - logger.debug(f"Registered ObjectState: scope={key}, type={type(state.object_instance).__name__}") - - # Fire callbacks for UI binding - cls._fire_register_callbacks(key, state) - - # Record snapshot for time-travel (captures new ObjectState in registry) - if not _skip_snapshot: - cls.record_snapshot(f"register {type(state.object_instance).__name__}", key) - - @classmethod - def unregister(cls, state: 'ObjectState', _skip_snapshot: bool = False) -> None: - """Unregister an ObjectState from the registry. - - Args: - state: The ObjectState to unregister. - _skip_snapshot: Internal flag for time-travel (don't record snapshot). - """ - key = cls._normalize_scope_id(state.scope_id) - if key in cls._states: - obj_type_name = type(state.object_instance).__name__ - del cls._states[key] - logger.debug(f"Unregistered ObjectState: scope={key}") - - # Fire callbacks for UI binding - cls._fire_unregister_callbacks(key, state) - - # Record snapshot for time-travel (captures ObjectState removal) - if not _skip_snapshot: - cls.record_snapshot(f"unregister {obj_type_name}", key) - - @classmethod - def unregister_scope_and_descendants(cls, scope_id: Optional[str], _skip_snapshot: bool = False) -> int: - """Unregister an ObjectState and all its descendants from the registry. - - This is used when deleting a plate - we need to cascade delete all child - ObjectStates (steps, functions) to prevent memory leaks. - - Example: - When deleting plate "/path/to/plate", this unregisters: - - "/path/to/plate" (the plate's PipelineConfig) - - "/path/to/plate::step_0" (step ObjectStates) - - "/path/to/plate::step_0::func_0" (function ObjectStates) - - etc. - - Args: - scope_id: The scope to unregister (along with all descendants). - _skip_snapshot: Internal flag for time-travel (don't record snapshot). - - Returns: - Number of ObjectStates unregistered. - """ - scope_key = cls._normalize_scope_id(scope_id) - - # Find all scopes to delete: exact match + descendants - scopes_to_delete = [] - for key in cls._states.keys(): - # Exact match - if key == scope_key: - scopes_to_delete.append(key) - # Descendant (starts with scope_key::) - elif scope_key and key.startswith(scope_key + "::"): - scopes_to_delete.append(key) - - # Delete all matching scopes and fire callbacks - for key in scopes_to_delete: - state = cls._states.pop(key) - logger.debug(f"Unregistered ObjectState (cascade): scope={key}") - # Fire callbacks for UI binding - cls._fire_unregister_callbacks(key, state) - - if scopes_to_delete: - logger.info(f"Cascade unregistered {len(scopes_to_delete)} ObjectState(s) for scope={scope_key}") - # Record single snapshot for cascade unregister (captures all removals at once) - if not _skip_snapshot: - cls.record_snapshot(f"unregister_cascade ({len(scopes_to_delete)} scopes)", scope_key) - - return len(scopes_to_delete) - - @classmethod - def get_by_scope(cls, scope_id: Optional[str]) -> Optional['ObjectState']: - """Get ObjectState by scope_id. - - Args: - scope_id: The scope identifier (e.g., "/path::step_0", or None/"" for global scope). - - Returns: - ObjectState if found, None otherwise. - """ - return cls._states.get(cls._normalize_scope_id(scope_id)) - - @classmethod - def get_all(cls) -> List['ObjectState']: - """Get all registered ObjectStates. - - Returns: - List of all ObjectState instances. Used by LiveContextService.collect(). - """ - return list(cls._states.values()) - - @classmethod - def clear(cls) -> None: - """Clear all registered states. For testing only.""" - cls._states.clear() - logger.debug("Cleared all ObjectStates from registry") - - # ========== TOKEN MANAGEMENT AND CHANGE NOTIFICATION ========== - - _token: int = 0 # Cache invalidation token - _change_callbacks: List[Callable[[], None]] = [] # Change listeners - - @classmethod - def get_token(cls) -> int: - """Get current cache invalidation token.""" - return cls._token - - @classmethod - def increment_token(cls, notify: bool = True) -> None: - """Increment cache invalidation token. - - Args: - notify: If True (default), notify listeners of the change. - Set to False when you need to invalidate caches but will - notify listeners later (e.g., after sibling refresh completes). - """ - cls._token += 1 - if notify: - cls._notify_change() - - @classmethod - def _notify_change(cls) -> None: - """Notify all listeners that something changed. - - UI-agnostic: No PyQt imports. If a callback's object was deleted, - the RuntimeError is caught and the callback is removed. - """ - logger.debug(f"πŸ”” _notify_change: notifying {len(cls._change_callbacks)} listeners") - dead_callbacks = [] - for callback in cls._change_callbacks: - try: - callback() - except RuntimeError as e: - # "wrapped C/C++ object has been deleted" - mark for removal - if "deleted" in str(e).lower(): - logger.debug(f" ⚠️ Callback's object was deleted, removing: {e}") - dead_callbacks.append(callback) - else: - logger.warning(f"Change callback failed: {e}") - except Exception as e: - logger.warning(f"Change callback failed: {e}") - - # Clean up dead callbacks - for cb in dead_callbacks: - cls._change_callbacks.remove(cb) - - @classmethod - def connect_listener(cls, callback: Callable[[], None]) -> None: - """Connect a listener callback that's called on any change. - - The callback should debounce and call collect() to get fresh values. - """ - if callback not in cls._change_callbacks: - cls._change_callbacks.append(callback) - logger.debug(f"Connected change listener: {callback}") - - @classmethod - def disconnect_listener(cls, callback: Callable[[], None]) -> None: - """Disconnect a change listener.""" - if callback in cls._change_callbacks: - cls._change_callbacks.remove(callback) - logger.debug(f"Disconnected change listener: {callback}") - - # ========== ANCESTOR OBJECT COLLECTION ========== - - @classmethod - def get_ancestor_objects(cls, scope_id: Optional[str], use_saved: bool = False) -> List[Any]: - """Get objects from this scope and all ancestors, leastβ†’most specific. - - Replaces LiveContextService.collect() + merge_ancestor_values() for simpler - context stack building. - - Args: - scope_id: The scope to get ancestors for (e.g., "/plate::step_0") - use_saved: If True, return saved baseline (object_instance) instead of - live state (to_object()). Used when computing _saved_resolved - to ensure saved baseline only depends on other saved baselines. - - Returns: - List of objects from ancestor scopes, ordered leastβ†’most specific. - Each object is from state.object_instance (saved) or state.to_object() (live). - """ - scope_key = cls._normalize_scope_id(scope_id) - - # Build list of ancestor scope keys (least-specific to most-specific) - # e.g., "/plate::step_0" -> ["", "/plate", "/plate::step_0"] - ancestors = [""] # Global scope always included - if scope_key: - parts = scope_key.split("::") - for i in range(len(parts)): - ancestors.append("::".join(parts[:i+1])) - - # Get objects from ancestor scopes - objects = [] - for ancestor_key in ancestors: - state = cls._states.get(ancestor_key) - if state: - if use_saved: - # Return saved baseline (object_instance is updated in mark_saved) - objects.append(state.object_instance) - else: - # Return live state with current edits - objects.append(state.to_object()) - - return objects - - @classmethod - def get_ancestor_objects_with_scopes(cls, scope_id: Optional[str], use_saved: bool = False) -> List[Tuple[str, Any]]: - """Get (scope_id, object) tuples from this scope and all ancestors. - - Similar to get_ancestor_objects() but includes the scope_id for each object. - Used for provenance tracking to determine which scope provided a resolved value. - - Args: - scope_id: The scope to get ancestors for (e.g., "/plate::step_0") - use_saved: If True, return saved baseline (object_instance) instead of - live state (to_object()). Used when computing _saved_resolved. - - Returns: - List of (scope_id, object) tuples from ancestor scopes, ordered leastβ†’most specific. - """ - scope_key = cls._normalize_scope_id(scope_id) - - # Build list of ancestor scope keys (least-specific to most-specific) - ancestors = [""] # Global scope always included - if scope_key: - parts = scope_key.split("::") - for i in range(len(parts)): - ancestors.append("::".join(parts[:i+1])) - - # Get (scope_id, object) tuples from ancestor scopes - results: List[Tuple[str, Any]] = [] - for ancestor_key in ancestors: - state = cls._states.get(ancestor_key) - if state: - obj = state.object_instance if use_saved else state.to_object() - results.append((ancestor_key, obj)) - - return results - - # ========== SCOPE + TYPE + FIELD AWARE INVALIDATION ========== - - @classmethod - def invalidate_by_type_and_scope( - cls, - scope_id: Optional[str], - changed_type: type, - field_name: str, - invalidate_saved: bool = False - ) -> None: - """Invalidate a specific field in states that could inherit from changed_type at scope_id. - - PERFORMANCE: Three-tier filtering: - 1. SCOPE: State must be at or below changed scope (descendants inherit) - 2. TYPE: State's type tree must include changed_type - 3. FIELD: Only invalidate the specific field that changed - - If changing GlobalPipelineConfig.napari_streaming_config.window_size: - - Only states with napari_streaming_config in their tree - - Only the 'window_size' field is invalidated, not all 20+ fields - - Steps without napari_streaming_config are NOT touched - - Args: - scope_id: The scope that changed (None/"" for global scope) - changed_type: The type of the ObjectState that was modified - field_name: The specific field that changed - invalidate_saved: If True, also invalidate saved_resolved cache for descendants - """ - from openhcs.config_framework.lazy_factory import get_base_type_for_lazy - from openhcs.config_framework.dual_axis_resolver import invalidate_mro_cache_for_field - - # PERFORMANCE: Targeted cache invalidation - only clear entries for this field/type - invalidate_mro_cache_for_field(changed_type, field_name) - - changed_scope = cls._normalize_scope_id(scope_id) - - # Normalize to base type for comparison (LazyX β†’ X) - base_changed_type = get_base_type_for_lazy(changed_type) or changed_type - - # DEBUG: Log invalidation for well_filter - if field_name == 'well_filter': - logger.debug(f"πŸ” invalidate_by_type_and_scope: scope={changed_scope!r}, type={base_changed_type.__name__}, field={field_name}, total_states={len(cls._states)}") - - for state in cls._states.values(): - state_scope = cls._normalize_scope_id(state.scope_id) - - # SCOPE CHECK: must be at or below changed scope - # Global scope (empty string) affects ALL states - if changed_scope == "": - # Global scope - always a descendant (or self if also global) - if field_name == 'well_filter': - logger.debug(f"πŸ” Checking state: scope={state_scope!r}, obj_type={type(state.object_instance).__name__}") - logger.debug(f"[SCOPE] Global change affects state scope={state_scope!r}") - else: - # Non-global: check exact match or descendant - is_self = (state_scope == changed_scope) - prefix = changed_scope + "::" - is_descendant = state_scope.startswith(prefix) - if not (is_self or is_descendant): - logger.debug(f"[SCOPE] SKIP: changed_scope={changed_scope!r} does not affect state_scope={state_scope!r}") - continue - logger.debug(f"[SCOPE] MATCH: changed_scope={changed_scope!r} affects state_scope={state_scope!r}") - - # TYPE + FIELD CHECK: find matching nested state and invalidate field - cls._invalidate_field_in_matching_states(state, base_changed_type, field_name, invalidate_saved) - - @classmethod - def _invalidate_field_in_matching_states( - cls, - state: 'ObjectState', - target_base_type: type, - field_name: str, - invalidate_saved: bool = False - ) -> None: - """Find fields in state that could inherit from target_base_type and invalidate them. - - With flat storage, we scan _path_to_type to find all dotted paths whose - container type matches or inherits from target_base_type. - - A field should be invalidated if: - 1. Its container type matches target_base_type exactly, OR - 2. Its container type inherits from target_base_type (has target_base_type in MRO) - - This handles sibling inheritance: when WellFilterConfig.well_filter changes, - both 'well_filter_config.well_filter' and 'step_well_filter_config.well_filter' - are invalidated (since StepWellFilterConfig inherits from WellFilterConfig). - - Args: - state: ObjectState to check - target_base_type: Normalized base type to match - field_name: Field to invalidate - invalidate_saved: If True, also invalidate saved_resolved cache for this field - """ - from openhcs.config_framework.lazy_factory import get_base_type_for_lazy - - invalidated_paths: set[str] = set() - - # Scan _path_to_type for matching container types - for dotted_path, container_type in state._path_to_type.items(): - # Normalize container type - container_base_type = get_base_type_for_lazy(container_type) or container_type - - # Check if target_base_type is in the MRO (container inherits the field) - type_matches = False - for mro_class in container_base_type.__mro__: - mro_base = get_base_type_for_lazy(mro_class) or mro_class - if mro_base == target_base_type: - type_matches = True - break - - # If type matches and path ends with the field_name, invalidate it - if type_matches and (dotted_path.endswith(f'.{field_name}') or dotted_path == field_name): - if dotted_path in state.parameters: - state.invalidate_field(dotted_path) - invalidated_paths.add(dotted_path) - - # If invalidating saved baseline, remove from saved_resolved so it recomputes - if invalidate_saved and dotted_path in state._saved_resolved: - del state._saved_resolved[dotted_path] - logger.debug(f"Invalidated saved_resolved cache for {dotted_path}") - - # Trigger recompute immediately to detect if resolved values actually changed. - # This ensures callbacks fire only when values change, not just when fields are invalidated. - # Prevents false flashes when Reset is clicked on already-reset fields. - if invalidated_paths: - state._ensure_live_resolved(notify_flash=True) - - # If we invalidated saved baseline, recompute it now with fresh ancestor values - if invalidate_saved: - # Recompute entire saved_resolved snapshot to pick up new ancestor saved values - old_saved_resolved = state._saved_resolved - state._saved_resolved = state._compute_resolved_snapshot(use_saved=True) - logger.debug(f"Recomputed saved_resolved baseline after invalidation") - - if old_saved_resolved != state._saved_resolved: - state._sync_materialized_state() - else: - state._sync_materialized_state() - - # ========== REGISTRY-LEVEL TIME-TRAVEL (DAG Model) ========== - - # Snapshots: Dict of all snapshots by ID (the DAG - snapshots are NEVER deleted) - # Each Snapshot contains id, timestamp, label, triggering_scope, parent_id, all_states - # The parent_id forms the DAG edges - _snapshots: Dict[str, Snapshot] = {} - - # Current head: None = at branch head (live), snapshot_id = time-traveling to that snapshot - _current_head: Optional[str] = None - - _history_enabled: bool = True - _max_history_size: int = 1000 # Max snapshots in DAG (increased since we don't truncate branches) - _in_time_travel: bool = False # Flag for PFM to know to refresh widget values - - # Limbo: ObjectStates temporarily removed during time-travel - # When traveling to a snapshot, ObjectStates not in that snapshot are moved here. - # When traveling forward or to head, they're restored from here. - _time_travel_limbo: Dict[str, 'ObjectState'] = {} - - # Timelines (branches): Named branches that point to a head snapshot - # "main" is always created automatically on first snapshot - # head_id always points to a valid key in _snapshots (guaranteed by construction) - _timelines: Dict[str, Timeline] = {} - _current_timeline: str = "main" - - @classmethod - def get_branch_history(cls, branch_name: Optional[str] = None) -> List[Snapshot]: - """Get ordered history for a branch by walking parent_id chain. - - Args: - branch_name: Branch to get history for. If None, uses current branch. - - Returns: - List of snapshots from oldest to newest (root first, head last). - Index 0 = oldest (root), index -1 = newest (head). - Empty list if branch has no snapshots yet. - """ - if branch_name is None: - branch_name = cls._current_timeline - - if branch_name not in cls._timelines: - return [] - - head_id = cls._timelines[branch_name].head_id - history = [] - current_id: Optional[str] = head_id - - while current_id is not None: - snapshot = cls._snapshots[current_id] # KeyError if bug - history.append(snapshot) - current_id = snapshot.parent_id - - history.reverse() # [oldest, ..., newest] - natural timeline order - return history - - @classmethod - def get_current_snapshot_index(cls) -> int: - """Get current position as index into branch history. - - Returns: - -1 if at head (live), else index into get_branch_history() list. - Index 0 = oldest, len-1 = head. - """ - if cls._current_head is None: - return -1 - - history = cls.get_branch_history() - for i, snapshot in enumerate(history): - if snapshot.id == cls._current_head: - return i - - logger.error(f"⏱️ BRANCH: current_head {cls._current_head} not in branch history") - return -1 - - @classmethod - def record_snapshot(cls, label: str = "", scope_id: Optional[str] = None) -> None: - """Record current state of ALL ObjectStates to history. - - Called automatically after significant state changes. - Each snapshot captures the full system state at a point in time. - - On FIRST edit, automatically records a baseline "init" snapshot first, - so users can always go back to the original state before any edits. - - Args: - label: Human-readable label (e.g., "edit group_by", "save") - scope_id: Optional scope that triggered the snapshot (for labeling) - """ - if not cls._history_enabled: - return - - # CRITICAL: Don't record snapshots during time-travel - if cls._in_time_travel: - return - - import time - - # FIRST EDIT: Record baseline "init" snapshot before the actual edit snapshot - is_first_snapshot = len(cls._snapshots) == 0 - if is_first_snapshot and label.startswith("edit"): - cls._record_snapshot_internal("init", time.time(), None) - - full_label = f"{label} [{scope_id}]" if scope_id else label - cls._record_snapshot_internal(full_label, time.time(), scope_id) - - @classmethod - def _record_snapshot_internal(cls, label: str, timestamp: float, triggering_scope: Optional[str]) -> None: - """Internal method to record a snapshot without baseline logic. - - DAG Model: - - Snapshots are added to _snapshots dict (never deleted) - - If _current_head is not None (time-traveling), we're diverging - - On diverge: create auto-branch for old future, new snapshot parents from _current_head - - Branch head_id is updated to point to new snapshot - """ - import uuid - - # Capture ALL registered ObjectStates as StateSnapshot dataclasses - all_states: Dict[str, StateSnapshot] = {} - for key, state in cls._states.items(): - all_states[key] = StateSnapshot( - saved_resolved=copy.deepcopy(state._saved_resolved), - live_resolved=copy.deepcopy(state._live_resolved) if state._live_resolved else {}, - parameters=copy.deepcopy(state.parameters), - provenance=copy.deepcopy(state._live_provenance), - ) - - # Determine parent_id for new snapshot - if cls._current_head is not None: - # We're in the past - diverging from _current_head - parent_id = cls._current_head - - # Auto-branch: preserve old future before we diverge - # The old branch head becomes an auto-saved branch - if cls._current_timeline in cls._timelines: - old_head_id = cls._timelines[cls._current_timeline].head_id - if old_head_id != cls._current_head: - # There IS an old future to preserve - branch_name = f"auto-{old_head_id[:8]}" - if branch_name not in cls._timelines: - cls._timelines[branch_name] = Timeline( - name=branch_name, - head_id=old_head_id, - base_id=cls._current_head, - description=f"Auto-saved from diverge at {cls._current_head[:8]}", - ) - old_snapshot = cls._snapshots[old_head_id] - logger.info(f"⏱️ AUTO-BRANCH: Created '{branch_name}' (was {old_snapshot.label})") - - # Clear limbo when diverging - cls._time_travel_limbo.clear() - else: - # At branch head - parent is current branch's head (if exists) - if cls._current_timeline in cls._timelines: - parent_id = cls._timelines[cls._current_timeline].head_id - else: - parent_id = None - - # Create new snapshot - snapshot = Snapshot( - id=str(uuid.uuid4()), - timestamp=timestamp, - label=label, - triggering_scope=triggering_scope, - parent_id=parent_id, - all_states=all_states, - ) - - # Add to DAG - cls._snapshots[snapshot.id] = snapshot - - # Update or create current branch to point to new snapshot - if cls._current_timeline not in cls._timelines: - cls._timelines[cls._current_timeline] = Timeline( - name=cls._current_timeline, - head_id=snapshot.id, - base_id=snapshot.id, - description="Main timeline" if cls._current_timeline == "main" else "", - ) - logger.info(f"⏱️ BRANCH: Created '{cls._current_timeline}' timeline") - else: - cls._timelines[cls._current_timeline].head_id = snapshot.id - - # Return to head (live state) - we're no longer time-traveling - cls._current_head = None - - # Enforce max DAG size (prune oldest snapshots not referenced by any branch) - if len(cls._snapshots) > cls._max_history_size: - cls._prune_unreachable_snapshots() - - logger.debug(f"⏱️ SNAPSHOT: Recorded '{label}' (id={snapshot.id[:8]})") - cls._fire_history_changed_callbacks() - - @classmethod - def _prune_unreachable_snapshots(cls) -> None: - """Remove snapshots not reachable from any branch head. - - Walks from each branch head to find all reachable snapshots, - then removes any that aren't reachable. - """ - reachable: Set[str] = set() - - for timeline in cls._timelines.values(): - current_id: Optional[str] = timeline.head_id - while current_id is not None and current_id not in reachable: - reachable.add(current_id) - snapshot = cls._snapshots.get(current_id) - current_id = snapshot.parent_id if snapshot else None - - # Remove unreachable - unreachable = set(cls._snapshots.keys()) - reachable - for snapshot_id in unreachable: - del cls._snapshots[snapshot_id] - - if unreachable: - logger.debug(f"⏱️ PRUNE: Removed {len(unreachable)} unreachable snapshots") - - @classmethod - def time_travel_to_snapshot(cls, snapshot_id: str) -> bool: - """Travel ALL ObjectStates to a specific snapshot by ID. - - Full time-travel: ObjectStates are registered/unregistered to match the snapshot. - ObjectStates not in the snapshot are moved to limbo. ObjectStates in snapshot - but not in registry are restored from limbo. - - Args: - snapshot_id: UUID of snapshot to travel to - - Returns: - True if travel succeeded. - """ - if snapshot_id not in cls._snapshots: - logger.error(f"⏱️ TIME_TRAVEL: Snapshot {snapshot_id} not found") - return False - - snapshot = cls._snapshots[snapshot_id] - cls._current_head = snapshot_id - - # Set flag so PFM knows to refresh widget values - cls._in_time_travel = True - try: - snapshot_scopes = set(snapshot.all_states.keys()) - current_scopes = set(cls._states.keys()) - - # PHASE 1: UNREGISTER ObjectStates not in snapshot (move to limbo) - scopes_to_limbo = current_scopes - snapshot_scopes - for scope_key in scopes_to_limbo: - state = cls._states.pop(scope_key) - cls._time_travel_limbo[scope_key] = state - cls._fire_unregister_callbacks(scope_key, state) - logger.debug(f"⏱️ TIME_TRAVEL: Moved to limbo: {scope_key}") - - # PHASE 2: RE-REGISTER ObjectStates from limbo - scopes_to_register = snapshot_scopes - current_scopes - for scope_key in scopes_to_register: - state = cls._time_travel_limbo.pop(scope_key) # KeyError if bug - cls._states[scope_key] = state - cls._fire_register_callbacks(scope_key, state) - logger.debug(f"⏱️ TIME_TRAVEL: Restored from limbo: {scope_key}") - - # PHASE 3: RESTORE state for all ObjectStates in snapshot - for scope_key, state_snap in snapshot.all_states.items(): - state = cls._states.get(scope_key) - if not state: - continue - - current_params = state.parameters.copy() if state.parameters else {} - target_live = state_snap.live_resolved - current_live = state._live_resolved.copy() if state._live_resolved else {} - - # Find changed resolved values - changed_paths = set() - all_keys = set(target_live.keys()) | set(current_live.keys()) - for key in all_keys: - if target_live.get(key) != current_live.get(key): - changed_paths.add(key) - - # Find changed raw parameters - all_param_keys = set(state_snap.parameters.keys()) | set(current_params.keys()) - for param_key in all_param_keys: - before = current_params.get(param_key) - after = state_snap.parameters.get(param_key) - if before != after and not (is_dataclass(before) or is_dataclass(after)): - changed_paths.add(param_key) - - # RESTORE state - state._saved_resolved = copy.deepcopy(state_snap.saved_resolved) - state._live_resolved = copy.deepcopy(state_snap.live_resolved) - state._live_provenance = copy.deepcopy(state_snap.provenance) - state.parameters = copy.deepcopy(state_snap.parameters) - state._sync_materialized_state() - - # Notify UI - if changed_paths and state._on_resolved_changed_callbacks: - for callback in state._on_resolved_changed_callbacks: - callback(changed_paths) - - # PHASE 4: Fire time-travel completion callbacks - if cls._on_time_travel_complete_callbacks: - dirty_states = [ - (scope_key, state) - for scope_key, state in cls._states.items() - if state.dirty_fields - ] - if dirty_states: - logger.debug(f"⏱️ TIME_TRAVEL: {len(dirty_states)} dirty states") - for callback in cls._on_time_travel_complete_callbacks: - callback(dirty_states, snapshot.triggering_scope) - finally: - cls._in_time_travel = False - - cls._fire_history_changed_callbacks() - logger.info(f"⏱️ TIME_TRAVEL: Traveled to {snapshot.label} ({snapshot_id[:8]})") - return True - - @classmethod - def time_travel_to(cls, index: int) -> bool: - """Travel to a snapshot by index in current branch history. - - Convenience method for UI sliders. Uses get_branch_history() to map - index to snapshot_id. - - Args: - index: Index into branch history (0 = oldest/root, -1 = newest/head) - - Returns: - True if travel succeeded. - """ - history = cls.get_branch_history() - if not history: - logger.warning("⏱️ TIME_TRAVEL: No history") - return False - - # Normalize negative index - if index < 0: - index = len(history) + index - - if index < 0 or index >= len(history): - logger.warning(f"⏱️ TIME_TRAVEL: Index {index} out of range [0, {len(history) - 1}]") - return False - - # Index len-1 = head (newest), returning to head means exit time-travel - if index == len(history) - 1: - return cls.time_travel_to_head() - - snapshot = history[index] - return cls.time_travel_to_snapshot(snapshot.id) - - @classmethod - def time_travel_back(cls) -> bool: - """Travel one step back in history (toward older/lower index).""" - history = cls.get_branch_history() - if not history: - return False - - current_index = cls.get_current_snapshot_index() - if current_index == -1: - # At head (len-1) - go one step back - if len(history) < 2: - return False - return cls.time_travel_to_snapshot(history[-2].id) - - # Already time-traveling - go one step older (lower index) - if current_index <= 0: - return False # Already at oldest - return cls.time_travel_to_snapshot(history[current_index - 1].id) - - @classmethod - def time_travel_forward(cls) -> bool: - """Travel one step forward in history (toward newer/higher index).""" - if cls._current_head is None: - return False # Already at head - - history = cls.get_branch_history() - current_index = cls.get_current_snapshot_index() - - # Go one step toward head (higher index) - next_index = current_index + 1 - if next_index >= len(history) - 1: - # At or past head - return to head - return cls.time_travel_to_head() - - return cls.time_travel_to_snapshot(history[next_index].id) - - @classmethod - def time_travel_to_head(cls) -> bool: - """Return to the latest state (exit time-travel mode). - - Restores state from current branch's head snapshot. - """ - if cls._current_head is None: - return True # Already at head - - if cls._current_timeline not in cls._timelines: - logger.warning("⏱️ TIME_TRAVEL: No current timeline") - return False - - head_id = cls._timelines[cls._current_timeline].head_id - result = cls.time_travel_to_snapshot(head_id) - cls._current_head = None # Mark as at head (not time-traveling) - return result - - @classmethod - def get_history_info(cls) -> List[Dict[str, Any]]: - """Get human-readable history for UI display. - - Returns history for current branch, oldest first (index 0 = oldest, -1 = head). - """ - import datetime - history = cls.get_branch_history() - current_index = cls.get_current_snapshot_index() - head_index = len(history) - 1 - - result = [] - for i, snapshot in enumerate(history): - is_head = (i == head_index) - is_current = (current_index == -1 and is_head) or (i == current_index) - result.append({ - 'index': i, - 'id': snapshot.id, - 'timestamp': datetime.datetime.fromtimestamp(snapshot.timestamp).strftime('%H:%M:%S.%f')[:-3], - 'label': snapshot.label or f"Snapshot #{i}", - 'is_current': is_current, - 'is_head': is_head, - 'num_states': len(snapshot.all_states), - 'parent_id': snapshot.parent_id, - }) - return result - - @classmethod - def get_history_length(cls) -> int: - """Get number of snapshots in current branch history.""" - return len(cls.get_branch_history()) - - @classmethod - def is_time_traveling(cls) -> bool: - """Check if currently viewing historical state (not at head).""" - return cls._current_head is not None - - # ========== BRANCH OPERATIONS ========== - - @classmethod - def create_branch(cls, name: str, description: str = "") -> Timeline: - """Create a new branch at current position. - - Args: - name: Branch name (must be unique) - description: Optional description - - Returns: - The created Timeline - """ - # Branch from current position - if cls._current_head is not None: - # Time-traveling - branch from current position - head_id = cls._current_head - elif cls._current_timeline in cls._timelines: - # At head of current branch - head_id = cls._timelines[cls._current_timeline].head_id - else: - logger.error("⏱️ BRANCH: No snapshots to branch from") - raise ValueError("No snapshots to branch from") - - timeline = Timeline( - name=name, - head_id=head_id, - base_id=head_id, - description=description, - ) - cls._timelines[name] = timeline - logger.info(f"⏱️ BRANCH: Created branch '{name}' at {head_id[:8]}") - return timeline - - @classmethod - def switch_branch(cls, name: str) -> bool: - """Switch to a different branch. - - Args: - name: Branch name - - Returns: - True if switch succeeded - """ - timeline = cls._timelines[name] # KeyError if branch doesn't exist - - # Switch to branch and travel to its head - cls._current_timeline = name - result = cls.time_travel_to_snapshot(timeline.head_id) - cls._current_head = None # At head of new branch - cls._fire_history_changed_callbacks() - logger.info(f"⏱️ BRANCH: Switched to '{name}'") - return result - - @classmethod - def delete_branch(cls, name: str) -> None: - """Delete a branch. - - Args: - name: Branch name to delete - - Note: Snapshots are not deleted - they may be reachable from other branches. - Unreachable snapshots are pruned automatically when DAG exceeds max size. - """ - del cls._timelines[name] # KeyError if branch doesn't exist - logger.info(f"⏱️ BRANCH: Deleted branch '{name}'") - cls._fire_history_changed_callbacks() - - @classmethod - def list_branches(cls) -> List[Dict[str, Any]]: - """List all branches. - - Returns: - List of dicts with branch info - """ - return [ - { - 'name': tl.name, - 'head_id': tl.head_id, - 'base_id': tl.base_id, - 'description': tl.description, - 'is_current': tl.name == cls._current_timeline, - } - for tl in cls._timelines.values() - ] - - @classmethod - def get_current_branch(cls) -> str: - """Get current branch name.""" - return cls._current_timeline - - # ========== PERSISTENCE ========== - - @classmethod - def export_history_to_dict(cls) -> Dict[str, Any]: - """Export history to a JSON-serializable dict. - - Returns: - Dict with 'snapshots', 'timelines', 'current_head', 'current_timeline'. - """ - return { - 'snapshots': {sid: snap.to_dict() for sid, snap in cls._snapshots.items()}, - 'timelines': [tl.to_dict() for tl in cls._timelines.values()], - 'current_head': cls._current_head, - 'current_timeline': cls._current_timeline, - } - - @classmethod - def import_history_from_dict(cls, data: Dict[str, Any]) -> None: - """Import history from a dict (e.g., loaded from JSON). - - Only imports state data for scope_ids that currently exist in the registry. - Scopes in the snapshot but not in the app are skipped. - - Args: - data: Dict with 'snapshots', 'timelines', 'current_head', 'current_timeline'. - """ - cls._snapshots.clear() - cls._timelines.clear() - current_scopes = set(cls._states.keys()) - - # Handle both old list format and new dict format - snapshots_data = data['snapshots'] - if isinstance(snapshots_data, list): - # Old format: list of snapshots - snapshot_items = [(s['id'], s) for s in snapshots_data] - else: - # New format: dict of id -> snapshot - snapshot_items = snapshots_data.items() - - for _snapshot_id, snapshot_data in snapshot_items: - # Filter to only scopes that exist in current registry - filtered_states: Dict[str, StateSnapshot] = {} - for scope_id, state_data in snapshot_data['states'].items(): - if scope_id in current_scopes: - filtered_states[scope_id] = StateSnapshot( - saved_resolved=state_data['saved_resolved'], - live_resolved=state_data['live_resolved'], - parameters=state_data['parameters'], - provenance=state_data['provenance'], - ) - - snapshot = Snapshot( - id=snapshot_data['id'], - timestamp=snapshot_data['timestamp'], - label=snapshot_data['label'], - triggering_scope=snapshot_data.get('triggering_scope'), - parent_id=snapshot_data.get('parent_id'), - all_states=filtered_states, - ) - cls._snapshots[snapshot.id] = snapshot - - # Import timelines - if 'timelines' in data: - for tl_data in data['timelines']: - tl = Timeline.from_dict(tl_data) - cls._timelines[tl.name] = tl - cls._current_timeline = data.get('current_timeline', 'main') - else: - cls._current_timeline = 'main' - - # Handle both old index format and new head format - if 'current_head' in data: - cls._current_head = data['current_head'] - elif 'current_index' in data: - # Old format - convert index to head - # Can't reliably convert, just go to head - cls._current_head = None - else: - cls._current_head = None - - @classmethod - def save_history_to_file(cls, filepath: str) -> None: - """Save history to a JSON file. - - Args: - filepath: Path to save the JSON file. - """ - import json - data = cls.export_history_to_dict() - with open(filepath, 'w') as f: - json.dump(data, f, indent=2) - logger.info(f"⏱️ Saved {len(cls._snapshots)} snapshots to {filepath}") - - @classmethod - def load_history_from_file(cls, filepath: str) -> None: - """Load history from a JSON file. - - Args: - filepath: Path to the JSON file. - """ - import json - with open(filepath, 'r') as f: - data = json.load(f) - cls.import_history_from_dict(data) - logger.info(f"⏱️ Loaded {len(cls._snapshots)} snapshots from {filepath}") - +from openhcs.config_framework.object_state_registry import ObjectStateRegistry class FieldProxy: """Type-safe proxy for accessing ObjectState fields via dotted attribute syntax. diff --git a/openhcs/config_framework/object_state_registry.py b/openhcs/config_framework/object_state_registry.py new file mode 100644 index 000000000..ecd3d97f7 --- /dev/null +++ b/openhcs/config_framework/object_state_registry.py @@ -0,0 +1,1182 @@ +""" +ObjectStateRegistry: Singleton registry of all ObjectState instances. + +Replaces LiveContextService._active_form_managers as the single source of truth +for all configuration state. Keyed by scope_id for efficient lookup. + +Lifecycle ownership: +- PipelineEditor: registers when step added, unregisters when step removed +- ImageBrowser: registers when opened, unregisters when closed +- Config window: registers PipelineConfig/GlobalPipelineConfig + +Thread safety: Not thread-safe (all operations expected on main thread). +""" +from dataclasses import is_dataclass, fields as dataclass_fields +import logging +from typing import Any, Callable, Dict, List, Optional, Set, Tuple, TYPE_CHECKING +import copy + +from openhcs.config_framework.snapshot_model import Snapshot, StateSnapshot, Timeline + +if TYPE_CHECKING: + from openhcs.config_framework.object_state import ObjectState + +logger = logging.getLogger(__name__) + + +class ObjectStateRegistry: + """Singleton registry of all ObjectState instances. + + Replaces LiveContextService._active_form_managers as the single source of truth + for all configuration state. Keyed by scope_id for efficient lookup. + + Lifecycle ownership: + - PipelineEditor: registers when step added, unregisters when step removed + - ImageBrowser: registers when opened, unregisters when closed + - Config window: registers PipelineConfig/GlobalPipelineConfig + + Thread safety: Not thread-safe (all operations expected on main thread). + """ + _states: Dict[str, 'ObjectState'] = {} # Keys are always strings (None normalized to "") + + # Registration lifecycle callbacks - UI subscribes to sync list items with ObjectState lifecycle + # Callbacks receive (scope_id: str, object_state: ObjectState) + _on_register_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] + _on_unregister_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] + + # Time-travel completion callbacks - UI subscribes to reopen windows for dirty states + # Callbacks receive (dirty_states, triggering_scope) where: + # - dirty_states: list of (scope_id, ObjectState) tuples with unsaved changes + # - triggering_scope: scope_id that triggered the snapshot (may be None) + _on_time_travel_complete_callbacks: List[Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]] = [] + + # History changed callbacks - fired when history is modified (snapshot added or time-travel) + # Used by TimeTravelWidget to stay in sync without polling + _on_history_changed_callbacks: List[Callable[[], None]] = [] + + @classmethod + def add_register_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: + """Subscribe to ObjectState registration events.""" + if callback not in cls._on_register_callbacks: + cls._on_register_callbacks.append(callback) + + @classmethod + def remove_register_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: + """Unsubscribe from ObjectState registration events.""" + if callback in cls._on_register_callbacks: + cls._on_register_callbacks.remove(callback) + + @classmethod + def add_unregister_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: + """Subscribe to ObjectState unregistration events.""" + if callback not in cls._on_unregister_callbacks: + cls._on_unregister_callbacks.append(callback) + + @classmethod + def remove_unregister_callback(cls, callback: Callable[[str, 'ObjectState'], None]) -> None: + """Unsubscribe from ObjectState unregistration events.""" + if callback in cls._on_unregister_callbacks: + cls._on_unregister_callbacks.remove(callback) + + @classmethod + def add_time_travel_complete_callback(cls, callback: Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]) -> None: + """Subscribe to time-travel completion events. + + Callback receives (dirty_states, triggering_scope) where: + - dirty_states: list of (scope_id, ObjectState) tuples with unsaved changes + - triggering_scope: scope_id that triggered the snapshot (may be None) + """ + if callback not in cls._on_time_travel_complete_callbacks: + cls._on_time_travel_complete_callbacks.append(callback) + + @classmethod + def remove_time_travel_complete_callback(cls, callback: Callable[[List[Tuple[str, 'ObjectState']], Optional[str]], None]) -> None: + """Unsubscribe from time-travel completion events.""" + if callback in cls._on_time_travel_complete_callbacks: + cls._on_time_travel_complete_callbacks.remove(callback) + + @classmethod + def add_history_changed_callback(cls, callback: Callable[[], None]) -> None: + """Subscribe to history change events (snapshot added or time-travel).""" + if callback not in cls._on_history_changed_callbacks: + cls._on_history_changed_callbacks.append(callback) + + @classmethod + def remove_history_changed_callback(cls, callback: Callable[[], None]) -> None: + """Unsubscribe from history change events.""" + if callback in cls._on_history_changed_callbacks: + cls._on_history_changed_callbacks.remove(callback) + + @classmethod + def _fire_history_changed_callbacks(cls) -> None: + """Fire all history changed callbacks.""" + for callback in cls._on_history_changed_callbacks: + try: + callback() + except Exception as e: + logger.warning(f"Error in history_changed callback: {e}") + + @classmethod + def _fire_register_callbacks(cls, scope_key: str, state: 'ObjectState') -> None: + """Fire all registered callbacks for ObjectState registration.""" + for callback in cls._on_register_callbacks: + try: + callback(scope_key, state) + except Exception as e: + logger.warning(f"Error in register callback: {e}") + + @classmethod + def _fire_unregister_callbacks(cls, scope_key: str, state: 'ObjectState') -> None: + """Fire all registered callbacks for ObjectState unregistration.""" + for callback in cls._on_unregister_callbacks: + try: + callback(scope_key, state) + except Exception as e: + logger.warning(f"Error in unregister callback: {e}") + + @classmethod + def _normalize_scope_id(cls, scope_id: Optional[str]) -> str: + """Normalize scope_id: None and "" both represent global scope.""" + return scope_id if scope_id is not None else "" + + @classmethod + def register(cls, state: 'ObjectState', _skip_snapshot: bool = False) -> None: + """Register an ObjectState in the registry. + + Args: + state: The ObjectState to register. + scope_id=None/"" for GlobalPipelineConfig (global scope). + scope_id=plate_path for PipelineConfig. + scope_id=plate_path::step_N for steps. + _skip_snapshot: Internal flag for time-travel (don't record snapshot). + """ + key = cls._normalize_scope_id(state.scope_id) + + if key in cls._states: + logger.warning(f"Overwriting existing ObjectState for scope: {key}") + + cls._states[key] = state + logger.debug(f"Registered ObjectState: scope={key}, type={type(state.object_instance).__name__}") + + # Fire callbacks for UI binding + cls._fire_register_callbacks(key, state) + + # Record snapshot for time-travel (captures new ObjectState in registry) + if not _skip_snapshot: + cls.record_snapshot(f"register {type(state.object_instance).__name__}", key) + + @classmethod + def unregister(cls, state: 'ObjectState', _skip_snapshot: bool = False) -> None: + """Unregister an ObjectState from the registry. + + Args: + state: The ObjectState to unregister. + _skip_snapshot: Internal flag for time-travel (don't record snapshot). + """ + key = cls._normalize_scope_id(state.scope_id) + if key in cls._states: + obj_type_name = type(state.object_instance).__name__ + del cls._states[key] + logger.debug(f"Unregistered ObjectState: scope={key}") + + # Fire callbacks for UI binding + cls._fire_unregister_callbacks(key, state) + + # Record snapshot for time-travel (captures ObjectState removal) + if not _skip_snapshot: + cls.record_snapshot(f"unregister {obj_type_name}", key) + + @classmethod + def unregister_scope_and_descendants(cls, scope_id: Optional[str], _skip_snapshot: bool = False) -> int: + """Unregister an ObjectState and all its descendants from the registry. + + This is used when deleting a plate - we need to cascade delete all child + ObjectStates (steps, functions) to prevent memory leaks. + + Example: + When deleting plate "/path/to/plate", this unregisters: + - "/path/to/plate" (the plate's PipelineConfig) + - "/path/to/plate::step_0" (step ObjectStates) + - "/path/to/plate::step_0::func_0" (function ObjectStates) + - etc. + + Args: + scope_id: The scope to unregister (along with all descendants). + _skip_snapshot: Internal flag for time-travel (don't record snapshot). + + Returns: + Number of ObjectStates unregistered. + """ + scope_key = cls._normalize_scope_id(scope_id) + + # Find all scopes to delete: exact match + descendants + scopes_to_delete = [] + for key in cls._states.keys(): + # Exact match + if key == scope_key: + scopes_to_delete.append(key) + # Descendant (starts with scope_key::) + elif scope_key and key.startswith(scope_key + "::"): + scopes_to_delete.append(key) + + # Delete all matching scopes and fire callbacks + for key in scopes_to_delete: + state = cls._states.pop(key) + logger.debug(f"Unregistered ObjectState (cascade): scope={key}") + # Fire callbacks for UI binding + cls._fire_unregister_callbacks(key, state) + + if scopes_to_delete: + logger.info(f"Cascade unregistered {len(scopes_to_delete)} ObjectState(s) for scope={scope_key}") + # Record single snapshot for cascade unregister (captures all removals at once) + if not _skip_snapshot: + cls.record_snapshot(f"unregister_cascade ({len(scopes_to_delete)} scopes)", scope_key) + + return len(scopes_to_delete) + + @classmethod + def get_by_scope(cls, scope_id: Optional[str]) -> Optional['ObjectState']: + """Get ObjectState by scope_id. + + Args: + scope_id: The scope identifier (e.g., "/path::step_0", or None/"" for global scope). + + Returns: + ObjectState if found, None otherwise. + """ + return cls._states.get(cls._normalize_scope_id(scope_id)) + + @classmethod + def get_all(cls) -> List['ObjectState']: + """Get all registered ObjectStates. + + Returns: + List of all ObjectState instances. Used by LiveContextService.collect(). + """ + return list(cls._states.values()) + + @classmethod + def clear(cls) -> None: + """Clear all registered states. For testing only.""" + cls._states.clear() + logger.debug("Cleared all ObjectStates from registry") + + # ========== TOKEN MANAGEMENT AND CHANGE NOTIFICATION ========== + + _token: int = 0 # Cache invalidation token + _change_callbacks: List[Callable[[], None]] = [] # Change listeners + + @classmethod + def get_token(cls) -> int: + """Get current cache invalidation token.""" + return cls._token + + @classmethod + def increment_token(cls, notify: bool = True) -> None: + """Increment cache invalidation token. + + Args: + notify: If True (default), notify listeners of the change. + Set to False when you need to invalidate caches but will + notify listeners later (e.g., after sibling refresh completes). + """ + cls._token += 1 + if notify: + cls._notify_change() + + @classmethod + def _notify_change(cls) -> None: + """Notify all listeners that something changed. + + UI-agnostic: No PyQt imports. If a callback's object was deleted, + the RuntimeError is caught and the callback is removed. + """ + logger.debug(f"πŸ”” _notify_change: notifying {len(cls._change_callbacks)} listeners") + dead_callbacks = [] + for callback in cls._change_callbacks: + try: + callback() + except RuntimeError as e: + # "wrapped C/C++ object has been deleted" - mark for removal + if "deleted" in str(e).lower(): + logger.debug(f" ⚠️ Callback's object was deleted, removing: {e}") + dead_callbacks.append(callback) + else: + logger.warning(f"Change callback failed: {e}") + except Exception as e: + logger.warning(f"Change callback failed: {e}") + + # Clean up dead callbacks + for cb in dead_callbacks: + cls._change_callbacks.remove(cb) + + @classmethod + def connect_listener(cls, callback: Callable[[], None]) -> None: + """Connect a listener callback that's called on any change. + + The callback should debounce and call collect() to get fresh values. + """ + if callback not in cls._change_callbacks: + cls._change_callbacks.append(callback) + logger.debug(f"Connected change listener: {callback}") + + @classmethod + def disconnect_listener(cls, callback: Callable[[], None]) -> None: + """Disconnect a change listener.""" + if callback in cls._change_callbacks: + cls._change_callbacks.remove(callback) + logger.debug(f"Disconnected change listener: {callback}") + + # ========== ANCESTOR OBJECT COLLECTION ========== + + @classmethod + def get_ancestor_objects(cls, scope_id: Optional[str], use_saved: bool = False) -> List[Any]: + """Get objects from this scope and all ancestors, leastβ†’most specific. + + Replaces LiveContextService.collect() + merge_ancestor_values() for simpler + context stack building. + + Args: + scope_id: The scope to get ancestors for (e.g., "/plate::step_0") + use_saved: If True, return saved baseline (object_instance) instead of + live state (to_object()). Used when computing _saved_resolved + to ensure saved baseline only depends on other saved baselines. + + Returns: + List of objects from ancestor scopes, ordered leastβ†’most specific. + Each object is from state.object_instance (saved) or state.to_object() (live). + """ + scope_key = cls._normalize_scope_id(scope_id) + + # Build list of ancestor scope keys (least-specific to most-specific) + # e.g., "/plate::step_0" -> ["", "/plate", "/plate::step_0"] + ancestors = [""] # Global scope always included + if scope_key: + parts = scope_key.split("::") + for i in range(len(parts)): + ancestors.append("::".join(parts[:i+1])) + + # Get objects from ancestor scopes + objects = [] + for ancestor_key in ancestors: + state = cls._states.get(ancestor_key) + if state: + if use_saved: + # Return saved baseline (object_instance is updated in mark_saved) + objects.append(state.object_instance) + else: + # Return live state with current edits + objects.append(state.to_object()) + + return objects + + @classmethod + def get_ancestor_objects_with_scopes(cls, scope_id: Optional[str], use_saved: bool = False) -> List[Tuple[str, Any]]: + """Get (scope_id, object) tuples from this scope and all ancestors. + + Similar to get_ancestor_objects() but includes the scope_id for each object. + Used for provenance tracking to determine which scope provided a resolved value. + + Args: + scope_id: The scope to get ancestors for (e.g., "/plate::step_0") + use_saved: If True, return saved baseline (object_instance) instead of + live state (to_object()). Used when computing _saved_resolved. + + Returns: + List of (scope_id, object) tuples from ancestor scopes, ordered leastβ†’most specific. + """ + scope_key = cls._normalize_scope_id(scope_id) + + # Build list of ancestor scope keys (least-specific to most-specific) + ancestors = [""] # Global scope always included + if scope_key: + parts = scope_key.split("::") + for i in range(len(parts)): + ancestors.append("::".join(parts[:i+1])) + + # Get (scope_id, object) tuples from ancestor scopes + results: List[Tuple[str, Any]] = [] + for ancestor_key in ancestors: + state = cls._states.get(ancestor_key) + if state: + obj = state.object_instance if use_saved else state.to_object() + results.append((ancestor_key, obj)) + + return results + + # ========== SCOPE + TYPE + FIELD AWARE INVALIDATION ========== + + @classmethod + def invalidate_by_type_and_scope( + cls, + scope_id: Optional[str], + changed_type: type, + field_name: str, + invalidate_saved: bool = False + ) -> None: + """Invalidate a specific field in states that could inherit from changed_type at scope_id. + + PERFORMANCE: Three-tier filtering: + 1. SCOPE: State must be at or below changed scope (descendants inherit) + 2. TYPE: State's type tree must include changed_type + 3. FIELD: Only invalidate the specific field that changed + + If changing GlobalPipelineConfig.napari_streaming_config.window_size: + - Only states with napari_streaming_config in their tree + - Only the 'window_size' field is invalidated, not all 20+ fields + - Steps without napari_streaming_config are NOT touched + + Args: + scope_id: The scope that changed (None/"" for global scope) + changed_type: The type of the ObjectState that was modified + field_name: The specific field that changed + invalidate_saved: If True, also invalidate saved_resolved cache for descendants + """ + from openhcs.config_framework.lazy_factory import get_base_type_for_lazy + from openhcs.config_framework.dual_axis_resolver import invalidate_mro_cache_for_field + + # PERFORMANCE: Targeted cache invalidation - only clear entries for this field/type + invalidate_mro_cache_for_field(changed_type, field_name) + + changed_scope = cls._normalize_scope_id(scope_id) + + # Normalize to base type for comparison (LazyX β†’ X) + base_changed_type = get_base_type_for_lazy(changed_type) or changed_type + + # DEBUG: Log invalidation for well_filter + if field_name == 'well_filter': + logger.debug(f"πŸ” invalidate_by_type_and_scope: scope={changed_scope!r}, type={base_changed_type.__name__}, field={field_name}, total_states={len(cls._states)}") + + for state in cls._states.values(): + state_scope = cls._normalize_scope_id(state.scope_id) + + # SCOPE CHECK: must be at or below changed scope + # Global scope (empty string) affects ALL states + if changed_scope == "": + # Global scope - always a descendant (or self if also global) + if field_name == 'well_filter': + logger.debug(f"πŸ” Checking state: scope={state_scope!r}, obj_type={type(state.object_instance).__name__}") + logger.debug(f"[SCOPE] Global change affects state scope={state_scope!r}") + else: + # Non-global: check exact match or descendant + is_self = (state_scope == changed_scope) + prefix = changed_scope + "::" + is_descendant = state_scope.startswith(prefix) + if not (is_self or is_descendant): + logger.debug(f"[SCOPE] SKIP: changed_scope={changed_scope!r} does not affect state_scope={state_scope!r}") + continue + logger.debug(f"[SCOPE] MATCH: changed_scope={changed_scope!r} affects state_scope={state_scope!r}") + + # TYPE + FIELD CHECK: find matching nested state and invalidate field + cls._invalidate_field_in_matching_states(state, base_changed_type, field_name, invalidate_saved) + + @classmethod + def _invalidate_field_in_matching_states( + cls, + state: 'ObjectState', + target_base_type: type, + field_name: str, + invalidate_saved: bool = False + ) -> None: + """Find fields in state that could inherit from target_base_type and invalidate them. + + With flat storage, we scan _path_to_type to find all dotted paths whose + container type matches or inherits from target_base_type. + + A field should be invalidated if: + 1. Its container type matches target_base_type exactly, OR + 2. Its container type inherits from target_base_type (has target_base_type in MRO) + + This handles sibling inheritance: when WellFilterConfig.well_filter changes, + both 'well_filter_config.well_filter' and 'step_well_filter_config.well_filter' + are invalidated (since StepWellFilterConfig inherits from WellFilterConfig). + + Args: + state: ObjectState to check + target_base_type: Normalized base type to match + field_name: Field to invalidate + invalidate_saved: If True, also invalidate saved_resolved cache for this field + """ + from openhcs.config_framework.lazy_factory import get_base_type_for_lazy + + invalidated_paths: set[str] = set() + + # Scan _path_to_type for matching container types + for dotted_path, container_type in state._path_to_type.items(): + # Normalize container type + container_base_type = get_base_type_for_lazy(container_type) or container_type + + # Check if target_base_type is in the MRO (container inherits the field) + type_matches = False + for mro_class in container_base_type.__mro__: + mro_base = get_base_type_for_lazy(mro_class) or mro_class + if mro_base == target_base_type: + type_matches = True + break + + # If type matches and path ends with the field_name, invalidate it + if type_matches and (dotted_path.endswith(f'.{field_name}') or dotted_path == field_name): + if dotted_path in state.parameters: + state.invalidate_field(dotted_path) + invalidated_paths.add(dotted_path) + + # If invalidating saved baseline, remove from saved_resolved so it recomputes + if invalidate_saved and dotted_path in state._saved_resolved: + del state._saved_resolved[dotted_path] + logger.debug(f"Invalidated saved_resolved cache for {dotted_path}") + + # Trigger recompute immediately to detect if resolved values actually changed. + # This ensures callbacks fire only when values change, not just when fields are invalidated. + # Prevents false flashes when Reset is clicked on already-reset fields. + if invalidated_paths: + state._ensure_live_resolved(notify_flash=True) + + # If we invalidated saved baseline, recompute it now with fresh ancestor values + if invalidate_saved: + # Recompute entire saved_resolved snapshot to pick up new ancestor saved values + old_saved_resolved = state._saved_resolved + state._saved_resolved = state._compute_resolved_snapshot(use_saved=True) + logger.debug(f"Recomputed saved_resolved baseline after invalidation") + + if old_saved_resolved != state._saved_resolved: + state._sync_materialized_state() + else: + state._sync_materialized_state() + + # ========== REGISTRY-LEVEL TIME-TRAVEL (DAG Model) ========== + + # Snapshots: Dict of all snapshots by ID (the DAG - snapshots are NEVER deleted) + # Each Snapshot contains id, timestamp, label, triggering_scope, parent_id, all_states + # The parent_id forms the DAG edges + _snapshots: Dict[str, Snapshot] = {} + + # Current head: None = at branch head (live), snapshot_id = time-traveling to that snapshot + _current_head: Optional[str] = None + + _history_enabled: bool = True + _max_history_size: int = 1000 # Max snapshots in DAG (increased since we don't truncate branches) + _in_time_travel: bool = False # Flag for PFM to know to refresh widget values + + # Limbo: ObjectStates temporarily removed during time-travel + # When traveling to a snapshot, ObjectStates not in that snapshot are moved here. + # When traveling forward or to head, they're restored from here. + _time_travel_limbo: Dict[str, 'ObjectState'] = {} + + # Timelines (branches): Named branches that point to a head snapshot + # "main" is always created automatically on first snapshot + # head_id always points to a valid key in _snapshots (guaranteed by construction) + _timelines: Dict[str, Timeline] = {} + _current_timeline: str = "main" + + @classmethod + def get_branch_history(cls, branch_name: Optional[str] = None) -> List[Snapshot]: + """Get ordered history for a branch by walking parent_id chain. + + Args: + branch_name: Branch to get history for. If None, uses current branch. + + Returns: + List of snapshots from oldest to newest (root first, head last). + Index 0 = oldest (root), index -1 = newest (head). + Empty list if branch has no snapshots yet. + """ + if branch_name is None: + branch_name = cls._current_timeline + + if branch_name not in cls._timelines: + return [] + + head_id = cls._timelines[branch_name].head_id + history = [] + current_id: Optional[str] = head_id + + while current_id is not None: + snapshot = cls._snapshots[current_id] # KeyError if bug + history.append(snapshot) + current_id = snapshot.parent_id + + history.reverse() # [oldest, ..., newest] - natural timeline order + return history + + @classmethod + def get_current_snapshot_index(cls) -> int: + """Get current position as index into branch history. + + Returns: + -1 if at head (live), else index into get_branch_history() list. + Index 0 = oldest, len-1 = head. + """ + if cls._current_head is None: + return -1 + + history = cls.get_branch_history() + for i, snapshot in enumerate(history): + if snapshot.id == cls._current_head: + return i + + logger.error(f"⏱️ BRANCH: current_head {cls._current_head} not in branch history") + return -1 + + @classmethod + def record_snapshot(cls, label: str = "", scope_id: Optional[str] = None) -> None: + """Record current state of ALL ObjectStates to history. + + Called automatically after significant state changes. + Each snapshot captures the full system state at a point in time. + + On FIRST edit, automatically records a baseline "init" snapshot first, + so users can always go back to the original state before any edits. + + Args: + label: Human-readable label (e.g., "edit group_by", "save") + scope_id: Optional scope that triggered the snapshot (for labeling) + """ + if not cls._history_enabled: + return + + # CRITICAL: Don't record snapshots during time-travel + if cls._in_time_travel: + return + + import time + + # FIRST EDIT: Record baseline "init" snapshot before the actual edit snapshot + is_first_snapshot = len(cls._snapshots) == 0 + if is_first_snapshot and label.startswith("edit"): + cls._record_snapshot_internal("init", time.time(), None) + + full_label = f"{label} [{scope_id}]" if scope_id else label + cls._record_snapshot_internal(full_label, time.time(), scope_id) + + @classmethod + def _record_snapshot_internal(cls, label: str, timestamp: float, triggering_scope: Optional[str]) -> None: + """Internal method to record a snapshot without baseline logic. + + DAG Model: + - Snapshots are added to _snapshots dict (never deleted) + - If _current_head is not None (time-traveling), we're diverging + - On diverge: create auto-branch for old future, new snapshot parents from _current_head + - Branch head_id is updated to point to new snapshot + """ + import uuid + + # Capture ALL registered ObjectStates as StateSnapshot dataclasses + all_states: Dict[str, StateSnapshot] = {} + for key, state in cls._states.items(): + all_states[key] = StateSnapshot( + saved_resolved=copy.deepcopy(state._saved_resolved), + live_resolved=copy.deepcopy(state._live_resolved) if state._live_resolved else {}, + parameters=copy.deepcopy(state.parameters), + provenance=copy.deepcopy(state._live_provenance), + ) + + # Determine parent_id for new snapshot + if cls._current_head is not None: + # We're in the past - diverging from _current_head + parent_id = cls._current_head + + # Auto-branch: preserve old future before we diverge + # The old branch head becomes an auto-saved branch + if cls._current_timeline in cls._timelines: + old_head_id = cls._timelines[cls._current_timeline].head_id + if old_head_id != cls._current_head: + # There IS an old future to preserve + branch_name = f"auto-{old_head_id[:8]}" + if branch_name not in cls._timelines: + cls._timelines[branch_name] = Timeline( + name=branch_name, + head_id=old_head_id, + base_id=cls._current_head, + description=f"Auto-saved from diverge at {cls._current_head[:8]}", + ) + old_snapshot = cls._snapshots[old_head_id] + logger.info(f"⏱️ AUTO-BRANCH: Created '{branch_name}' (was {old_snapshot.label})") + + # Clear limbo when diverging + cls._time_travel_limbo.clear() + else: + # At branch head - parent is current branch's head (if exists) + if cls._current_timeline in cls._timelines: + parent_id = cls._timelines[cls._current_timeline].head_id + else: + parent_id = None + + # Create new snapshot + snapshot = Snapshot( + id=str(uuid.uuid4()), + timestamp=timestamp, + label=label, + triggering_scope=triggering_scope, + parent_id=parent_id, + all_states=all_states, + ) + + # Add to DAG + cls._snapshots[snapshot.id] = snapshot + + # Update or create current branch to point to new snapshot + if cls._current_timeline not in cls._timelines: + cls._timelines[cls._current_timeline] = Timeline( + name=cls._current_timeline, + head_id=snapshot.id, + base_id=snapshot.id, + description="Main timeline" if cls._current_timeline == "main" else "", + ) + logger.info(f"⏱️ BRANCH: Created '{cls._current_timeline}' timeline") + else: + cls._timelines[cls._current_timeline].head_id = snapshot.id + + # Return to head (live state) - we're no longer time-traveling + cls._current_head = None + + # Enforce max DAG size (prune oldest snapshots not referenced by any branch) + if len(cls._snapshots) > cls._max_history_size: + cls._prune_unreachable_snapshots() + + logger.debug(f"⏱️ SNAPSHOT: Recorded '{label}' (id={snapshot.id[:8]})") + cls._fire_history_changed_callbacks() + + @classmethod + def _prune_unreachable_snapshots(cls) -> None: + """Remove snapshots not reachable from any branch head. + + Walks from each branch head to find all reachable snapshots, + then removes any that aren't reachable. + """ + reachable: Set[str] = set() + + for timeline in cls._timelines.values(): + current_id: Optional[str] = timeline.head_id + while current_id is not None and current_id not in reachable: + reachable.add(current_id) + snapshot = cls._snapshots.get(current_id) + current_id = snapshot.parent_id if snapshot else None + + # Remove unreachable + unreachable = set(cls._snapshots.keys()) - reachable + for snapshot_id in unreachable: + del cls._snapshots[snapshot_id] + + if unreachable: + logger.debug(f"⏱️ PRUNE: Removed {len(unreachable)} unreachable snapshots") + + @classmethod + def time_travel_to_snapshot(cls, snapshot_id: str) -> bool: + """Travel ALL ObjectStates to a specific snapshot by ID. + + Full time-travel: ObjectStates are registered/unregistered to match the snapshot. + ObjectStates not in the snapshot are moved to limbo. ObjectStates in snapshot + but not in registry are restored from limbo. + + Args: + snapshot_id: UUID of snapshot to travel to + + Returns: + True if travel succeeded. + """ + if snapshot_id not in cls._snapshots: + logger.error(f"⏱️ TIME_TRAVEL: Snapshot {snapshot_id} not found") + return False + + snapshot = cls._snapshots[snapshot_id] + cls._current_head = snapshot_id + + # Set flag so PFM knows to refresh widget values + cls._in_time_travel = True + try: + snapshot_scopes = set(snapshot.all_states.keys()) + current_scopes = set(cls._states.keys()) + + # PHASE 1: UNREGISTER ObjectStates not in snapshot (move to limbo) + scopes_to_limbo = current_scopes - snapshot_scopes + for scope_key in scopes_to_limbo: + state = cls._states.pop(scope_key) + cls._time_travel_limbo[scope_key] = state + cls._fire_unregister_callbacks(scope_key, state) + logger.debug(f"⏱️ TIME_TRAVEL: Moved to limbo: {scope_key}") + + # PHASE 2: RE-REGISTER ObjectStates from limbo + scopes_to_register = snapshot_scopes - current_scopes + for scope_key in scopes_to_register: + state = cls._time_travel_limbo.pop(scope_key) # KeyError if bug + cls._states[scope_key] = state + cls._fire_register_callbacks(scope_key, state) + logger.debug(f"⏱️ TIME_TRAVEL: Restored from limbo: {scope_key}") + + # PHASE 3: RESTORE state for all ObjectStates in snapshot + for scope_key, state_snap in snapshot.all_states.items(): + state = cls._states.get(scope_key) + if not state: + continue + + current_params = state.parameters.copy() if state.parameters else {} + target_live = state_snap.live_resolved + current_live = state._live_resolved.copy() if state._live_resolved else {} + + # Find changed resolved values + changed_paths = set() + all_keys = set(target_live.keys()) | set(current_live.keys()) + for key in all_keys: + if target_live.get(key) != current_live.get(key): + changed_paths.add(key) + + # Find changed raw parameters + all_param_keys = set(state_snap.parameters.keys()) | set(current_params.keys()) + for param_key in all_param_keys: + before = current_params.get(param_key) + after = state_snap.parameters.get(param_key) + if before != after and not (is_dataclass(before) or is_dataclass(after)): + changed_paths.add(param_key) + + # RESTORE state + state._saved_resolved = copy.deepcopy(state_snap.saved_resolved) + state._live_resolved = copy.deepcopy(state_snap.live_resolved) + state._live_provenance = copy.deepcopy(state_snap.provenance) + state.parameters = copy.deepcopy(state_snap.parameters) + state._sync_materialized_state() + + # Notify UI + if changed_paths and state._on_resolved_changed_callbacks: + for callback in state._on_resolved_changed_callbacks: + callback(changed_paths) + + # PHASE 4: Fire time-travel completion callbacks + if cls._on_time_travel_complete_callbacks: + dirty_states = [ + (scope_key, state) + for scope_key, state in cls._states.items() + if state.dirty_fields + ] + if dirty_states: + logger.debug(f"⏱️ TIME_TRAVEL: {len(dirty_states)} dirty states") + for callback in cls._on_time_travel_complete_callbacks: + callback(dirty_states, snapshot.triggering_scope) + finally: + cls._in_time_travel = False + + cls._fire_history_changed_callbacks() + logger.info(f"⏱️ TIME_TRAVEL: Traveled to {snapshot.label} ({snapshot_id[:8]})") + return True + + @classmethod + def time_travel_to(cls, index: int) -> bool: + """Travel to a snapshot by index in current branch history. + + Convenience method for UI sliders. Uses get_branch_history() to map + index to snapshot_id. + + Args: + index: Index into branch history (0 = oldest/root, -1 = newest/head) + + Returns: + True if travel succeeded. + """ + history = cls.get_branch_history() + if not history: + logger.warning("⏱️ TIME_TRAVEL: No history") + return False + + # Normalize negative index + if index < 0: + index = len(history) + index + + if index < 0 or index >= len(history): + logger.warning(f"⏱️ TIME_TRAVEL: Index {index} out of range [0, {len(history) - 1}]") + return False + + # Index len-1 = head (newest), returning to head means exit time-travel + if index == len(history) - 1: + return cls.time_travel_to_head() + + snapshot = history[index] + return cls.time_travel_to_snapshot(snapshot.id) + + @classmethod + def time_travel_back(cls) -> bool: + """Travel one step back in history (toward older/lower index).""" + history = cls.get_branch_history() + if not history: + return False + + current_index = cls.get_current_snapshot_index() + if current_index == -1: + # At head (len-1) - go one step back + if len(history) < 2: + return False + return cls.time_travel_to_snapshot(history[-2].id) + + # Already time-traveling - go one step older (lower index) + if current_index <= 0: + return False # Already at oldest + return cls.time_travel_to_snapshot(history[current_index - 1].id) + + @classmethod + def time_travel_forward(cls) -> bool: + """Travel one step forward in history (toward newer/higher index).""" + if cls._current_head is None: + return False # Already at head + + history = cls.get_branch_history() + current_index = cls.get_current_snapshot_index() + + # Go one step toward head (higher index) + next_index = current_index + 1 + if next_index >= len(history) - 1: + # At or past head - return to head + return cls.time_travel_to_head() + + return cls.time_travel_to_snapshot(history[next_index].id) + + @classmethod + def time_travel_to_head(cls) -> bool: + """Return to the latest state (exit time-travel mode). + + Restores state from current branch's head snapshot. + """ + if cls._current_head is None: + return True # Already at head + + if cls._current_timeline not in cls._timelines: + logger.warning("⏱️ TIME_TRAVEL: No current timeline") + return False + + head_id = cls._timelines[cls._current_timeline].head_id + result = cls.time_travel_to_snapshot(head_id) + cls._current_head = None # Mark as at head (not time-traveling) + return result + + @classmethod + def get_history_info(cls) -> List[Dict[str, Any]]: + """Get human-readable history for UI display. + + Returns history for current branch, oldest first (index 0 = oldest, -1 = head). + """ + import datetime + history = cls.get_branch_history() + current_index = cls.get_current_snapshot_index() + head_index = len(history) - 1 + + result = [] + for i, snapshot in enumerate(history): + is_head = (i == head_index) + is_current = (current_index == -1 and is_head) or (i == current_index) + result.append({ + 'index': i, + 'id': snapshot.id, + 'timestamp': datetime.datetime.fromtimestamp(snapshot.timestamp).strftime('%H:%M:%S.%f')[:-3], + 'label': snapshot.label or f"Snapshot #{i}", + 'is_current': is_current, + 'is_head': is_head, + 'num_states': len(snapshot.all_states), + 'parent_id': snapshot.parent_id, + }) + return result + + @classmethod + def get_history_length(cls) -> int: + """Get number of snapshots in current branch history.""" + return len(cls.get_branch_history()) + + @classmethod + def is_time_traveling(cls) -> bool: + """Check if currently viewing historical state (not at head).""" + return cls._current_head is not None + + # ========== BRANCH OPERATIONS ========== + + @classmethod + def create_branch(cls, name: str, description: str = "") -> Timeline: + """Create a new branch at current position. + + Args: + name: Branch name (must be unique) + description: Optional description + + Returns: + The created Timeline + """ + # Branch from current position + if cls._current_head is not None: + # Time-traveling - branch from current position + head_id = cls._current_head + elif cls._current_timeline in cls._timelines: + # At head of current branch + head_id = cls._timelines[cls._current_timeline].head_id + else: + logger.error("⏱️ BRANCH: No snapshots to branch from") + raise ValueError("No snapshots to branch from") + + timeline = Timeline( + name=name, + head_id=head_id, + base_id=head_id, + description=description, + ) + cls._timelines[name] = timeline + logger.info(f"⏱️ BRANCH: Created branch '{name}' at {head_id[:8]}") + return timeline + + @classmethod + def switch_branch(cls, name: str) -> bool: + """Switch to a different branch. + + Args: + name: Branch name + + Returns: + True if switch succeeded + """ + timeline = cls._timelines[name] # KeyError if branch doesn't exist + + # Switch to branch and travel to its head + cls._current_timeline = name + result = cls.time_travel_to_snapshot(timeline.head_id) + cls._current_head = None # At head of new branch + cls._fire_history_changed_callbacks() + logger.info(f"⏱️ BRANCH: Switched to '{name}'") + return result + + @classmethod + def delete_branch(cls, name: str) -> None: + """Delete a branch. + + Args: + name: Branch name to delete + + Note: Snapshots are not deleted - they may be reachable from other branches. + Unreachable snapshots are pruned automatically when DAG exceeds max size. + """ + del cls._timelines[name] # KeyError if branch doesn't exist + logger.info(f"⏱️ BRANCH: Deleted branch '{name}'") + cls._fire_history_changed_callbacks() + + @classmethod + def list_branches(cls) -> List[Dict[str, Any]]: + """List all branches. + + Returns: + List of dicts with branch info + """ + return [ + { + 'name': tl.name, + 'head_id': tl.head_id, + 'base_id': tl.base_id, + 'description': tl.description, + 'is_current': tl.name == cls._current_timeline, + } + for tl in cls._timelines.values() + ] + + @classmethod + def get_current_branch(cls) -> str: + """Get current branch name.""" + return cls._current_timeline + + # ========== PERSISTENCE ========== + + @classmethod + def export_history_to_dict(cls) -> Dict[str, Any]: + """Export history to a JSON-serializable dict. + + Returns: + Dict with 'snapshots', 'timelines', 'current_head', 'current_timeline'. + """ + return { + 'snapshots': {sid: snap.to_dict() for sid, snap in cls._snapshots.items()}, + 'timelines': [tl.to_dict() for tl in cls._timelines.values()], + 'current_head': cls._current_head, + 'current_timeline': cls._current_timeline, + } + + @classmethod + def import_history_from_dict(cls, data: Dict[str, Any]) -> None: + """Import history from a dict (e.g., loaded from JSON). + + Only imports state data for scope_ids that currently exist in the registry. + Scopes in the snapshot but not in the app are skipped. + + Args: + data: Dict with 'snapshots', 'timelines', 'current_head', 'current_timeline'. + """ + cls._snapshots.clear() + cls._timelines.clear() + current_scopes = set(cls._states.keys()) + + # Handle both old list format and new dict format + snapshots_data = data['snapshots'] + if isinstance(snapshots_data, list): + # Old format: list of snapshots + snapshot_items = [(s['id'], s) for s in snapshots_data] + else: + # New format: dict of id -> snapshot + snapshot_items = snapshots_data.items() + + for _snapshot_id, snapshot_data in snapshot_items: + # Filter to only scopes that exist in current registry + filtered_states: Dict[str, StateSnapshot] = {} + for scope_id, state_data in snapshot_data['states'].items(): + if scope_id in current_scopes: + filtered_states[scope_id] = StateSnapshot( + saved_resolved=state_data['saved_resolved'], + live_resolved=state_data['live_resolved'], + parameters=state_data['parameters'], + provenance=state_data['provenance'], + ) + + snapshot = Snapshot( + id=snapshot_data['id'], + timestamp=snapshot_data['timestamp'], + label=snapshot_data['label'], + triggering_scope=snapshot_data.get('triggering_scope'), + parent_id=snapshot_data.get('parent_id'), + all_states=filtered_states, + ) + cls._snapshots[snapshot.id] = snapshot + + # Import timelines + if 'timelines' in data: + for tl_data in data['timelines']: + tl = Timeline.from_dict(tl_data) + cls._timelines[tl.name] = tl + cls._current_timeline = data.get('current_timeline', 'main') + else: + cls._current_timeline = 'main' + + # Handle both old index format and new head format + if 'current_head' in data: + cls._current_head = data['current_head'] + elif 'current_index' in data: + # Old format - convert index to head + # Can't reliably convert, just go to head + cls._current_head = None + else: + cls._current_head = None + + @classmethod + def save_history_to_file(cls, filepath: str) -> None: + """Save history to a JSON file. + + Args: + filepath: Path to save the JSON file. + """ + import json + data = cls.export_history_to_dict() + with open(filepath, 'w') as f: + json.dump(data, f, indent=2) + logger.info(f"⏱️ Saved {len(cls._snapshots)} snapshots to {filepath}") + + @classmethod + def load_history_from_file(cls, filepath: str) -> None: + """Load history from a JSON file. + + Args: + filepath: Path to the JSON file. + """ + import json + with open(filepath, 'r') as f: + data = json.load(f) + cls.import_history_from_dict(data) + logger.info(f"⏱️ Loaded {len(cls._snapshots)} snapshots from {filepath}") + + diff --git a/openhcs/pyqt_gui/app.py b/openhcs/pyqt_gui/app.py index 58cd2eaee..d8b201f33 100644 --- a/openhcs/pyqt_gui/app.py +++ b/openhcs/pyqt_gui/app.py @@ -90,7 +90,8 @@ def init_function_registry_background(): # This was missing and caused placeholder resolution to fall back to static defaults from openhcs.config_framework.global_config import set_global_config_for_editing from openhcs.config_framework.lazy_factory import ensure_global_config_context - from openhcs.config_framework.object_state import ObjectState, ObjectStateRegistry + from openhcs.config_framework.object_state import ObjectState + from openhcs.config_framework.object_state_registry import ObjectStateRegistry from openhcs.core.config import GlobalPipelineConfig # Set for editing (UI placeholders) - this uses threading.local() storage diff --git a/openhcs/pyqt_gui/widgets/function_pane.py b/openhcs/pyqt_gui/widgets/function_pane.py index 161ec6701..4157b983b 100644 --- a/openhcs/pyqt_gui/widgets/function_pane.py +++ b/openhcs/pyqt_gui/widgets/function_pane.py @@ -287,7 +287,8 @@ def create_parameter_form(self) -> QWidget: # - To keep each function pane only as tall as its content, we explicitly # disable the inner scroll area and let the outer FunctionListWidget # handle scrolling for long forms. - from openhcs.config_framework.object_state import ObjectState, ObjectStateRegistry + from openhcs.config_framework.object_state import ObjectState + from openhcs.config_framework.object_state_registry import ObjectStateRegistry from openhcs.pyqt_gui.widgets.shared.services.scope_token_service import ScopeTokenService # Build function-specific scope: step_scope::func_N @@ -325,7 +326,7 @@ def create_parameter_form(self) -> QWidget: def cleanup_object_state(self) -> None: """Unregister ObjectState on widget destruction.""" - from openhcs.config_framework.object_state import ObjectStateRegistry + from openhcs.config_framework.object_state_registry import ObjectStateRegistry if hasattr(self, '_func_state') and self._func_state: ObjectStateRegistry.unregister(self._func_state) self._func_state = None diff --git a/openhcs/pyqt_gui/widgets/step_parameter_editor.py b/openhcs/pyqt_gui/widgets/step_parameter_editor.py index 8c248a405..bc411c949 100644 --- a/openhcs/pyqt_gui/widgets/step_parameter_editor.py +++ b/openhcs/pyqt_gui/widgets/step_parameter_editor.py @@ -110,7 +110,8 @@ def __init__(self, step: FunctionStep, service_adapter=None, color_scheme: Optio # The step is the overlay (what's being edited), not the parent context # Context hierarchy: GlobalPipelineConfig (thread-local) -> PipelineConfig (context_obj) -> Step (overlay) from openhcs.pyqt_gui.widgets.shared.parameter_form_manager import FormManagerConfig - from openhcs.config_framework.object_state import ObjectState, ObjectStateRegistry + from openhcs.config_framework.object_state import ObjectState + from openhcs.config_framework.object_state_registry import ObjectStateRegistry # Look up ObjectState from registry using scope_id # ObjectState MUST be registered by PipelineEditorWidget when step was added @@ -309,7 +310,7 @@ def setup_ui(self): # Header label (stored for scope accent styling) self.header_label = QLabel("Step Parameters") - self.header_label.setStyleSheet(f"color: {self.color_scheme.to_hex(self.color_scheme.text_accent)}; font-weight: bold; font-size: 14px;") + self.header_label.setObjectName("section_header") header_layout.addWidget(self.header_label) header_layout.addStretch() diff --git a/openhcs/pyqt_gui/windows/config_window.py b/openhcs/pyqt_gui/windows/config_window.py index 31a1785ee..eb48c5dc4 100644 --- a/openhcs/pyqt_gui/windows/config_window.py +++ b/openhcs/pyqt_gui/windows/config_window.py @@ -29,7 +29,7 @@ from openhcs.core.config import GlobalPipelineConfig, PipelineConfig from openhcs.config_framework import is_global_config_type from openhcs.ui.shared.code_editor_form_updater import CodeEditorFormUpdater -from openhcs.config_framework.object_state import ObjectStateRegistry +from openhcs.config_framework.object_state_registry import ObjectStateRegistry # ❌ REMOVED: require_config_context decorator - enhanced decorator events system handles context automatically from openhcs.core.lazy_placeholder_simplified import LazyDefaultPlaceholderService @@ -112,7 +112,8 @@ def __init__(self, config_class: Type, current_config: Any, # This fixes the circular context bug where reset showed old values instead of global defaults # Create or lookup ObjectState from registry - callers own state directly - from openhcs.config_framework.object_state import ObjectState, ObjectStateRegistry + from openhcs.config_framework.object_state import ObjectState + from openhcs.config_framework.object_state_registry import ObjectStateRegistry self.state = ObjectStateRegistry.get_by_scope(self.scope_id) if self.state is None: self.state = ObjectState( @@ -479,7 +480,7 @@ def save_config(self, *, close_window=True): # CRITICAL: Invalidate ALL descendant caches so they re-resolve with the new SAVED thread-local # This is necessary when saving None values - descendants must pick up the new None # instead of continuing to use cached values resolved from the old saved thread-local - from openhcs.config_framework.object_state import ObjectStateRegistry + from openhcs.config_framework.object_state_registry import ObjectStateRegistry ObjectStateRegistry.increment_token(notify=True) logger.debug(f"Invalidated all descendant caches after updating SAVED thread-local") diff --git a/plans/rot_elimination/00_MASTER_PLAN.md b/plans/rot_elimination/00_MASTER_PLAN.md new file mode 100644 index 000000000..0db16aca4 --- /dev/null +++ b/plans/rot_elimination/00_MASTER_PLAN.md @@ -0,0 +1,196 @@ +# Rot Elimination Master Plan + +## Overview + +9 plans to eliminate ~3500 lines of rot. No wrappers. No backwards compatibility. + +## ❌ PROJECT-LEVEL ANTIPATTERNS TO AVOID + +**DO NOT add "temporary" compatibility during migration:** +```python +# ❌ WRONG: "We'll remove this later" +def old_api(): + warnings.warn("Deprecated, use new_api()", DeprecationWarning) + return new_api() # DON'T - there is no "later" +``` +Delete old API. Update callers. One PR. No deprecation period. + +**DO NOT create parallel implementations "for safety":** +```python +# ❌ WRONG: Keep both paths +if USE_NEW_SYSTEM: + result = new_executor.execute(plan) +else: + result = step.process(context) # DON'T KEEP OLD PATH +``` +One path. New system only. Old path deleted. + +**DO NOT squash multiple plans into one commit:** +``` +# ❌ WRONG: One commit for everything +commit: "Implement streaming unification + message protocol + executor extraction" +``` +One plan per commit. Test after each. Can revert individual commits if broken. + +Multiple plans in one PR is fine β€” just use separate commits. + +**DO NOT add feature flags for gradual rollout:** +```python +# ❌ WRONG: Feature flag +ENABLE_TYPED_PLANS = os.environ.get('ENABLE_TYPED_PLANS', False) +if ENABLE_TYPED_PLANS: + plan = StepPlan(...) # DON'T +else: + plan = {'input_dir': ...} +``` +No flags. New system is THE system. Ship it or don't. + +**DO NOT "clean up later":** +```python +# ❌ WRONG: TODO for future +# TODO: Remove this once all callers are migrated +def legacy_helper(): # DON'T LEAVE THESE + ... +``` +If it's not deleted in this PR, it never gets deleted. Clean up NOW. + +**DO NOT add tests for deprecated behavior:** +```python +# ❌ WRONG: Testing the old way still works +def test_legacy_callback_system_still_works(): + ObjectStateRegistry.add_register_callback(...) # DON'T TEST DELETED CODE +``` +Tests verify NEW behavior. Old behavior doesn't exist. + +**The principle: Every PR should make the codebase strictly simpler. If line count goes up, you're doing it wrong.** + +## Dependency Graph + +```mermaid +flowchart TD + subgraph Independent["Independent (Can Start Immediately)"] + PSYGNAL[plan_01_psygnal_migration] + STYLE[plan_01_centralized_styling] + CONFIG[plan_01_eliminate_config_merging] + SERIAL[plan_01_python_source_serializer] + DIM[plan_03_dimension_abstraction] + end + + subgraph Streaming["Streaming (Sequential)"] + STREAM1[plan_01_streaming_backend_unification] + STREAM2[plan_02_streaming_message_protocol] + STREAM1 --> STREAM2 + end + + subgraph Compiler["Compiler (Sequential)"] + COMP[plan_01_generic_compiler_spec] + EXEC[plan_02_step_executor_extraction] + COMP --> EXEC + end + + style PSYGNAL fill:#51cf66 + style STYLE fill:#51cf66 + style CONFIG fill:#51cf66 + style SERIAL fill:#51cf66 + style DIM fill:#51cf66 + style STREAM1 fill:#339af0 + style STREAM2 fill:#339af0 + style COMP fill:#ff6b6b + style EXEC fill:#ff6b6b +``` + +## Execution Order + +### Wave 1: Independent Plans (Parallel) +These have no dependencies and can be implemented in any order: + +| Plan | Lines Saved | Risk | Effort | +|------|-------------|------|--------| +| `plan_01_psygnal_migration` | ~120 | Low | 2h | +| `plan_01_centralized_styling` | ~200 | Low | 4h | +| `plan_01_eliminate_config_merging` | ~90 | Low | 1h | +| `plan_01_python_source_serializer` | ~950 | Low | 4h | +| `plan_03_dimension_abstraction` | ~400 | Low | 3h | + +### Wave 2: Streaming Plans (Sequential) +Must be done in order: + +| Plan | Lines Saved | Risk | Effort | +|------|-------------|------|--------| +| `plan_01_streaming_backend_unification` | ~300 | Low | 3h | +| `plan_02_streaming_message_protocol` | ~100 | Low | 2h | + +### Wave 3: Compiler Plans (Sequential, Highest Risk) +Must be done in order. Touches core execution path. + +| Plan | Lines Saved | Risk | Effort | +|------|-------------|------|--------| +| `plan_01_generic_compiler_spec` | ~200 | Medium | 6h | +| `plan_02_step_executor_extraction` | ~647 | Medium | 8h | + +## Testing Strategy + +### End-to-End Test (Required) + +Every plan must pass this before merge: + +```bash +pytest tests/integration/test_main.py::test_main[disk-ImageXpress-3d-multiprocessing-direct-none-none] -v +``` + +This is THE integration test. If it passes, core execution works. + +### Per-Plan Testing + +1. **E2E Test** β€” Above command must pass +2. **Unit Tests** β€” Run in terminal, don't save to files +3. **Manual Testing** β€” User tests UI/streaming where applicable + +### What Needs Manual Testing (by user) + +| Area | Why Manual | +|------|------------| +| Streaming to Napari | Requires running viewer | +| Streaming to Fiji | Requires Fiji installation | +| UI Styling | Visual verification | +| Flash animations | Visual verification | +| Config window behavior | Interactive testing | + +### Regression Watchlist + +| Area | What to Watch | How to Test | +|------|---------------|-------------| +| Execution | Steps actually run | E2E test above | +| Streaming | Data reaches viewers | Manual by user | +| Config | Resolution works | E2E test covers basic resolution | +| UI | Styling looks right | Manual by user | + +## Success Criteria + +- [ ] All 9 plans implemented +- [ ] ~3500 lines deleted +- [ ] Zero new wrappers or compatibility shims +- [ ] All existing tests pass +- [ ] Manual smoke test passes +- [ ] No regressions in core functionality + +## Risk Mitigation + +1. **Compiler plans are highest risk** β€” Do them last, after all other plans prove the pattern works +2. **Test after each plan** β€” Don't batch multiple plans before testing +3. **Revert fast** β€” If a plan causes regressions, revert and reassess + +## Plan Summaries + +| Plan | What It Does | +|------|--------------| +| `plan_01_psygnal_migration` | Replace 8 callback lists with psygnal Signals | +| `plan_01_centralized_styling` | Move 227 setStyleSheet calls to app-level stylesheet | +| `plan_01_eliminate_config_merging` | Delete 90 lines that duplicate config framework | +| `plan_01_python_source_serializer` | Replace 950-line pickle_to_python with type-dispatched formatters | +| `plan_03_dimension_abstraction` | Replace 103 channel/slice/frame references with HyperstackDimensions | +| `plan_01_streaming_backend_unification` | Merge Napari/Fiji backends into one ABC | +| `plan_02_streaming_message_protocol` | Replace stringly-typed JSON with typed dataclasses | +| `plan_01_generic_compiler_spec` | Replace Dict[str, Any] plans with typed frozen StepPlan | +| `plan_02_step_executor_extraction` | Move 647-line process() to dedicated StepExecutor | + diff --git a/plans/rot_elimination/plan_01_centralized_styling.md b/plans/rot_elimination/plan_01_centralized_styling.md new file mode 100644 index 000000000..5f9447baf --- /dev/null +++ b/plans/rot_elimination/plan_01_centralized_styling.md @@ -0,0 +1,194 @@ +# plan_01_centralized_styling.md +## Component: PyQt Centralized Styling + +### Objective + +Eliminate 227 scattered `setStyleSheet()` calls and 25+ `StyleSheetGenerator` instantiations. Widgets should declare WHAT they are (via object names), not HOW they look (via style strings). + +### The Rot + +```python +# 25+ widgets do this: +class MyWidget(QWidget): + def __init__(self, color_scheme, ...): + self.style_generator = StyleSheetGenerator(self.color_scheme) # ❌ Duplicate + + def setup_ui(self): + btn.setStyleSheet(self.style_generator.generate_button_style()) # ❌ Imperative + label.setStyleSheet(f"color: {self.color_scheme.to_hex(...)};") # ❌ Inline +``` + +**Counts:** +- 227 `setStyleSheet()` calls across pyqt_gui/ +- 25+ `StyleSheetGenerator` instantiations +- 2+ duplicate `_get_button_style()` methods + +### The Insight + +Qt stylesheets cascade like CSS. Application-level stylesheet applies to ALL widgets. Widget-level `setStyleSheet()` is only needed for: +1. **Dynamic state** β€” e.g., flash animations, error highlighting +2. **Variant styling** β€” e.g., primary vs secondary buttons + +Everything else should be in the application stylesheet. + +### Existing Infrastructure (Underutilized) + +```python +# openhcs/pyqt_gui/shared/palette_manager.py +class PaletteManager: + def __init__(self): + self.style_generator = StyleSheetGenerator(self.color_scheme) # βœ… ONE instance + +# openhcs/pyqt_gui/shared/style_generator.py +def generate_complete_application_style(self) -> str: + """Generate complete application-wide QStyleSheet.""" # βœ… Already exists +``` + +### Plan + +**Step 1: Audit and Extend Application Stylesheet** + +The `generate_complete_application_style()` exists but doesn't cover all widget types. + +1. Grep all `setStyleSheet()` calls +2. Categorize: which are static (can move to app stylesheet) vs dynamic (must stay) +3. Add missing widget type rules to `generate_complete_application_style()` + +**Step 2: Introduce Object Name Convention** + +For variant styling, widgets declare their role: + +```python +# Widget code (declaration only): +btn.setObjectName("primary_button") +btn.setObjectName("secondary_button") +btn.setObjectName("danger_button") + +# Application stylesheet (styling only): +QPushButton#primary_button { background-color: ...; } +QPushButton#secondary_button { background-color: ...; } +QPushButton#danger_button { background-color: ...; } +``` + +**Step 3: Remove Widget-Level StyleSheetGenerator** + +For each widget that creates its own `StyleSheetGenerator`: +1. DELETE `self.style_generator = StyleSheetGenerator(...)` +2. DELETE all static `setStyleSheet()` calls +3. Widget inherits from application stylesheet automatically + +**Step 4: Keep Dynamic Styling Where Needed** + +Some `setStyleSheet()` calls are legitimate: +- Flash animations (scope colors, dirty indicators) +- Error states (red borders) +- Runtime-computed styles + +These stay, but should use a centralized helper: + +```python +# Instead of inline style strings: +widget.setStyleSheet(f"border: 2px solid {color};") + +# Use centralized helper: +from openhcs.pyqt_gui.shared.style_helpers import apply_flash_border +apply_flash_border(widget, scope_color) +``` + +### Qt Stylesheet Limitations (Acknowledged) + +Qt stylesheets have quirks: +- Specificity rules differ from CSS +- Some properties don't cascade (e.g., `font` on QGroupBox) +- Pseudo-states (`:hover`, `:pressed`) can be finicky + +**Mitigation:** Test each widget type after migration. If Qt fights back, document the exception and keep minimal widget-level styling. + +### Cleanup β€” DELETE ALL OF THIS + +**From 25+ widget files:** +```python +# DELETE all of these patterns: +self.style_gen = StyleSheetGenerator(self.color_scheme) +self.style_generator = StyleSheetGenerator(...) +from openhcs.pyqt_gui.shared.style_generator import StyleSheetGenerator # (if unused after) + +# DELETE static setStyleSheet calls: +btn.setStyleSheet(self.style_generator.generate_button_style()) +label.setStyleSheet(f"color: {self.color_scheme.to_hex(...)};") +``` + +**DELETE duplicate methods:** +```python +def _get_button_style(self): # function_list_editor.py +def _get_button_style(self): # step_parameter_editor.py +``` + +**Estimated deletion:** ~200 lines of scattered styling code + +**No wrappers. No backwards compatibility.** +- If a widget looks wrong after migration, fix the application stylesheet +- Don't add widget-level styling back + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT create a StyleManager singleton that widgets call:** +```python +# ❌ WRONG: Centralized but still imperative +class StyleManager: + @staticmethod + def get_button_style() -> str: ... + +# Widget still calls it: +btn.setStyleSheet(StyleManager.get_button_style()) # DON'T +``` +Widget code should have ZERO style strings. App stylesheet handles it. + +**DO NOT create helper methods for "common patterns":** +```python +# ❌ WRONG: Helper method +def apply_standard_button_style(btn: QPushButton): + btn.setStyleSheet(...) # DON'T + +# Widget code: +apply_standard_button_style(self.btn) # DON'T +``` +Use object names instead. Widget declares `btn.setObjectName("primary")`, app stylesheet has `QPushButton#primary { ... }`. + +**DO NOT keep "special case" setStyleSheet calls:** +```python +# ❌ WRONG: "Just this one special case" +# Application stylesheet handles most buttons, but THIS one is special: +special_btn.setStyleSheet(f"background: {special_color};") # DON'T +``` +If it's a variant, use object names. If it's truly dynamic (flash animation), use the flash system. + +**DO NOT create style "themes" or "presets" as method calls:** +```python +# ❌ WRONG: Theme presets +btn.apply_theme("dark") # DON'T +btn.set_style_preset("compact") # DON'T +``` +These are the same imperative pattern. Use object names + app stylesheet. + +**EXCEPTION: Dynamic styling IS allowed for:** +- Flash animations (scope colors, dirty indicators) β€” use centralized flash system +- Error states that change at runtime β€” use helper like `apply_error_border(widget)` +- These are the ONLY legitimate setStyleSheet calls in widget code + +### Files to Modify + +**Extend:** +- `openhcs/pyqt_gui/shared/style_generator.py` β€” Add missing widget type rules + +**Clean up (remove StyleSheetGenerator):** +- `openhcs/pyqt_gui/dialogs/custom_function_manager_dialog.py` +- `openhcs/pyqt_gui/dialogs/function_selector_dialog.py` +- `openhcs/pyqt_gui/widgets/shared/abstract_table_browser.py` +- `openhcs/pyqt_gui/widgets/shared/column_filter_widget.py` +- `openhcs/pyqt_gui/widgets/shared/zmq_server_manager.py` +- ... and 20+ more + +### Implementation Draft + +*(Only after smell loop passes)* diff --git a/plans/rot_elimination/plan_01_eliminate_config_merging.md b/plans/rot_elimination/plan_01_eliminate_config_merging.md new file mode 100644 index 000000000..7a4ad48d6 --- /dev/null +++ b/plans/rot_elimination/plan_01_eliminate_config_merging.md @@ -0,0 +1,150 @@ +# plan_01_eliminate_config_merging.md +## Component: Eliminate Orchestrator Config Merging + +### Objective +Delete `_merge_nested_dataclass()` and `_create_merged_config()` from orchestrator.py. These 90 lines of imperative config merging code duplicate what the config framework's dual-axis resolver already does declaratively. + +### Findings + +**Current Rot (orchestrator.py lines 72-161):** +```python +def _merge_nested_dataclass(pipeline_value, global_value): + # 36 lines of imperative field-by-field merging + for field in dataclass_fields(type(pipeline_value)): + raw_pipeline_field = pipeline_value.__dict__.get(field.name) + if raw_pipeline_field is not None: + # more imperative logic... + +def _create_merged_config(pipeline_config, global_config): + # 49 lines of imperative merging with lazy config handling + for field in fields(GlobalPipelineConfig): + pipeline_value = pipeline_config.__dict__.get(field.name) + if pipeline_value is not None: + if hasattr(pipeline_value, 'to_base_config'): + # more special casing... +``` + +**The Config Framework Already Solves This:** + +`config_context()` + lazy `__getattribute__` resolution: +```python +with config_context(pipeline_config): # Pushes to context stack + # ANY access to a lazy field automatically: + # 1. Checks instance value (if not None, return it) + # 2. Walks context stack from most to least specific + # 3. Walks MRO for sibling inheritance + # 4. Returns first non-None value found + value = step.path_planning_config.well_filter # Resolved automatically +``` + +**Why the Duplication Exists:** +The orchestrator was written before the config framework's dual-axis resolver was complete. It manually merges configs to create a "materialized" GlobalPipelineConfig. But this materialization is unnecessary β€” lazy resolution handles it. + +### Plan + +**Phase 1: Audit Call Sites** + +Find all usages of `_create_merged_config`: +```python +# orchestrator.py +self._effective_config = _create_merged_config(...) # in apply_pipeline_config() +``` + +**Phase 2: Replace with config_context()** + +Instead of eagerly materializing a merged config: +```python +# BEFORE (imperative) +def apply_pipeline_config(self, pipeline_config): + self._effective_config = _create_merged_config(pipeline_config, self.global_config) + +def get_effective_config(self): + return self._effective_config +``` + +```python +# AFTER (declarative) +def apply_pipeline_config(self, pipeline_config): + self._pipeline_config = pipeline_config + # No merging. Config framework handles it lazily. + +def get_effective_config(self): + # Just wrap access in config_context β€” lazy resolution does the rest + with config_context(self._pipeline_config): + return get_current_temp_global() +``` + +**Phase 3: Delete Dead Code** + +Delete both functions: +- `_merge_nested_dataclass()` (lines 72-108) +- `_create_merged_config()` (lines 111-160) + +~90 lines removed. + +### Structural Properties + +- **No imperative merging** β€” Config merging is declarative via context stack +- **No field-by-field loops** β€” MRO resolution handles inheritance +- **No special-casing lazy vs non-lazy** β€” Lazy `__getattribute__` handles both +- **Single source of truth** β€” Config framework is THE config resolution system + +### Risk Assessment + +**Low risk** β€” The config framework's resolver is battle-tested (used by all UI resolution). The orchestrator's merging logic was a workaround for pre-framework code. + +### Cleanup β€” DELETE ALL OF THIS + +**Functions to DELETE from `orchestrator.py`:** +```python +def _merge_nested_dataclass(...) # DELETE (lines 72-108) +def _create_merged_config(...) # DELETE (lines 111-160) +``` + +**No wrappers. No backwards compatibility.** +- Replace merge calls with `with config_context(step_config): ...` +- Field access inside context returns resolved values via lazy resolution +- If something doesn't resolve correctly, fix the config hierarchy β€” don't add special-case merging + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT create a "simplified" merge helper:** +```python +# ❌ WRONG: Wrapper around config_context +def merge_configs(global_config, step_config): + with config_context(step_config): + return get_current_temp_global() # DON'T CREATE WRAPPER +``` +Just use `config_context` directly. No wrapper needed. + +**DO NOT add "fallback" merging for edge cases:** +```python +# ❌ WRONG: Fallback to old behavior +with config_context(step_config): + value = getattr(config, field) + if value is None: + value = _merge_nested_dataclass(...) # DON'T KEEP OLD LOGIC +``` +If resolution returns None, that's the correct value. Fix the config hierarchy, not the resolver. + +**DO NOT create a MergedConfig class:** +```python +# ❌ WRONG: Wrapper class +class MergedConfig: + def __init__(self, global_config, step_config): + self._merged = _create_merged_config(...) # DON'T +``` +Use the existing lazy dataclass resolution. No new classes. + +**DO NOT add field-by-field copying logic:** +```python +# ❌ WRONG: Manual field copying +for field in fields(step_config): + if getattr(step_config, field.name) is not None: + setattr(result, field.name, ...) # DON'T +``` +This is the exact rot we're deleting. Lazy resolution handles this. + +### Implementation Draft + +(After smell loop approval) diff --git a/plans/rot_elimination/plan_01_generic_compiler_spec.md b/plans/rot_elimination/plan_01_generic_compiler_spec.md new file mode 100644 index 000000000..85d9e37b7 --- /dev/null +++ b/plans/rot_elimination/plan_01_generic_compiler_spec.md @@ -0,0 +1,389 @@ +# plan_01_generic_compiler_spec.md +## Component: Declarative Compilation Phase System + +### Objective + +Transform OpenHCS compilation from imperative dict mutation into **declarative phase composition** following the patterns already cracked in the codebase: + +- `@auto_create_decorator` β†’ generates decorator β†’ generates classes β†’ injects fields +- `@numpy`, `@torch` β†’ function declares contract β†’ framework uses metadata +- `AutoRegisterMeta` β†’ class declares key β†’ auto-registers in global registry + +The compilation system is the LAST piece still imperative. Fixing it completes the platform. + +### The Pattern + +Every cracked system in OpenHCS follows: + +``` +DECLARATION β†’ INTROSPECTION β†’ DERIVATION β†’ EXECUTION +``` + +| System | Declaration | Introspection | Derivation | +|--------|-------------|---------------|------------| +| Config | Dataclass with defaults | `@auto_create_decorator` | Lazy class, field injection | +| Memory | `@torch` decorator | `func.input_memory_type` | Auto-conversion | +| Registry | `_microscope_type = 'imagexpress'` | `AutoRegisterMeta` | Auto-registration | +| **Compilation** | **Resolver methods** | **`__init_subclass__`** | **Schema + compile()** | + +### The Disease + +Current compilation is imperative mutation: + +```python +# CURRENT (Imperative - Wrong) +class PathPlanner: + def plan_step(self, step, context): + plan = {} + plan['input_dir'] = self._resolve_input_dir(step, context) # Imperative + plan['output_dir'] = self._resolve_output_dir(step, context) # Imperative + plan['func'] = step.func # Imperative + return plan # Dict[str, Any] - stringly typed garbage +``` + +No contracts. No composition. No derivation. + +### Plan + +#### 1. CompilationPhase ABC with Introspection + +```python +from abc import ABC, abstractmethod +from dataclasses import make_dataclass, field +from typing import get_type_hints, Callable, Dict, Type +import inspect + +class CompilationPhase(ABC): + """ABC that introspects resolver methods and derives provides/compile(). + + Following @dataclass pattern: you declare fields, framework derives __init__. + Here: you declare resolvers, framework derives provides + compile(). + + ABC, not Protocol. MANIFESTO Section V: "A contract you must detect is not a contract." + """ + + # Derived by __init_subclass__ from resolver methods + _resolvers: Dict[str, Callable] + provides: Dict[str, Type] + + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + + # Introspect resolver methods + cls._resolvers = {} + cls.provides = {} + + for name, method in inspect.getmembers(cls, predicate=inspect.isfunction): + if name.startswith('_') or name in ('compile', 'validate'): + continue + + sig = inspect.signature(method) + params = list(sig.parameters.values()) + + # Resolver signature: (self, step: AbstractStep, context: ProcessingContext) -> T + if len(params) >= 3 and sig.return_annotation != inspect.Signature.empty: + cls._resolvers[name] = method + cls.provides[name] = sig.return_annotation + + def compile(self, step: 'AbstractStep', plan: 'StepPlan', context: 'ProcessingContext') -> 'StepPlan': + """Auto-generated: calls all resolvers and returns updated plan.""" + from dataclasses import replace + updates = {} + for field_name, resolver in self._resolvers.items(): + updates[field_name] = resolver(self, step, context) + return replace(plan, **updates) +``` + +#### 2. Phases Declare Resolvers as Methods + +```python +class PathPlanningPhase(CompilationPhase): + """Phase declares resolvers. Framework introspects and wires. + + Each method with signature (step, context) -> T becomes a field resolver. + Method name = field name. Return annotation = field type. + """ + + def input_dir(self, step: AbstractStep, context: ProcessingContext) -> Path: + if step.pipeline_position == 0: + return context.input_dir + prev_plan = context.step_plans[step.pipeline_position - 1] + return prev_plan.output_dir + + def output_dir(self, step: AbstractStep, context: ProcessingContext) -> Path: + return self._build_output_path(step, context) + + def func(self, step: AbstractStep, context: ProcessingContext) -> FuncPattern: + return step.func + + def special_inputs(self, step: AbstractStep, context: ProcessingContext) -> Dict[str, Path]: + return self._resolve_special_inputs(step, context) + + def special_outputs(self, step: AbstractStep, context: ProcessingContext) -> OrderedDict[str, Path]: + return self._resolve_special_outputs(step, context) + + # Helper methods (prefixed with _) are NOT resolvers + def _build_output_path(self, step, context) -> Path: + ... + + +class MaterializationPhase(CompilationPhase): + """Determines storage backends for each step.""" + + def read_backend(self, step: AbstractStep, context: ProcessingContext) -> Backend: + if step.pipeline_position == 0: + return Backend.DISK + return context.step_plans[step.pipeline_position - 1].write_backend + + def write_backend(self, step: AbstractStep, context: ProcessingContext) -> Backend: + if self._needs_materialization(step, context): + return Backend.ZARR + return Backend.MEMORY + + +class MemoryContractPhase(CompilationPhase): + """Validates and plans memory type conversions.""" + + def input_memory_type(self, step: AbstractStep, context: ProcessingContext) -> MemoryType: + func = context.step_plans[step.pipeline_position].func + return getattr(func, 'input_memory_type', MemoryType.NUMPY) + + def output_memory_type(self, step: AbstractStep, context: ProcessingContext) -> MemoryType: + func = context.step_plans[step.pipeline_position].func + return getattr(func, 'output_memory_type', MemoryType.NUMPY) + + +class GPUAssignmentPhase(CompilationPhase): + """Assigns GPU resources for GPU memory types.""" + + def gpu_id(self, step: AbstractStep, context: ProcessingContext) -> Optional[int]: + plan = context.step_plans[step.pipeline_position] + if plan.input_memory_type in GPU_MEMORY_TYPES: + return self._assign_gpu(context) + return None +``` + +#### 3. StepPlan Schema Derived from All Phases + +```python +# Phase registry - populated by __init_subclass__ or decorator +COMPILATION_PHASES: List[Type[CompilationPhase]] = [ + PathPlanningPhase, + MaterializationPhase, + MemoryContractPhase, + GPUAssignmentPhase, +] + +def derive_step_plan_schema() -> Type: + """Derive StepPlan dataclass from all registered phases. + + Following @auto_create_decorator pattern: schema is DERIVED, not hardcoded. + Adding a new phase automatically extends the schema. + """ + all_fields = [ + # Base fields always present + ('step_name', str, field(default='')), + ('step_type', str, field(default='')), + ('axis_id', str, field(default='')), + ('pipeline_position', int, field(default=0)), + ] + + # Collect fields from all phases + for phase_cls in COMPILATION_PHASES: + for field_name, field_type in phase_cls.provides.items(): + # Optional because not all phases may have run yet + all_fields.append((field_name, Optional[field_type], field(default=None))) + + return make_dataclass('StepPlan', all_fields, frozen=True) + +# Generated at import time +StepPlan = derive_step_plan_schema() +``` + +#### 4. Compiler Executes Phases in Order + +```python +class PipelineCompiler: + """Compiles pipeline by executing phases in order. + + Each phase transforms StepPlan, adding its provided fields. + Final StepPlan has all fields from all phases. + """ + + phases: List[CompilationPhase] = [phase_cls() for phase_cls in COMPILATION_PHASES] + + def compile(self, steps: List[AbstractStep], context: ProcessingContext) -> List[StepPlan]: + plans = [] + + for i, step in enumerate(steps): + # Initialize with base fields + plan = StepPlan( + step_name=step.name, + step_type=step.__class__.__name__, + axis_id=context.axis_id, + pipeline_position=i, + ) + + # Each phase adds its fields + for phase in self.phases: + plan = phase.compile(step, plan, context) + + plans.append(plan) + context.step_plans[i] = plan # Inject for next step's resolvers + + return plans +``` + +#### 5. Steps Execute Against Typed StepPlan + +```python +class FunctionStep(AbstractStep): + def process(self, context: ProcessingContext, step_index: int) -> None: + plan: StepPlan = context.step_plans[step_index] + + # Typed access - no more plan.get('func') or plan['input_dir'] + func = plan.func + input_dir = plan.input_dir + input_memory_type = plan.input_memory_type + gpu_id = plan.gpu_id + + # Execute with typed plan + ... +``` + +### OpenHCS as a Spec + +This pattern makes OpenHCS a **spec for dimensional dataflow compilers**: + +| Domain | Dimensions | Phases | +|--------|------------|--------| +| HCS | well, channel, site, z, time | PathPlanning, Materialization, Memory, GPU | +| Genomics | sample, lane, read, barcode | PathPlanning, ResourceAllocation, Parallelization | +| Astronomy | field, exposure, filter, chip | PathPlanning, Storage, Calibration | + +The platform provides: +- `CompilationPhase` ABC with introspection +- `derive_step_plan_schema()` for runtime schema generation +- Frozen context for thread-safe execution + +The domain provides: +- Dimension definitions +- Processing functions with contracts +- Domain-specific phases + +### Findings + +| Phase | Provides (derived from resolver methods) | +|-------|------------------------------------------| +| PathPlanningPhase | input_dir, output_dir, func, special_inputs, special_outputs, funcplan | +| MaterializationPhase | read_backend, write_backend, materialized_backend | +| MemoryContractPhase | input_memory_type, output_memory_type | +| GPUAssignmentPhase | gpu_id | + +**The `func` redundancy is eliminated.** PathPlanningPhase stores it once. MemoryContractPhase reads from the existing plan. + +### The Cascade + +``` +CompilationPhase subclass defined + β†’ __init_subclass__ introspects resolver methods + β†’ Derives provides = {method_name: return_type, ...} + β†’ Generates compile() that calls all resolvers + +At import time: + β†’ derive_step_plan_schema() collects all phase.provides + β†’ make_dataclass creates StepPlan with typed fields + β†’ StepPlan is frozen dataclass, not Dict[str, Any] + +At compile time: + β†’ Compiler iterates phases in order + β†’ Each phase.compile(step, plan, context) returns updated plan + β†’ Final StepPlan is typed, validated, frozen +``` + +### Cleanup β€” DELETE ALL OF THIS + +**Code to DELETE from `path_planning.py`, `materialization.py`, etc.:** +```python +def plan_step(self, step, context): + plan = {} + plan['input_dir'] = self._resolve_input_dir(...) # DELETE imperative dict mutation + plan['output_dir'] = self._resolve_output_dir(...) + return plan # DELETE Dict[str, Any] return +``` + +**Patterns to DELETE:** +- All `plan = {}` dict initialization β†’ replaced by typed dataclass construction +- All `plan['key'] = value` mutations β†’ replaced by resolver method declarations +- All `Dict[str, Any]` return types β†’ replaced by frozen `StepPlan` dataclass + +**No wrappers. No backwards compatibility.** +- `StepPlan` is a frozen dataclass, not `Dict[str, Any]` +- Code that accesses `plan['key']` β†’ fails loud, update to `plan.key` +- Code that mutates plan after creation β†’ fails loud, plans are frozen + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT keep Dict[str, Any] with TypedDict hints:** +```python +# ❌ WRONG: TypedDict is still a dict +class StepPlanDict(TypedDict): + input_dir: Path + output_dir: Path + func: Callable +``` +Use frozen dataclass. TypedDict is runtime-mutable and has no frozen semantics. + +**DO NOT add __getitem__ for dict-style access:** +```python +# ❌ WRONG: Backwards-compatible dict access +@dataclass(frozen=True) +class StepPlan: + input_dir: Path + + def __getitem__(self, key): + return getattr(self, key) # DON'T ADD THIS +``` +Attribute access only. `plan.input_dir`, not `plan['input_dir']`. + +**DO NOT create mutable intermediate plans:** +```python +# ❌ WRONG: Mutable during compilation +plan = StepPlan() # mutable +for phase in phases: + phase.compile(plan) # mutates +plan.freeze() # DON'T +``` +Each phase returns a NEW frozen plan. Immutable by construction. + +**DO NOT create abstract compile() methods for phases:** +```python +# ❌ WRONG: Abstract method per phase +class PathPlanningPhase(CompilationPhase): + @abstractmethod + def compile(self, step, plan, context): ... # DON'T +``` +Phases declare resolver METHODS (e.g., `resolve_input_dir()`). `__init_subclass__` derives `compile()`. No abstract `compile()`. + +**DO NOT create phase subclasses per step type:** +```python +# ❌ WRONG: Per-step phase classes +class FunctionStepPathPlanningPhase(PathPlanningPhase): ... +class CustomStepPathPlanningPhase(PathPlanningPhase): ... +``` +ONE phase class per concern. Dispatch on step attributes, not step type. + +**DO NOT store `func` in multiple phases:** +```python +# ❌ WRONG: Redundant storage +class PathPlanningPhase: + provides = ['input_dir', 'func'] # func here + +class MemoryContractPhase: + provides = ['memory_type', 'func'] # func here too - DON'T +``` +`func` is in PathPlanningPhase. Later phases read from existing plan. + +### Implementation Draft + +*Awaiting smell loop approval.* diff --git a/plans/rot_elimination/plan_01_psygnal_migration.md b/plans/rot_elimination/plan_01_psygnal_migration.md new file mode 100644 index 000000000..30ad25c37 --- /dev/null +++ b/plans/rot_elimination/plan_01_psygnal_migration.md @@ -0,0 +1,236 @@ +# plan_01_psygnal_migration.md +## Component: Signal Infrastructure Unification + +### Objective + +Replace hand-rolled callback lists in ObjectStateRegistry with psygnal Signals. This is a surgical migration β€” not a rewrite of all Qt signals. The rot is specifically in `object_state.py` where we maintain 8 callback lists with 24 boilerplate methods. + +### The Rot + +```python +# openhcs/config_framework/object_state.py β€” 8 callback lists, 24 methods + +# CLASS-LEVEL (ObjectStateRegistry) β€” 5 lists +_on_register_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] +_on_unregister_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] +_on_time_travel_complete_callbacks: List[Callable[[List[Tuple], Optional[str]], None]] = [] +_on_history_changed_callbacks: List[Callable[[], None]] = [] +_change_callbacks: List[Callable[[], None]] = [] + +# INSTANCE-LEVEL (ObjectState) β€” 3 lists per instance +_on_state_changed_callbacks: List[Callable[[], None]] = [] +_on_resolved_changed_callbacks: List[Callable[[Set[str]], None]] = [] +_on_time_travel_callbacks: List[Callable[[], None]] = [] + +# Each list requires 3 methods: +# add_*_callback(), remove_*_callback(), _fire_*_callbacks() +# Plus manual dead callback detection: if "deleted" in str(e).lower() +``` + +### The Solution: psygnal + +```python +from psygnal import Signal + +class ObjectStateRegistry: + # Class-level signals (replace 5 callback lists) + registered = Signal(str, object) # (scope_id, ObjectState) + unregistered = Signal(str, object) # (scope_id, ObjectState) + history_changed = Signal() # no args + time_travel_complete = Signal(list, str) # (dirty_states, triggering_scope) + changed = Signal() # token incremented + +class ObjectState: + # Instance-level signals (replace 3 callback lists) + resolved_changed = Signal(set) # Set[str] changed paths + state_changed = Signal() # dirty/sig-diff changed + time_traveled = Signal() # restored via time-travel +``` + +**What psygnal provides:** +- `.connect(callback)` β€” subscribe +- `.disconnect(callback)` β€” unsubscribe +- `.emit(*args)` β€” fire +- Automatic weak reference cleanup (no more `if "deleted" in str(e).lower()`) +- Type-checked arguments +- Thread safety + +### Caller Enumeration (Complete) + +**ObjectStateRegistry class-level signals:** + +| Signal | Caller | File | Line | +|--------|--------|------|------| +| `registered` | `AbstractManagerWidget._on_registry_register` | `abstract_manager_widget.py` | 452 | +| `unregistered` | `AbstractManagerWidget._on_registry_unregister` | `abstract_manager_widget.py` | 451 | +| `unregistered` | `MainWindow._on_object_state_unregistered` | `main.py` | 591 | +| `time_travel_complete` | `MainWindow._on_time_travel_complete` | `main.py` | 587 | +| `history_changed` | `TimeTravelWidget._update_ui` | `time_travel_widget.py` | 49 | +| `history_changed` | `SnapshotBrowserWindow._on_refresh` | `snapshot_browser_window.py` | 128 | +| `changed` | `ParameterFormManager._on_live_context_changed` | `parameter_form_manager.py` | 267 | + +**ObjectState instance-level signals:** + +| Signal | Caller | File | Line | +|--------|--------|------|------| +| `resolved_changed` | `ParameterFormManager._on_resolved_values_changed` | `parameter_form_manager.py` | 328 | +| `resolved_changed` | `FunctionListEditor.on_resolved_changed` | `function_list_editor.py` | 749 | +| `state_changed` | `ParameterFormManager._on_state_changed` | `parameter_form_manager.py` | 335 | +| `state_changed` | `ConfigHierarchyTree.on_state_changed` | `config_hierarchy_tree.py` | 206 | + +### Plan + +**Step 1: Add psygnal dependency** +```bash +uv add psygnal +``` + +**Step 2: Replace ObjectStateRegistry class-level signals** + +Before: +```python +_on_register_callbacks: List[Callable[[str, 'ObjectState'], None]] = [] + +@classmethod +def add_register_callback(cls, callback): + if callback not in cls._on_register_callbacks: + cls._on_register_callbacks.append(callback) + +@classmethod +def _fire_register_callbacks(cls, scope_key, state): + for callback in cls._on_register_callbacks: + try: + callback(scope_key, state) + except Exception as e: + logger.warning(f"Error in register callback: {e}") +``` + +After: +```python +registered = Signal(str, object) # (scope_id, ObjectState) + +# In register(): +cls.registered.emit(key, state) +``` + +**Step 3: Update callers** + +Before: +```python +ObjectStateRegistry.add_unregister_callback(self._on_registry_unregister) +``` + +After: +```python +ObjectStateRegistry.unregistered.connect(self._on_registry_unregister) +``` + +**Step 4: Replace ObjectState instance-level signals** + +Before: +```python +self._on_resolved_changed_callbacks: List[Callable[[Set[str]], None]] = [] + +def on_resolved_changed(self, callback): + if callback not in self._on_resolved_changed_callbacks: + self._on_resolved_changed_callbacks.append(callback) +``` + +After: +```python +self.resolved_changed = Signal(set) + +# Callers: +state.resolved_changed.connect(self._on_resolved_values_changed) +``` + +### Cleanup β€” DELETE ALL OF THIS + +**From `ObjectStateRegistry` (class-level):** +```python +# DELETE these 5 lists: +_on_register_callbacks: List[...] +_on_unregister_callbacks: List[...] +_on_time_travel_complete_callbacks: List[...] +_on_history_changed_callbacks: List[...] +_change_callbacks: List[...] + +# DELETE these 15 methods: +add_register_callback(), remove_register_callback(), _fire_register_callbacks() +add_unregister_callback(), remove_unregister_callback(), _fire_unregister_callbacks() +add_time_travel_complete_callback(), remove_time_travel_complete_callback() +add_history_changed_callback(), remove_history_changed_callback(), _fire_history_changed_callbacks() +connect_listener(), disconnect_listener(), _fire_change_callbacks() +``` + +**From `ObjectState` (instance-level):** +```python +# DELETE these 3 lists: +_on_state_changed_callbacks: List[...] +_on_resolved_changed_callbacks: List[...] +_on_time_travel_callbacks: List[...] + +# DELETE these 9 methods: +on_state_changed(), off_state_changed(), _notify_state_changed() +on_resolved_changed(), off_resolved_changed() +on_time_travel(), off_time_travel() +``` + +**Estimated deletion:** ~120 lines of boilerplate + +**No wrappers. No backwards compatibility.** +- All `add_*_callback()` β†’ `.connect()` +- All `remove_*_callback()` β†’ `.disconnect()` +- All `_fire_*_callbacks()` β†’ `.emit()` +- If a caller uses old API, it fails loud β€” update the caller + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT create wrapper methods:** +```python +# ❌ WRONG: Wrapper that calls the signal +def add_register_callback(cls, callback): + cls.registered.connect(callback) # DON'T DO THIS +``` +Callers must use `.connect()` directly. Delete the wrapper entirely. + +**DO NOT keep old lists "for compatibility":** +```python +# ❌ WRONG: Keeping both systems +registered = Signal(str, object) +_on_register_callbacks: List[...] = [] # DON'T KEEP THIS +``` +Delete the lists. There is no transition period. + +**DO NOT create adapter classes:** +```python +# ❌ WRONG: Adapter pattern +class SignalAdapter: + def __init__(self, signal): + self.signal = signal + def add_callback(self, cb): + self.signal.connect(cb) +``` +This is the same boilerplate with extra steps. Delete it. + +**DO NOT add try/except for "gradual migration":** +```python +# ❌ WRONG: Fallback to old system +try: + ObjectStateRegistry.registered.connect(callback) +except AttributeError: + ObjectStateRegistry.add_register_callback(callback) # DON'T +``` +There is no gradual migration. Old API fails loud. Fix the caller. + +### Out of Scope + +- Qt `pyqtSignal` in GUI widgets β€” stays as-is (already Qt-coupled) +- `GlobalEventBus` β€” stays as-is (already Qt-coupled) +- `CustomFunctionSignals` β€” stays as-is (already Qt-coupled) + +This plan is surgical: only `object_state.py` and its 11 callers. + +### Implementation Draft + +*(Only after smell loop passes)* diff --git a/plans/rot_elimination/plan_01_python_source_serializer.md b/plans/rot_elimination/plan_01_python_source_serializer.md new file mode 100644 index 000000000..4414716ed --- /dev/null +++ b/plans/rot_elimination/plan_01_python_source_serializer.md @@ -0,0 +1,395 @@ +# plan_01_python_source_serializer.md +## Component: Declarative Python Source Serializer + +### Objective + +Replace 1142 lines of copy-pasted imperative code in `pickle_to_python.py` with a declarative serialization framework. Types declare how they serialize. System derives the rest. + +### The Rot + +**7 public functions doing the same dance:** +```python +def generate_complete_function_pattern_code(...) # 45 lines +def generate_clean_dataclass_repr(...) # 90 lines +def generate_readable_function_repr(...) # 95 lines +def generate_step_code(...) # 12 lines (wrapper) +def generate_complete_pipeline_steps_code(...) # 86 lines +def generate_complete_orchestrator_code(...) # 300+ lines +def generate_config_code(...) # 35 lines +``` + +**Every function repeats:** +1. `all_function_imports = defaultdict(set)` β€” import collection +2. `all_enum_imports = defaultdict(set)` β€” more import collection +3. Walk data structure manually +4. Handle special cases (Enum, dataclass, callable, lazy dataclass, Path, list, dict) +5. Two-pass generation (collect imports β†’ resolve collisions β†’ regenerate) +6. Build code string with f-strings + +**Evidence of rot:** +```python +# String matching for type detection (line 260) +elif is_dataclass(value) and 'Lazy' in value.__class__.__name__: + +# Same import collection in 4+ functions +all_function_imports = defaultdict(set) +all_enum_imports = defaultdict(set) +all_decorated_functions = set() + +# Two-pass pattern copy-pasted everywhere +temp_import_lines, temp_name_mappings = format_imports_as_strings(...) +# ... generate code ... +if final_name_mappings != temp_name_mappings: + # regenerate everything +``` + +### The Insight + +This is **serialization**. Converting Python objects to Python source code. + +The cracked pattern: +1. **Type β†’ Formatter dispatch** β€” Each type knows how to serialize +2. **Import collection is automatic** β€” Formatter returns `(code, imports)` +3. **One traversal** β€” Walk once, collect imports AND generate code +4. **Collision resolution as post-process** β€” Not interleaved with generation + +### Plan + +#### 1. Define Serialization Result + +```python +@dataclass(frozen=True) +class SourceFragment: + """Result of serializing a value to Python source.""" + code: str + imports: FrozenSet[Tuple[str, str]] # (module, name) pairs + + def __add__(self, other: 'SourceFragment') -> 'SourceFragment': + return SourceFragment( + code=self.code + other.code, + imports=self.imports | other.imports + ) +``` + +#### 2. Define Formatter Protocol (via ABC) + +```python +class SourceFormatter(ABC): + """Formats a value to Python source code.""" + + @abstractmethod + def can_format(self, value: Any) -> bool: + """Return True if this formatter handles this value type.""" + ... + + @abstractmethod + def format(self, value: Any, context: 'FormatContext') -> SourceFragment: + """Format value to Python source, returning code and required imports.""" + ... +``` + +#### 3. Define Format Context + +```python +@dataclass +class FormatContext: + """Context passed through serialization.""" + indent: int = 0 + clean_mode: bool = False + name_mappings: Dict[Tuple[str, str], str] = field(default_factory=dict) + + def indented(self) -> 'FormatContext': + return replace(self, indent=self.indent + 1) + + @property + def indent_str(self) -> str: + return " " * self.indent +``` + +#### 4. Implement Type-Specific Formatters + +```python +class EnumFormatter(SourceFormatter): + def can_format(self, value: Any) -> bool: + return isinstance(value, Enum) + + def format(self, value: Enum, ctx: FormatContext) -> SourceFragment: + cls = value.__class__ + import_pair = (cls.__module__, cls.__name__) + name = ctx.name_mappings.get(import_pair, cls.__name__) + return SourceFragment( + code=f"{name}.{value.name}", + imports=frozenset([import_pair]) + ) + +class DataclassFormatter(SourceFormatter): + def can_format(self, value: Any) -> bool: + return is_dataclass(value) and not isinstance(value, type) + + def format(self, value: Any, ctx: FormatContext) -> SourceFragment: + cls = value.__class__ + import_pair = (cls.__module__, cls.__name__) + + # Get all field fragments + field_fragments = [] + all_imports = {import_pair} + + for f in fields(value): + field_value = object.__getattribute__(value, f.name) + + # Skip None in clean_mode for lazy dataclasses + if ctx.clean_mode and field_value is None: + continue + + frag = to_source(field_value, ctx.indented()) + all_imports |= frag.imports + field_fragments.append(f"{f.name}={frag.code}") + + if not field_fragments: + return SourceFragment(f"{cls.__name__}()", frozenset(all_imports)) + + inner = f",\n{ctx.indented().indent_str}".join(field_fragments) + code = f"{cls.__name__}(\n{ctx.indented().indent_str}{inner}\n{ctx.indent_str})" + return SourceFragment(code, frozenset(all_imports)) + +class CallableFormatter(SourceFormatter): + def can_format(self, value: Any) -> bool: + return callable(value) and not isinstance(value, type) + + def format(self, value: Callable, ctx: FormatContext) -> SourceFragment: + import_pair = (value.__module__, value.__name__) + name = ctx.name_mappings.get(import_pair, value.__name__) + return SourceFragment(name, frozenset([import_pair])) + +class ListFormatter(SourceFormatter): + def can_format(self, value: Any) -> bool: + return isinstance(value, list) + + def format(self, value: list, ctx: FormatContext) -> SourceFragment: + if not value: + return SourceFragment("[]", frozenset()) + + item_frags = [to_source(item, ctx.indented()) for item in value] + all_imports = frozenset().union(*(f.imports for f in item_frags)) + + inner = f",\n{ctx.indented().indent_str}".join(f.code for f in item_frags) + code = f"[\n{ctx.indented().indent_str}{inner}\n{ctx.indent_str}]" + return SourceFragment(code, all_imports) + +# ... PathFormatter, DictFormatter, StrFormatter, etc. +``` + +#### 5. Formatter Registry with `__init_subclass__` + +```python +class SourceFormatter(ABC): + """Base formatter with auto-registration.""" + + _registry: ClassVar[List['SourceFormatter']] = [] + + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + if not getattr(cls, '__abstract__', False): + SourceFormatter._registry.append(cls()) + + @classmethod + def get_formatter(cls, value: Any) -> 'SourceFormatter': + for formatter in cls._registry: + if formatter.can_format(value): + return formatter + raise TypeError(f"No formatter for {type(value)}") +``` + +#### 6. Single Entry Point + +```python +def to_source(value: Any, ctx: FormatContext = None) -> SourceFragment: + """Convert any value to Python source code.""" + ctx = ctx or FormatContext() + formatter = SourceFormatter.get_formatter(value) + return formatter.format(value, ctx) +``` + +#### 7. Import Resolution as Post-Process + +```python +def resolve_imports(imports: FrozenSet[Tuple[str, str]]) -> Tuple[List[str], Dict]: + """Resolve import collisions and generate import lines.""" + # Group by name to detect collisions + name_to_modules = defaultdict(list) + for module, name in imports: + name_to_modules[name].append(module) + + import_lines = [] + name_mappings = {} + + for module, name in sorted(imports): + if len(name_to_modules[name]) > 1: + # Collision - use qualified alias + alias = f"{name}_{module.split('.')[-1]}" + import_lines.append(f"from {module} import {name} as {alias}") + name_mappings[(module, name)] = alias + else: + import_lines.append(f"from {module} import {name}") + name_mappings[(module, name)] = name + + return import_lines, name_mappings +``` + +#### 8. Public API (ONE Function) + +```python +def generate_python_source(obj: Any, header: str = "", clean_mode: bool = False) -> str: + """Generate complete Python source with imports. + + THIS IS THE ONLY PUBLIC FUNCTION. All legacy functions are DELETED. + """ + # First pass: collect imports + ctx = FormatContext(clean_mode=clean_mode) + fragment = to_source(obj, ctx) + + # Resolve collisions + import_lines, name_mappings = resolve_imports(fragment.imports) + + # Second pass: regenerate with resolved names + ctx = FormatContext(clean_mode=clean_mode, name_mappings=name_mappings) + fragment = to_source(obj, ctx) + + # Combine + code_lines = [] + if header: + code_lines.append(header) + code_lines.append("") + code_lines.extend(import_lines) + code_lines.append("") + code_lines.append(fragment.code) + + return "\n".join(code_lines) +``` + +### Cleanup β€” DELETE ALL OF THIS + +**Functions to DELETE from `pickle_to_python.py` (NO WRAPPERS):** +```python +def generate_complete_function_pattern_code(...) # DELETE +def generate_clean_dataclass_repr(...) # DELETE +def generate_readable_function_repr(...) # DELETE +def generate_step_code(...) # DELETE +def generate_complete_pipeline_steps_code(...) # DELETE +def generate_complete_orchestrator_code(...) # DELETE +def generate_config_code(...) # DELETE +``` + +**Call sites to update:** +Every caller of the deleted functions gets updated to call `generate_python_source()` directly. No wrappers. No shims. No deprecation period. + +### Why This Is Cracked + +| Old (1142 lines) | New (~200 lines) | +|------------------|------------------| +| 7 functions doing same thing | 1 generic `to_source()` | +| Manual type dispatch in each function | `__init_subclass__` auto-registration | +| Import collection repeated everywhere | Returns `SourceFragment` with imports | +| Two-pass interleaved with generation | Two-pass as separate concern | +| String matching `'Lazy' in __name__` | Proper `can_format()` predicates | +| Copy-paste for each context | ONE function, callers pass header | + +**Adding a new type:** Define formatter, done. Auto-registered. + +**Adding a new output context:** Call `generate_python_source()` with different header. + +### Architecture Diagram + +```mermaid +flowchart TD + subgraph Input + OBJ[Any Python Object] + end + + subgraph Core["Serialization Core (~100 lines)"] + TS[to_source] + REG[Formatter Registry] + TS -->|"get_formatter()"| REG + REG -->|dispatch| FMT[Matched Formatter] + FMT -->|"returns"| SF[SourceFragment] + end + + subgraph Formatters["Type Formatters (auto-registered)"] + EF[EnumFormatter] + DF[DataclassFormatter] + CF[CallableFormatter] + LF[ListFormatter] + PF[PathFormatter] + end + + subgraph PostProcess["Import Resolution"] + SF -->|imports| RI[resolve_imports] + RI -->|name_mappings| TS + end + + subgraph Output + GPS[generate_python_source] + GPS -->|calls| TS + GPS -->|produces| CODE[Complete Python Source] + end + + OBJ --> GPS + + REG -.->|contains| EF + REG -.->|contains| DF + REG -.->|contains| CF + REG -.->|contains| LF + REG -.->|contains| PF +``` + +### Dependencies + +None. This is a standalone refactor. + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT keep the old functions as wrappers:** +```python +# ❌ WRONG: Wrapper for backwards compatibility +def generate_pipeline_code(pipeline): + return generate_python_source(pipeline, header="# Pipeline") # DON'T KEEP +``` +Delete all 7 old functions. Callers use `generate_python_source()` directly. + +**DO NOT create separate formatter registries per context:** +```python +# ❌ WRONG: Multiple registries +PIPELINE_FORMATTERS = {} +CONFIG_FORMATTERS = {} +STEP_FORMATTERS = {} +``` +ONE registry. Formatters are context-agnostic. Header varies, formatters don't. + +**DO NOT add can_format() methods that check module paths:** +```python +# ❌ WRONG: String matching on module names +def can_format(self, obj) -> bool: + return 'openhcs.config' in type(obj).__module__ # DON'T +``` +Use `isinstance()` or structural checks. No string matching on module paths. + +**DO NOT create separate two-pass logic per output context:** +```python +# ❌ WRONG: Duplicated import resolution +def generate_pipeline_code(pipeline): + # ... generate code ... + imports = resolve_imports(...) # DON'T DUPLICATE +``` +Import resolution is ONE function called by `generate_python_source()`. Not per-context. + +**DO NOT add special cases for "Lazy" types:** +```python +# ❌ WRONG: String matching on type names +if 'Lazy' in type(obj).__name__: + return format_lazy(obj) # DON'T +``` +Lazy types get their own formatter with proper `isinstance()` check. + +### Implementation Draft + +*Awaiting smell loop approval.* diff --git a/plans/rot_elimination/plan_01_streaming_backend_unification.md b/plans/rot_elimination/plan_01_streaming_backend_unification.md new file mode 100644 index 000000000..7973bdaea --- /dev/null +++ b/plans/rot_elimination/plan_01_streaming_backend_unification.md @@ -0,0 +1,261 @@ +# plan_01_streaming_backend_unification.md +## Component: Declarative Streaming Backend + +### Objective + +Eliminate duplication between `NapariStreamingBackend` and `FijiStreamingBackend` by making implementations **purely declarative**. No methods to override. Declare class attributes, `__init_subclass__` derives behavior. + +**Manifesto alignment:** +- *"When you see duplication, you don't copy. You extract."* (Β§IX) +- *"Declare what, derive how"* (OpenHCS pattern) + +### Findings + +**Current state β€” 80% duplication in `save_batch()`** + +Both backends do the same 8-step dance. The genuine differences are **static configuration**, not polymorphic behavior: + +| Aspect | Napari | Fiji | +|--------|--------|------| +| ROI converter class | `NapariROIConverter` | `FijiROIConverter` | +| ROI payload key | `'shapes'` | `'rois'` | +| Colormap key | `'colormap'` | `'lut'` | +| Extra display fields | `['variable_size_handling']` | `['auto_contrast']` | +| Socket pattern | `SocketPattern.PUB_NOBLOCK` | `SocketPattern.REQ_REP` | + +**These are not behaviors. They are declarations.** + +### The Insight + +If differences are static, they should be CLASS ATTRIBUTES, not methods: + +```python +class NapariStreamingBackend(StreamingBackend): + # DECLARATION ONLY - no methods + _backend_type = Backend.NAPARI_STREAM.value + roi_converter = NapariROIConverter + roi_payload_key = 'shapes' + colormap_key = 'colormap' + display_fields = ['colormap', 'variable_size_handling'] + socket_pattern = SocketPattern.PUB_NOBLOCK +``` + +The ABC reads these declarations and **derives** `save_batch()` behavior. + +### Plan + +#### 1. Define Socket Pattern Enum + +```python +class SocketPattern(Enum): + PUB_NOBLOCK = "pub_noblock" # Non-blocking publish + REQ_REP = "req_rep" # Request-reply with ack +``` + +#### 2. StreamingBackend ABC with `__init_subclass__` + +```python +class StreamingBackend(DataSink): + """Streaming backend with derived behavior. + + Subclasses declare attributes. ABC derives save_batch(). + """ + + # Required declarations (enforced by __init_subclass__) + roi_converter: Type[ROIConverter] + roi_payload_key: str + colormap_key: str + display_fields: List[str] + socket_pattern: SocketPattern + + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + + # Validate all required declarations exist + required = ['roi_converter', 'roi_payload_key', 'colormap_key', + 'display_fields', 'socket_pattern'] + missing = [r for r in required if not hasattr(cls, r)] + if missing and not getattr(cls, '__abstract__', False): + raise TypeError(f"{cls.__name__} missing declarations: {missing}") + + # Derive socket sender from pattern + cls._send = cls._derive_sender(cls.socket_pattern) + + @staticmethod + def _derive_sender(pattern: SocketPattern) -> Callable: + """Return send function for socket pattern.""" + if pattern == SocketPattern.PUB_NOBLOCK: + return lambda pub, msg: pub.send_json(msg, zmq.NOBLOCK) + elif pattern == SocketPattern.REQ_REP: + return lambda pub, msg: (pub.send_json(msg), pub.recv()) + + def save_batch(self, data_list, file_paths, **kwargs) -> None: + """Generic save_batch derived from class declarations.""" + # Build batch items (same for all backends) + batch_items = [] + for data, path in zip(data_list, file_paths): + if self._is_roi(data): + item = self._prepare_roi(data, path) + else: + item = self._prepare_image(data, path) + batch_items.append(item) + + # Build message using declared keys + message = { + 'type': 'batch', + 'items': batch_items, + 'display_config': self._build_display_config(kwargs), + } + + # Send using derived sender + self._send(self._publisher, message) + + def _prepare_roi(self, data, path) -> dict: + """Prepare ROI using declared converter and key.""" + converted = self.roi_converter.convert(data) + return {'path': str(path), self.roi_payload_key: converted} + + def _build_display_config(self, kwargs) -> dict: + """Build display config using declared fields.""" + config = kwargs.get('display_config', {}) + return {field: getattr(config, field, None) for field in self.display_fields} +``` + +#### 3. Implementations Are Pure Declaration + +```python +class NapariStreamingBackend(StreamingBackend): + """Napari streaming. Declaration only.""" + _backend_type = Backend.NAPARI_STREAM.value + + roi_converter = NapariROIConverter + roi_payload_key = 'shapes' + colormap_key = 'colormap' + display_fields = ['colormap', 'variable_size_handling'] + socket_pattern = SocketPattern.PUB_NOBLOCK + + +class FijiStreamingBackend(StreamingBackend): + """Fiji streaming. Declaration only.""" + _backend_type = Backend.FIJI_STREAM.value + + roi_converter = FijiROIConverter + roi_payload_key = 'rois' + colormap_key = 'lut' + display_fields = ['lut', 'auto_contrast'] + socket_pattern = SocketPattern.REQ_REP +``` + +**Zero methods. Pure declaration. Behavior derived.** + +### Why This Is Cracked + +| Old (Competent) | New (Cracked) | +|-----------------|---------------| +| Abstract methods to override | Class attributes to declare | +| 4 methods per implementation | 0 methods per implementation | +| Manual `save_batch()` in each | One generic `save_batch()` in ABC | +| Polymorphic dispatch | Structural derivation from declarations | +| Implementations have behavior | Implementations have configuration | + +### Architecture Diagram + +```mermaid +classDiagram + class StreamingBackend { + <> + +roi_converter: Type + +roi_payload_key: str + +colormap_key: str + +display_fields: List[str] + +socket_pattern: SocketPattern + +__init_subclass__() derives behavior + +save_batch() generic, uses declarations + } + + class NapariStreamingBackend { + roi_converter = NapariROIConverter + roi_payload_key = 'shapes' + colormap_key = 'colormap' + display_fields = [...] + socket_pattern = PUB_NOBLOCK + %%NO METHODS%% + } + + class FijiStreamingBackend { + roi_converter = FijiROIConverter + roi_payload_key = 'rois' + colormap_key = 'lut' + display_fields = [...] + socket_pattern = REQ_REP + %%NO METHODS%% + } + + StreamingBackend <|-- NapariStreamingBackend + StreamingBackend <|-- FijiStreamingBackend + + note for NapariStreamingBackend "Declaration only\nNo methods" + note for FijiStreamingBackend "Declaration only\nNo methods" +``` + +### Cleanup β€” DELETE ALL OF THIS + +**Code to DELETE from `napari_stream.py` and `fiji_stream.py`:** +- All duplicated `save_batch()` method bodies β†’ replaced by ABC generic implementation +- All per-backend ROI conversion logic β†’ moved to ABC, dispatched by `roi_converter` declaration +- All per-backend colormap handling β†’ moved to ABC, keyed by `colormap_key` declaration + +**No wrappers. No backwards compatibility.** +- `save_batch()` is DELETED from implementations +- Implementations become declaration-only classes (~10 lines each) +- If something breaks, fix the caller β€” don't add a shim + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT keep abstract methods in the ABC:** +```python +# ❌ WRONG: Abstract methods to override +class StreamingBackend(ABC): + @abstractmethod + def convert_roi(self, roi): ... # DON'T - use class attribute + + @abstractmethod + def get_colormap_key(self) -> str: ... # DON'T - use class attribute +``` +Use class attributes (`roi_converter`, `colormap_key`) that the ABC reads. No abstract methods. + +**DO NOT override save_batch() in implementations:** +```python +# ❌ WRONG: Override with "minor differences" +class NapariStreamingBackend(StreamingBackend): + def save_batch(self, ...): + # "Just a small tweak for Napari..." + super().save_batch(...) # DON'T +``` +If there's a difference, parameterize it via class attribute. One `save_batch()` in ABC. + +**DO NOT create helper methods in implementations:** +```python +# ❌ WRONG: Helper methods +class NapariStreamingBackend(StreamingBackend): + def _prepare_napari_payload(self, data): # DON'T + ... +``` +Implementations have ZERO methods. All logic in ABC, parameterized by declarations. + +**DO NOT use if/elif chains for backend dispatch:** +```python +# ❌ WRONG: Type dispatch in ABC +def save_batch(self): + if isinstance(self, NapariStreamingBackend): + ... + elif isinstance(self, FijiStreamingBackend): + ... +``` +Read from class attributes. No isinstance checks. + +**The test: Implementation classes should be ~10 lines of pure attribute declarations. If you're adding methods, you're doing it wrong.** + +### Implementation Draft + +*Awaiting smell loop approval.* diff --git a/plans/rot_elimination/plan_02_step_executor_extraction.md b/plans/rot_elimination/plan_02_step_executor_extraction.md new file mode 100644 index 000000000..99eeb7933 --- /dev/null +++ b/plans/rot_elimination/plan_02_step_executor_extraction.md @@ -0,0 +1,276 @@ +# plan_02_step_executor_extraction.md +## Component: Derived Step Execution + +### Objective + +Eliminate the 647-line `FunctionStep.process()` god method by extracting execution into a **single generic executor** that reads typed plans. Not a "phase pipeline" β€” the complexity is real and must be preserved. The cracked solution is **one place** instead of scattered across step classes. + +### The Rot + +```python +# openhcs/core/steps/function_step.py β€” 647 lines of execution logic +class FunctionStep(AbstractStep): + def process(self, context: ProcessingContext, step_index: int) -> None: + step_plan = context.step_plans[step_index] + func = step_plan.get('func') # Stringly-typed Dict[str, Any] + ... +``` + +### The Reality (Not Oversimplified) + +The 647 lines handle REAL complexity that can't be abstracted away: + +1. **Pattern discovery** β€” `microscope_handler.get_patterns_for_well()` +2. **Grouping** β€” `prepare_patterns_and_functions()` groups by component +3. **Function chains** β€” `_execute_chain_core()` for `[func1, func2, ...]` +4. **Dict patterns** β€” different functions per channel: `{'ch1': func_a, 'ch2': func_b}` +5. **Special I/O** β€” loading ROIs, saving auxiliary outputs via `funcplan` +6. **Memory stacking** β€” `stack_slices()` / `unstack_slices()` for 3D arrays +7. **Streaming** β€” sending results to Napari/Fiji after processing +8. **Materialization** β€” writing memory-backend data to disk + +**The previous plan's "composable phases" was wrong.** These aren't linear phases β€” they're interleaved concerns. The complexity is intrinsic. + +### The Actual Insight + +The problem isn't the complexity β€” it's the LOCATION. This logic is: +1. Inside `FunctionStep` (coupling step declaration to execution) +2. Scattered across module-level functions (`_execute_function_core`, `_execute_chain_core`, `_process_single_pattern_group`) +3. Reading from `Dict[str, Any]` instead of typed plan + +**Cracked solution:** ONE `StepExecutor` class that: +- Takes typed `StepPlan` (not dict) +- Contains ALL execution logic (not scattered) +- Is called by orchestrator, not by step + +### Plan + +#### 1. Typed StepPlan (from compiler spec) + +```python +@dataclass(frozen=True) +class StepPlan: + """Typed, frozen plan. Not Dict[str, Any].""" + step_name: str + axis_id: str + input_dir: Path + output_dir: Path + func: Union[Callable, List[Callable], Dict[str, Callable]] + variable_components: List[str] + group_by: Optional[GroupBy] + special_inputs: Dict[str, SpecialInputInfo] + special_outputs: OrderedDict[str, SpecialOutputInfo] + funcplan: Dict[str, List[str]] + read_backend: Backend + write_backend: Backend + input_memory_type: MemoryType + output_memory_type: MemoryType + streaming_configs: List[StreamingConfig] + zarr_config: Optional[ZarrConfig] + device_id: Optional[int] +``` + +#### 2. Single StepExecutor (Contains ALL Logic) + +```python +class StepExecutor: + """ONE class with ALL execution logic. + + Not phases. Not polymorphism. Just consolidated code. + The 647 lines move HERE β€” they don't disappear. + """ + + def execute(self, plan: StepPlan, context: ProcessingContext) -> None: + """Execute a compiled step plan.""" + # Pattern discovery + patterns_by_well = self._discover_patterns(plan, context) + + # Grouping + grouped_patterns, comp_to_funcs, comp_to_args = self._prepare_patterns( + patterns_by_well[plan.axis_id], plan + ) + + # Process each group + for comp_val, patterns in grouped_patterns.items(): + func_or_chain = comp_to_funcs[comp_val] + base_args = comp_to_args[comp_val] + + for pattern in patterns: + self._process_pattern( + pattern, func_or_chain, base_args, plan, context, comp_val + ) + + # Streaming (after all patterns processed) + self._stream_results(plan, context) + + # Materialization + self._materialize_outputs(plan, context) + + def _process_pattern(self, pattern, func, args, plan, context, comp_val) -> None: + """Process single pattern β€” load, stack, execute, unstack, save.""" + # Load and stack + data_stack = self._load_and_stack(pattern, plan, context) + + # Execute (handles single func, chain, or dict pattern) + if isinstance(func, list): + result = self._execute_chain(data_stack, func, args, plan, context, comp_val) + else: + result = self._execute_single(data_stack, func, args, plan, context, comp_val) + + # Unstack and save + self._unstack_and_save(result, pattern, plan, context) + + # ... rest of the 647 lines, consolidated here +``` + +#### 3. FunctionStep Becomes Pure Declaration + +```python +@dataclass +class FunctionStep(AbstractStep): + """Declaration only. Zero execution logic.""" + func: FuncPattern + + # NO process() method + # Compiler produces StepPlan + # Orchestrator calls StepExecutor.execute(plan) +``` + +#### 4. Orchestrator Calls Executor + +```python +class Orchestrator: + def run_step(self, step: AbstractStep, step_index: int) -> None: + # Compilation already happened β€” plan is in context + plan = self.context.step_plans[step_index] + + # One executor, takes any plan + executor = StepExecutor() + executor.execute(plan, self.context) +``` + +### Why This Is Actually Cracked + +| Old | New | +|-----|-----| +| Execution in `FunctionStep.process()` | Execution in `StepExecutor.execute()` | +| Scattered module functions | Consolidated in one class | +| `Dict[str, Any]` plan | Typed frozen `StepPlan` | +| Step knows how to execute itself | Step is pure declaration | +| 647 lines in step file | 647 lines in executor file (moved, not deleted) | + +**The lines don't shrink. The coupling shrinks.** FunctionStep goes from 1467 β†’ ~50 lines. The execution logic moves to a dedicated executor. + +### What This Enables + +1. **Other step types** reuse the same executor (if they produce compatible StepPlan) +2. **Testing** can mock StepExecutor without mocking steps +3. **Debugging** has one place to look for execution issues +4. **Future optimization** (parallelism, caching) has one entry point + +### Architecture Diagram + +```mermaid +flowchart TD + subgraph Compile["Compile Time"] + FS[FunctionStep] -->|"compiler.compile()"| SP[StepPlan] + end + + subgraph Execute["Execute Time"] + SP -->|"executor.execute(plan)"| SE[StepExecutor] + SE --> DP[discover_patterns] + DP --> PP[prepare_patterns] + PP --> PR[process_pattern loop] + PR --> ST[stream_results] + ST --> MT[materialize_outputs] + end + + style FS fill:#339af0,color:#fff + style SP fill:#51cf66,color:#fff + style SE fill:#51cf66,color:#fff +``` + +### Dependencies + +- `plan_01_generic_compiler_spec.md` β€” Typed StepPlan with all execution info + +### Cleanup β€” DELETE ALL OF THIS + +**Files to modify:** + +1. **`openhcs/core/steps/function_step.py`** (1467 β†’ ~50 lines) + - DELETE: `process()` method (lines 821-1036) + - DELETE: `_execute_function_core()` (lines 384-491) + - DELETE: `_execute_chain_core()` (lines 493-571) + - DELETE: `_process_single_pattern_group()` (lines 573-798) + - DELETE: All streaming/materialization helpers + - KEEP: `__init__()`, class attributes, `func` property + +2. **NEW: `openhcs/core/execution/step_executor.py`** (~500 lines) + - All deleted code moves here, consolidated + - Takes typed `StepPlan` instead of `Dict[str, Any]` + - Single entry point: `StepExecutor.execute(plan, context)` + +3. **`openhcs/core/orchestrator.py`** + - CHANGE: `step.process(context, i)` β†’ `StepExecutor().execute(plan, context)` + +**No wrappers. No backwards compatibility.** +- Steps lose `process()` method entirely +- Orchestrator calls executor directly +- If something breaks, fix the plan structure β€” don't add special cases + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT create per-step executor subclasses:** +```python +# ❌ WRONG: Polymorphic dispatch +class ExecutorBase(ABC): + @abstractmethod + def execute(self, plan, context): ... + +class FunctionStepExecutor(ExecutorBase): ... +class CustomStepExecutor(ExecutorBase): ... +``` +ONE `StepExecutor` class. Dispatch on plan structure, not step type. + +**DO NOT keep process() as a delegation wrapper:** +```python +# ❌ WRONG: Wrapper method +class FunctionStep: + def process(self, context, step_index): + plan = context.step_plans[step_index] + StepExecutor().execute(plan, context) # DON'T KEEP process() +``` +Delete `process()` entirely. Orchestrator calls executor directly. + +**DO NOT create "ExecutionPhase" abstractions:** +```python +# ❌ WRONG: Over-engineering +class LoadPhase(ExecutionPhase): ... +class TransformPhase(ExecutionPhase): ... +class SavePhase(ExecutionPhase): ... +``` +The 647 lines are interleaved concerns, not linear phases. Keep them as methods in `StepExecutor`, not as separate classes. + +**DO NOT add step_type field to dispatch on:** +```python +# ❌ WRONG: Type field dispatch +if plan.step_type == "function": + self._execute_function(plan) +elif plan.step_type == "custom": + self._execute_custom(plan) +``` +The plan structure IS the dispatch. If `plan.func` is a list, execute as chain. If dict, execute as dict pattern. No type field. + +**DO NOT create abstract methods in AbstractStep for execution:** +```python +# ❌ WRONG: Execution in step hierarchy +class AbstractStep(ABC): + @abstractmethod + def get_executor(self) -> StepExecutor: ... +``` +Steps are DECLARATION only. They don't know about execution. + +### Implementation Draft + +*Awaiting smell loop approval.* diff --git a/plans/rot_elimination/plan_02_streaming_message_protocol.md b/plans/rot_elimination/plan_02_streaming_message_protocol.md new file mode 100644 index 000000000..5c15c269b --- /dev/null +++ b/plans/rot_elimination/plan_02_streaming_message_protocol.md @@ -0,0 +1,295 @@ +# plan_02_streaming_message_protocol.md +## Component: Typed Streaming Message Protocol + +### Objective + +Eliminate stringly-typed JSON message handling in streaming infrastructure. Replace copy-pasted dict building/parsing with typed dataclasses that fail loud on malformed data. + +**Manifesto alignment:** +- *"No silent failures. If something is wrong, Python raises an error."* (Β§V) +- *"Declarative code is read structurally. The shape of the data IS the shape of the system."* (Β§VI) +- *"Each abstraction must solve one problem completely, generically, once."* (Β§II) + +### Findings + +**Current state β€” JavaScript-tier stringly-typed messaging:** + +The codebase has `zmq_messages.py` with typed dataclasses (`ExecuteRequest`, `ImageAck`, `ROIMessage`) β€” but streaming ignores them entirely. Instead: + +1. **Sender** (`napari_stream.py`, `fiji_stream.py`) builds raw dicts with magic strings +2. **Receiver** (`napari_stream_visualizer.py`, `fiji_viewer_server.py`) parses with `.get()` and fallback defaults +3. **No validation** β€” malformed messages silently produce garbage +4. **Duplicated parsing** β€” both servers have identical `process_image_message()` parsing logic + +**Evidence of rot:** + +```python +# SENDER: napari_stream.py line 145 +message = { + 'type': 'batch', + 'images': batch_images, + 'display_config': {'colormap': ..., 'component_modes': ...}, +} + +# RECEIVER: napari_stream_visualizer.py line 1338 +msg_type = data.get("type") +if msg_type == "batch": + images = data.get("images", []) +``` + +Magic strings. No types. Silent fallbacks. Copy-pasted between Napari and Fiji. + +### Current Architecture (Rot) + +```mermaid +flowchart TD + subgraph Sender["Sender (Backend)"] + NB[NapariStreamingBackend] + FB[FijiStreamingBackend] + NB -->|"build raw dict"| D1["{'type': 'batch', 'images': [...]}"] + FB -->|"build raw dict"| D2["{'type': 'batch', 'images': [...]}"] + end + + subgraph Wire["ZMQ Wire"] + D1 -->|send_json| JSON1[JSON bytes] + D2 -->|send_json| JSON2[JSON bytes] + end + + subgraph Receiver["Receiver (Server)"] + JSON1 -->|json.loads| P1["data.get('type')"] + JSON2 -->|json.loads| P2["data.get('type')"] + P1 -->|"magic string check"| NS[NapariViewerServer] + P2 -->|"magic string check"| FS[FijiViewerServer] + end + + style D1 fill:#ff6b6b,color:#fff + style D2 fill:#ff6b6b,color:#fff + style P1 fill:#ff6b6b,color:#fff + style P2 fill:#ff6b6b,color:#fff +``` + +**Problems visible in diagram:** +- Two parallel paths doing identical work +- Raw dicts at sender, raw parsing at receiver +- No type safety crossing the wire + +### Proposed Architecture (Clean) + +```mermaid +flowchart TD + subgraph Sender["Sender (ABC)"] + SB[StreamingBackend ABC] + SB -->|"build typed"| BM[BatchMessage dataclass] + BM -->|".to_json()"| JSON[JSON bytes] + end + + subgraph Wire["ZMQ Wire"] + JSON -->|send| ZMQ[ZMQ Socket] + end + + subgraph Receiver["Receiver (ABC)"] + ZMQ -->|recv| BYTES[JSON bytes] + BYTES -->|"BatchMessage.from_json()"| BM2[BatchMessage dataclass] + BM2 -->|"validated, typed"| VS[ViewerServer ABC] + VS -->|polymorphic| NS2[NapariViewerServer] + VS -->|polymorphic| FS2[FijiViewerServer] + end + + style BM fill:#51cf66,color:#fff + style BM2 fill:#51cf66,color:#fff + style SB fill:#339af0,color:#fff + style VS fill:#339af0,color:#fff +``` + +**Improvements:** +- Single code path through ABCs +- Typed dataclasses cross the wire +- Validation happens once in `from_json()` +- Implementations only handle viewer-specific display logic + +### Message Type Hierarchy + +```mermaid +classDiagram + class StreamingMessage { + <> + +type: MessageType + +timestamp: float + +to_json() bytes + +from_json(bytes) StreamingMessage + +validate() None + } + + class BatchMessage { + +type = MessageType.BATCH + +items: List[StreamingItem] + +display_config: DisplayConfig + +component_names_metadata: Dict + } + + class StreamingItem { + <> + +item_type: ItemType + +path: str + +image_id: str + +metadata: ComponentMetadata + } + + class ImageItem { + +item_type = ItemType.IMAGE + +shm_name: str + +shape: Tuple[int, ...] + +dtype: str + } + + class ShapesItem { + +item_type = ItemType.SHAPES + +shapes: List[ShapeData] + } + + class RoisItem { + +item_type = ItemType.ROIS + +rois: List[bytes] + } + + class DisplayConfig { + +colormap_or_lut: str + +component_modes: Dict[str, str] + +component_order: List[str] + +extra: Dict[str, Any] + } + + class ComponentMetadata { + +source: str + +well: Optional[str] + +channel: Optional[str] + +site: Optional[str] + +z: Optional[str] + +time: Optional[str] + } + + StreamingMessage <|-- BatchMessage + BatchMessage *-- StreamingItem + BatchMessage *-- DisplayConfig + StreamingItem <|-- ImageItem + StreamingItem <|-- ShapesItem + StreamingItem <|-- RoisItem + StreamingItem *-- ComponentMetadata +``` + +### Plan + +1. **Define message enums in `zmq_messages.py`:** + ```python + class StreamingMessageType(Enum): + BATCH = "batch" + SINGLE = "single" # legacy + + class StreamingItemType(Enum): + IMAGE = "image" + SHAPES = "shapes" + POINTS = "points" + ROIS = "rois" + ``` + +2. **Define typed dataclasses for all message components** (see hierarchy above) + +3. **Add validation in `from_json()`:** + ```python + @classmethod + def from_json(cls, data: bytes) -> 'BatchMessage': + d = json.loads(data) + if d.get('type') != StreamingMessageType.BATCH.value: + raise ValueError(f"Expected batch message, got {d.get('type')}") + # ... validate and construct + ``` + +4. **Refactor sender (StreamingBackend ABC):** + - Build `BatchMessage` dataclass instead of raw dict + - Call `.to_json()` before sending + +5. **Refactor receiver (ViewerServer ABC):** + - Call `BatchMessage.from_json()` to parse + - Pass typed objects to `_process_single_item(item: StreamingItem)` + +6. **Push common parsing to ABC:** + - `ZMQServer` ABC gets `_parse_streaming_message()` + - Implementations only override display logic + +### Cleanup β€” DELETE ALL OF THIS + +**Code to DELETE from `napari_stream.py` and `fiji_stream.py`:** +```python +message = { + 'type': 'batch', + 'images': [...], + 'display_config': {...} +} # DELETE all raw dict building +``` + +**Code to DELETE from `napari_stream_visualizer.py` and `fiji_viewer_server.py`:** +```python +msg_type = data.get("type") +if msg_type == "batch": + images = data.get("images", []) + # ... DELETE all stringly-typed parsing with .get() fallbacks +``` + +**No wrappers. No backwards compatibility.** +- Senders build `BatchMessage` dataclass β†’ call `.to_json()` +- Receivers call `BatchMessage.from_json()` β†’ get typed object or fail loud +- If old code sends raw dicts, it breaks β€” update the sender + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT add fallback parsing for untyped dicts:** +```python +# ❌ WRONG: Fallback for old format +def from_json(cls, data: bytes) -> 'BatchMessage': + d = json.loads(data) + try: + return cls._parse_typed(d) + except KeyError: + return cls._parse_legacy_dict(d) # DON'T ADD FALLBACK +``` +Fail loud. If the message is malformed, raise. Update the sender. + +**DO NOT use .get() with default fallbacks:** +```python +# ❌ WRONG: Silent fallback to default +item_type = data.get('type', 'image') # DON'T - fails silently +items = data.get('items', []) # DON'T - hides missing field +``` +Access directly. Missing field = KeyError = fail loud. + +**DO NOT create separate message classes per backend:** +```python +# ❌ WRONG: Per-backend messages +class NapariBatchMessage(BatchMessage): ... +class FijiBatchMessage(BatchMessage): ... +``` +ONE `BatchMessage` class. Backend differences are in display config, not message structure. + +**DO NOT keep raw dict building "for debugging":** +```python +# ❌ WRONG: Parallel paths +if DEBUG: + message = {'type': 'batch', ...} # DON'T KEEP RAW DICTS +else: + message = BatchMessage(...).to_json() +``` +One path. Always typed dataclass. Debug by inspecting the dataclass. + +**DO NOT add optional fields to avoid migration:** +```python +# ❌ WRONG: All fields optional +@dataclass +class BatchMessage: + items: Optional[List[StreamingItem]] = None # DON'T - hides bugs + display_config: Optional[DisplayConfig] = None +``` +Required fields are required. Optional means semantically optional, not "might be missing in legacy code." + +### Implementation Draft + +*Pending smell loop approval.* diff --git a/plans/rot_elimination/plan_03_dimension_abstraction.md b/plans/rot_elimination/plan_03_dimension_abstraction.md new file mode 100644 index 000000000..bcc973837 --- /dev/null +++ b/plans/rot_elimination/plan_03_dimension_abstraction.md @@ -0,0 +1,240 @@ +# plan_03_dimension_abstraction.md +## Component: Fiji Viewer Dimension Abstraction + +### Objective +Eliminate the 103 references to `channel_components`, `slice_components`, `frame_components` triplication in `fiji_viewer_server.py`. Replace with ONE generic dimension abstraction. + +### Findings + +**Current Rot β€” Same Pattern 6+ Times:** + +```python +# Pattern appears at lines 778-780, 798-800, 909-911, 1078-1096, 1199-1215, 1342+ +c_key = tuple(meta[comp] for comp in channel_components) if channel_components else () +z_key = tuple(meta[comp] for comp in slice_components) if slice_components else () +t_key = tuple(meta[comp] for comp in frame_components) if frame_components else () +``` + +```python +# Pattern appears at lines 1078-1102 +if channel_components and channel_values: + current_channel = imp.getChannel() + if 0 < current_channel <= len(channel_values): + channel_tuple = channel_values[current_channel - 1] + for comp_idx, comp_name in enumerate(channel_components): + # ... +if slice_components and slice_values: # SAME PATTERN +if frame_components and frame_values: # SAME PATTERN +``` + +**Method Signatures Are Unreadable:** +```python +def _add_slices_to_existing_hyperstack( + self, existing_imp, new_images: List[Dict[str, Any]], + window_key: str, + channel_components: List[str], # ← Dimension 1 + slice_components: List[str], # ← Dimension 2 + frame_components: List[str], # ← Dimension 3 + display_config_dict: Dict[str, Any], + channel_values: List[tuple] = None, # ← Dimension 1 values + slice_values: List[tuple] = None, # ← Dimension 2 values + frame_values: List[tuple] = None, # ← Dimension 3 values +): +``` + +Six parallel parameters for three dimensions! + +### Plan + +**Phase 1: Dimension Dataclass** + +```python +@dataclass +class HyperstackDimension: + """A single dimension of a hyperstack (channel, slice, or frame).""" + name: str # "channel", "slice", or "frame" + components: List[str] # Component names mapped to this dimension + values: List[tuple] # Unique value tuples for this dimension + + def build_key(self, metadata: Dict[str, Any]) -> tuple: + """Build coordinate key from metadata.""" + return tuple(metadata[comp] for comp in self.components) if self.components else () + + def get_index(self, metadata: Dict[str, Any]) -> int: + """Get 0-based index for metadata coordinates.""" + key = self.build_key(metadata) + try: + return self.values.index(key) + except ValueError: + return -1 +``` + +**Phase 2: Dimension Container** + +```python +@dataclass +class HyperstackDimensions: + """All three dimensions of a hyperstack.""" + channel: HyperstackDimension + slice: HyperstackDimension + frame: HyperstackDimension + + def build_coord_key(self, metadata: Dict[str, Any]) -> Tuple[tuple, tuple, tuple]: + """Build full (C, Z, T) coordinate key.""" + return ( + self.channel.build_key(metadata), + self.slice.build_key(metadata), + self.frame.build_key(metadata), + ) + + def __iter__(self): + """Iterate over dimensions for generic processing.""" + return iter([self.channel, self.slice, self.frame]) +``` + +**Phase 3: Simplify Method Signatures** + +```python +# BEFORE: 6 parameters for dimensions +def _add_slices_to_existing_hyperstack( + self, existing_imp, new_images, + window_key, channel_components, slice_components, frame_components, + display_config_dict, channel_values, slice_values, frame_values +): + +# AFTER: 1 parameter +def _add_slices_to_existing_hyperstack( + self, existing_imp, new_images, + window_key, dimensions: HyperstackDimensions, + display_config_dict +): +``` + +**Phase 4: Replace Triplicated Logic** + +```python +# BEFORE: Same pattern 3x +c_key = tuple(meta[comp] for comp in channel_components) if channel_components else () +z_key = tuple(meta[comp] for comp in slice_components) if slice_components else () +t_key = tuple(meta[comp] for comp in frame_components) if frame_components else () + +# AFTER: Generic +c_key, z_key, t_key = dimensions.build_coord_key(meta) +``` + +```python +# BEFORE: Same pattern 3x +if channel_components and channel_values: + current = imp.getChannel() + # ... 8 lines of logic ... +if slice_components and slice_values: + current = imp.getSlice() + # ... 8 lines of logic ... +if frame_components and frame_values: + current = imp.getFrame() + # ... 8 lines of logic ... + +# AFTER: Generic loop +dimension_getters = { + 'channel': imp.getChannel, + 'slice': imp.getSlice, + 'frame': imp.getFrame, +} +for dim in dimensions: + if dim.components and dim.values: + current = dimension_getters[dim.name]() + if 0 < current <= len(dim.values): + # ... 4 lines of generic logic ... +``` + +### Structural Properties + +- **No triplication** β€” Each dimension pattern appears exactly once +- **Single source of truth** β€” `HyperstackDimension` defines dimension behavior +- **Generic iteration** β€” Loop over dimensions instead of copy-paste +- **Readable signatures** β€” `dimensions: HyperstackDimensions` replaces 6 parameters + +### Cleanup β€” DELETE ALL OF THIS + +**Patterns to DELETE from `fiji_viewer_server.py` (103 occurrences):** +```python +# DELETE all 6+ copies of this: +c_key = tuple(meta[comp] for comp in channel_components) if channel_components else () +z_key = tuple(meta[comp] for comp in slice_components) if slice_components else () +t_key = tuple(meta[comp] for comp in frame_components) if frame_components else () + +# DELETE all 6+ copies of this: +if channel_components and channel_values: + current = imp.getChannel() + # ... logic ... +if slice_components and slice_values: + current = imp.getSlice() + # ... logic ... +if frame_components and frame_values: + current = imp.getFrame() + # ... logic ... +``` + +**Parameters to DELETE from method signatures:** +```python +# DELETE these 6 separate parameters: +channel_components: List[str], slice_components: List[str], frame_components: List[str], +channel_values: List[tuple], slice_values: List[tuple], frame_values: List[tuple] + +# REPLACE with 1 parameter: +dimensions: HyperstackDimensions +``` + +**No wrappers. No backwards compatibility.** +- All triplicated patterns β†’ replaced by single generic implementation +- All 6-parameter signatures β†’ replaced by 1 `dimensions` parameter +- If internal code uses the old patterns, it breaks β€” update to use `HyperstackDimensions` + +### ❌ ANTIPATTERNS TO AVOID + +**DO NOT keep the 6 parameters and wrap them:** +```python +# ❌ WRONG: Wrapper that builds dimensions from old params +def process_hyperstack( + channel_components, slice_components, frame_components, # DON'T KEEP + channel_values, slice_values, frame_values +): + dimensions = HyperstackDimensions.from_components(...) # DON'T WRAP +``` +Callers build `HyperstackDimensions` directly. No wrapper. + +**DO NOT keep per-dimension helper methods:** +```python +# ❌ WRONG: Helper per dimension +def _process_channel(self, channel_components, channel_values): ... +def _process_slice(self, slice_components, slice_values): ... +def _process_frame(self, frame_components, frame_values): ... +``` +ONE generic method that iterates over `dimensions`. No per-dimension methods. + +**DO NOT add dimension-specific branches in generic code:** +```python +# ❌ WRONG: if/elif on dimension name +for dim in dimensions: + if dim.name == 'channel': + self._handle_channel(dim) # DON'T + elif dim.name == 'slice': + self._handle_slice(dim) # DON'T +``` +All dimensions have same interface. Generic loop, no name checks. + +**DO NOT keep triplicated logic "for clarity":** +```python +# ❌ WRONG: "More readable" triplication +# Channel key extraction +c_key = tuple(meta[c] for c in channel_components) +# Slice key extraction +z_key = tuple(meta[c] for c in slice_components) +# Frame key extraction +t_key = tuple(meta[c] for c in frame_components) +``` +ONE generic extraction: `keys = {d.name: d.extract_key(meta) for d in dimensions}` + +### Implementation Draft + +(After smell loop approval)