From ba3af179ddac523c1375d6f27e2027cb760b043f Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Mon, 10 Nov 2025 13:49:38 +0100 Subject: [PATCH 1/6] Remove Unique_name fallback and integrate serialization with observer pattern. --- src/easyscience/global_object/map.py | 2 +- src/easyscience/variable/descriptor_number.py | 12 + src/easyscience/variable/parameter.py | 237 +++++++----------- .../variable/parameter_dependency_resolver.py | 2 +- ...test_parameter_dependency_serialization.py | 140 +++-------- 5 files changed, 142 insertions(+), 251 deletions(-) diff --git a/src/easyscience/global_object/map.py b/src/easyscience/global_object/map.py index 53a3aac4..0f29c1bc 100644 --- a/src/easyscience/global_object/map.py +++ b/src/easyscience/global_object/map.py @@ -261,7 +261,7 @@ def is_connected(self, vertices_encountered=None, start_vertex=None) -> bool: return False def _clear(self): - """Reset the map to an empty state.""" + """Reset the map to an empty state. Only to be used for testing""" for vertex in self.vertices(): self.prune(vertex) gc.collect() diff --git a/src/easyscience/variable/descriptor_number.py b/src/easyscience/variable/descriptor_number.py index 66f0e7ab..c72edb3c 100644 --- a/src/easyscience/variable/descriptor_number.py +++ b/src/easyscience/variable/descriptor_number.py @@ -1,6 +1,7 @@ from __future__ import annotations import numbers +import uuid from typing import Any from typing import Dict from typing import List @@ -50,6 +51,7 @@ def __init__( url: Optional[str] = None, display_name: Optional[str] = None, parent: Optional[Any] = None, + **kwargs: Any # Additional keyword arguments (used for (de)serialization) ): """Constructor for the DescriptorNumber class @@ -65,6 +67,10 @@ def __init__( """ self._observers: List[DescriptorNumber] = [] + # Extract dependency_id if provided during deserialization + if '_dependency_id' in kwargs: + self._dependency_id = kwargs.pop('_dependency_id') + if not isinstance(value, numbers.Number) or isinstance(value, bool): raise TypeError(f'{value=} must be a number') if variance is not None: @@ -112,10 +118,14 @@ def from_scipp(cls, name: str, full_value: Variable, **kwargs) -> DescriptorNumb def _attach_observer(self, observer: DescriptorNumber) -> None: """Attach an observer to the descriptor.""" self._observers.append(observer) + if not hasattr(self, '_dependency_id'): + self._dependency_id = str(uuid.uuid4()) def _detach_observer(self, observer: DescriptorNumber) -> None: """Detach an observer from the descriptor.""" self._observers.remove(observer) + if not self._observers: + del self._dependency_id def _notify_observers(self) -> None: """Notify all observers of a change.""" @@ -323,6 +333,8 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: raw_dict['value'] = self._scalar.value raw_dict['unit'] = str(self._scalar.unit) raw_dict['variance'] = self._scalar.variance + if hasattr(self, '_dependency_id'): + raw_dict['_dependency_id'] = self._dependency_id return raw_dict def __add__(self, other: Union[DescriptorNumber, numbers.Number]) -> DescriptorNumber: diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index 39692963..49c172c4 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -11,6 +11,7 @@ import weakref from typing import Any from typing import Dict +from typing import List from typing import Optional from typing import Union @@ -36,14 +37,6 @@ class Parameter(DescriptorNumber): # We copy the parent's _REDIRECT and modify it to avoid altering the parent's class dict _REDIRECT = DescriptorNumber._REDIRECT.copy() _REDIRECT['callback'] = None - # Skip these attributes during normal serialization as they are handled specially - _REDIRECT['_dependency_interpreter'] = None - _REDIRECT['_clean_dependency_string'] = None - # Skip the new serialization parameters - they'll be handled by _convert_to_dict - _REDIRECT['_dependency_string'] = None - _REDIRECT['_dependency_map_unique_names'] = None - _REDIRECT['_dependency_map_dependency_ids'] = None - _REDIRECT['__dependency_id'] = None def __init__( self, @@ -84,11 +77,8 @@ def __init__( """ # noqa: E501 # Extract and ignore serialization-specific fields from kwargs kwargs.pop('_dependency_string', None) - kwargs.pop('_dependency_map_unique_names', None) kwargs.pop('_dependency_map_dependency_ids', None) kwargs.pop('_independent', None) - # Extract dependency_id if provided during deserialization - provided_dependency_id = kwargs.pop('__dependency_id', None) if not isinstance(min, numbers.Number): raise TypeError('`min` must be a number') @@ -119,6 +109,7 @@ def __init__( url=url, display_name=display_name, parent=parent, + **kwargs, # Additional keyword arguments (used for (de)serialization) ) self._callback = callback # Callback is used by interface to link to model @@ -128,13 +119,6 @@ def __init__( # Create additional fitting elements self._initial_scalar = copy.deepcopy(self._scalar) - # Generate unique dependency ID for serialization/deserialization - # Use provided dependency_id if available (during deserialization), otherwise generate new one - if provided_dependency_id is not None: - self.__dependency_id = provided_dependency_id - else: - import uuid - self.__dependency_id = str(uuid.uuid4()) @classmethod def from_dependency(cls, name: str, dependency_expression: str, dependency_map: Optional[dict] = None, **kwargs) -> Parameter: # noqa: E501 @@ -147,7 +131,7 @@ def from_dependency(cls, name: str, dependency_expression: str, dependency_map: :param kwargs: Additional keyword arguments to pass to the Parameter constructor. :return: A new dependent Parameter object. """ # noqa: E501 - # Set default values for required parameters if not provided in kwargs + # Set default values for required parameters for the constructor, they get overwritten by the dependency anyways default_kwargs = { 'value': 0.0, 'unit': '', @@ -155,7 +139,7 @@ def from_dependency(cls, name: str, dependency_expression: str, dependency_map: 'min': -np.inf, 'max': np.inf } - # Update with user-provided kwargs, giving precedence to user values + # Update with user-provided kwargs, to avoid errors. default_kwargs.update(kwargs) parameter = cls(name=name, **default_kwargs) parameter.make_dependent_on(dependency_expression=dependency_expression, dependency_map=dependency_map) @@ -331,15 +315,6 @@ def dependency_map(self) -> Dict[str, DescriptorNumber]: def dependency_map(self, new_map: Dict[str, DescriptorNumber]) -> None: raise AttributeError('Dependency map is read-only. Use `make_dependent_on` to change the dependency map.') - @property - def dependency_id(self) -> str: - """ - Get the unique dependency ID of this parameter used for serialization. - - :return: The dependency ID of this parameter. - """ - return self.__dependency_id - @property def value_no_call_back(self) -> numbers.Number: """ @@ -553,6 +528,90 @@ def free(self) -> bool: def free(self, value: bool) -> None: self.fixed = not value + def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: + """ Overwrite the as_dict method to handle dependency information. """ + raw_dict = super().as_dict(skip=skip) + + + # Add dependency information for dependent parameters + if not self._independent: + # Save the dependency expression + raw_dict['_dependency_string'] = self._dependency_string + + # Mark that this parameter is dependent + raw_dict['_independent'] = self._independent + + # Convert dependency_map to use dependency_ids + raw_dict['_dependency_map_dependency_ids'] = {} + for key, obj in self._dependency_map.items(): + raw_dict['_dependency_map_dependency_ids'][key] = obj._dependency_id + + return raw_dict + + @classmethod + def from_dict(cls, obj_dict: dict) -> 'Parameter': + """ + Custom deserialization to handle parameter dependencies. + Override the parent method to handle dependency information. + """ + # Extract dependency information before creating the parameter + raw_dict = obj_dict.copy() # Don't modify the original dict + dependency_string = raw_dict.pop('_dependency_string', None) + dependency_map_dependency_ids = raw_dict.pop('_dependency_map_dependency_ids', None) + is_independent = raw_dict.pop('_independent', True) + # Note: Keep _dependency_id in the dict so it gets passed to __init__ + + # Create the parameter using the base class method (dependency_id is now handled in __init__) + param = super().from_dict(raw_dict) + + # Store dependency information for later resolution + if not is_independent: + param._pending_dependency_string = dependency_string + param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids + # Keep parameter as independent initially - will be made dependent after all objects are loaded + param._independent = True + + return param + + def resolve_pending_dependencies(self) -> None: + """Resolve pending dependencies after deserialization. + + This method should be called after all parameters have been deserialized + to establish dependency relationships using dependency_ids. + """ + if hasattr(self, '_pending_dependency_string'): + dependency_string = self._pending_dependency_string + dependency_map = {} + + if hasattr(self, '_pending_dependency_map_dependency_ids'): + dependency_map_dependency_ids = self._pending_dependency_map_dependency_ids + + # Build dependency_map by looking up objects by dependency_id + for key, dependency_id in dependency_map_dependency_ids.items(): + dep_obj = self._find_parameter_by_dependency_id(dependency_id) + if dep_obj is not None: + dependency_map[key] = dep_obj + else: + raise ValueError(f"Cannot find parameter with dependency_id '{dependency_id}'") + + # Establish the dependency relationship + try: + self.make_dependent_on(dependency_expression=dependency_string, dependency_map=dependency_map) + except Exception as e: + raise ValueError(f"Error establishing dependency '{dependency_string}': {e}") + + # Clean up temporary attributes + delattr(self, '_pending_dependency_string') + delattr(self, '_pending_dependency_map_dependency_ids') + + def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: + """Find a parameter by its dependency_id from all parameters in the global map.""" + for obj in self._global_object.map._store.values(): + if isinstance(obj, DescriptorNumber) and hasattr(obj, '_dependency_id') and obj._dependency_id == dependency_id: + return obj + return None + + def _revert_dependency(self, skip_detach=False) -> None: """ Revert the dependency to the old dependency. This is used when an error is raised during setting the dependency. @@ -595,68 +654,6 @@ def _process_dependency_unique_names(self, dependency_expression: str): raise ValueError(f'The object with unique_name {stripped_name} is not a Parameter or DescriptorNumber. Please check your dependency expression.') # noqa: E501 self._clean_dependency_string = clean_dependency_string - def _convert_to_dict(self, d: dict, encoder, skip=None, **kwargs) -> dict: - """Custom serialization to handle parameter dependencies.""" - if skip is None: - skip = [] - - # Add dependency information for dependent parameters - if not self._independent: - # Save the dependency expression - d['_dependency_string'] = self._dependency_string - - # Convert dependency_map to use dependency_ids (preferred) and unique_names (fallback) - d['_dependency_map_dependency_ids'] = {} - d['_dependency_map_unique_names'] = {} - - for key, dep_obj in self._dependency_map.items(): - # Store dependency_id if available (more reliable) - if hasattr(dep_obj, '__dependency_id'): - d['_dependency_map_dependency_ids'][key] = dep_obj.__dependency_id - # Also store unique_name as fallback - if hasattr(dep_obj, 'unique_name'): - d['_dependency_map_unique_names'][key] = dep_obj.unique_name - else: - # This is quite impossible - throw an error - raise ValueError(f'The object with unique_name {key} does not have a unique_name attribute.') - - # Always include dependency_id for this parameter - d['__dependency_id'] = self.__dependency_id - - # Mark that this parameter is dependent - d['_independent'] = self._independent - - return d - - @classmethod - def from_dict(cls, obj_dict: dict) -> 'Parameter': - """ - Custom deserialization to handle parameter dependencies. - Override the parent method to handle dependency information. - """ - # Extract dependency information before creating the parameter - d = obj_dict.copy() # Don't modify the original dict - dependency_string = d.pop('_dependency_string', None) - dependency_map_unique_names = d.pop('_dependency_map_unique_names', None) - dependency_map_dependency_ids = d.pop('_dependency_map_dependency_ids', None) - is_independent = d.pop('_independent', True) - # Note: Keep __dependency_id in the dict so it gets passed to __init__ - - # Create the parameter using the base class method (dependency_id is now handled in __init__) - param = super().from_dict(d) - - # Store dependency information for later resolution - if not is_independent and dependency_string is not None: - param._pending_dependency_string = dependency_string - if dependency_map_dependency_ids: - param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids - if dependency_map_unique_names: - param._pending_dependency_map_unique_names = dependency_map_unique_names - # Keep parameter as independent initially - will be made dependent after all objects are loaded - param._independent = True - - return param - def __copy__(self) -> Parameter: new_obj = super().__copy__() new_obj._callback = property() @@ -990,61 +987,3 @@ def __abs__(self) -> Parameter: parameter = Parameter.from_scipp(name=self.name, full_value=new_full_value, min=min_value, max=max_value) parameter.name = parameter.unique_name return parameter - - def resolve_pending_dependencies(self) -> None: - """Resolve pending dependencies after deserialization. - - This method should be called after all parameters have been deserialized - to establish dependency relationships using dependency_ids or unique_names as fallback. - """ - if hasattr(self, '_pending_dependency_string'): - dependency_string = self._pending_dependency_string - dependency_map = {} - - # Try dependency IDs first (more reliable) - if hasattr(self, '_pending_dependency_map_dependency_ids'): - dependency_map_dependency_ids = self._pending_dependency_map_dependency_ids - - # Build dependency_map by looking up objects by dependency_id - for key, dependency_id in dependency_map_dependency_ids.items(): - dep_obj = self._find_parameter_by_dependency_id(dependency_id) - if dep_obj is not None: - dependency_map[key] = dep_obj - else: - raise ValueError(f"Cannot find parameter with dependency_id '{dependency_id}'") - - # Fallback to unique_names if dependency IDs not available or incomplete - if hasattr(self, '_pending_dependency_map_unique_names'): - dependency_map_unique_names = self._pending_dependency_map_unique_names - - for key, unique_name in dependency_map_unique_names.items(): - if key not in dependency_map: # Only add if not already resolved by dependency_id - try: - # Look up the parameter by unique_name in the global map - dep_obj = self._global_object.map.get_item_by_key(unique_name) - if dep_obj is not None: - dependency_map[key] = dep_obj - else: - raise ValueError(f"Cannot find parameter with unique_name '{unique_name}'") - except Exception as e: - raise ValueError(f"Error resolving dependency '{key}' -> '{unique_name}': {e}") - - # Establish the dependency relationship - try: - self.make_dependent_on(dependency_expression=dependency_string, dependency_map=dependency_map) - except Exception as e: - raise ValueError(f"Error establishing dependency '{dependency_string}': {e}") - - # Clean up temporary attributes - delattr(self, '_pending_dependency_string') - if hasattr(self, '_pending_dependency_map_dependency_ids'): - delattr(self, '_pending_dependency_map_dependency_ids') - if hasattr(self, '_pending_dependency_map_unique_names'): - delattr(self, '_pending_dependency_map_unique_names') - - def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['Parameter']: - """Find a parameter by its dependency_id from all parameters in the global map.""" - for obj in self._global_object.map._store.values(): - if isinstance(obj, Parameter) and hasattr(obj, '__dependency_id') and obj.__dependency_id == dependency_id: - return obj - return None diff --git a/src/easyscience/variable/parameter_dependency_resolver.py b/src/easyscience/variable/parameter_dependency_resolver.py index bbd84214..bbb5789f 100644 --- a/src/easyscience/variable/parameter_dependency_resolver.py +++ b/src/easyscience/variable/parameter_dependency_resolver.py @@ -67,7 +67,7 @@ def _collect_parameters(item: Any, parameters: List[Parameter]) -> None: resolved_count += 1 except Exception as e: error_count += 1 - dependency_id = getattr(param, '__dependency_id', 'unknown') + dependency_id = getattr(param, '_dependency_id', 'unknown') errors.append(f"Failed to resolve dependencies for parameter '{param.name}'" \ f" (unique_name: '{param.unique_name}', dependency_id: '{dependency_id}'): {e}") diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index fcfe31fa..af916153 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -40,8 +40,8 @@ def test_independent_parameter_serialization(self, clear_global_map): # Should not contain dependency fields assert '_dependency_string' not in serialized - assert '_dependency_map_unique_names' not in serialized - assert serialized['_independent'] is True + assert '_dependency_map_dependency_ids' not in serialized + assert '_independent' not in serialized # Deserialize global_object.map._clear() @@ -71,7 +71,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should contain dependency information assert serialized['_dependency_string'] == "2 * a" - assert serialized['_dependency_map_unique_names'] == {"a": a.unique_name} + assert serialized['_dependency_map_dependency_ids'] == {"a": a._dependency_id} assert serialized['_independent'] is False # Deserialize @@ -81,7 +81,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should have pending dependency info assert hasattr(new_b, '_pending_dependency_string') assert new_b._pending_dependency_string == "2 * a" - assert new_b._pending_dependency_map_unique_names == {"a": a.unique_name} + assert new_b._pending_dependency_map_dependency_ids == {"a": a._dependency_id} assert new_b.independent is True # Initially independent until dependencies resolved def test_dependency_resolution_after_deserialization(self, clear_global_map): @@ -149,8 +149,8 @@ def test_unique_name_dependency_serialization(self, clear_global_map): # Should contain unique name mapping assert b_serialized['_dependency_string'] == '2 * "Parameter_0"' - assert "__Parameter_0__" in b_serialized['_dependency_map_unique_names'] - assert b_serialized['_dependency_map_unique_names']["__Parameter_0__"] == a.unique_name + assert "__Parameter_0__" in b_serialized['_dependency_map_dependency_ids'] + assert b_serialized['_dependency_map_dependency_ids']["__Parameter_0__"] == a._dependency_id # Deserialize both and resolve global_object.map._clear() @@ -295,7 +295,7 @@ def test_error_handling_missing_dependency(self, clear_global_map): new_b = Parameter.from_dict(b_data) # Should raise error when trying to resolve - with pytest.raises(ValueError, match="Error resolving dependency"): + with pytest.raises(ValueError, match="Cannot find parameter with dependency_id"): new_b.resolve_pending_dependencies() def test_backward_compatibility_base_deserializer(self, clear_global_map): @@ -323,8 +323,15 @@ def test_backward_compatibility_base_deserializer(self, clear_global_map): assert deserialized.name == "b" assert deserialized.independent is True # Base path doesn't handle dependencies - def test_dependency_id_system_order_independence(self, clear_global_map): + @pytest.mark.parametrize("order", [ + ["x", "y", "z"], + ["z", "x", "y"], + ["y", "z", "x"], + ["z", "y", "x"] + ], ids=['normal_order', 'dependent_first', 'mixed_order', 'dependent_first_reverse']) + def test_dependency_id_system_order_independence(self, clear_global_map, order): """Test that dependency IDs allow parameters to be loaded in any order.""" + # WHEN # Create parameters with dependencies x = Parameter(name="x", value=5.0, unit="m", min=0, max=20) y = Parameter(name="y", value=10.0, unit="m", min=0, max=30) @@ -340,9 +347,8 @@ def test_dependency_id_system_order_independence(self, clear_global_map): assert z.value == 50.0 # 5 * 10 # Get dependency IDs - x_dep_id = x.dependency_id - y_dep_id = y.dependency_id - z_dep_id = z.dependency_id + x_dep_id = x._dependency_id + y_dep_id = y._dependency_id # Serialize all parameters params_data = { @@ -352,78 +358,37 @@ def test_dependency_id_system_order_independence(self, clear_global_map): } # Verify dependency IDs are in serialized data - assert params_data["x"]["__dependency_id"] == x_dep_id - assert params_data["y"]["__dependency_id"] == y_dep_id - assert params_data["z"]["__dependency_id"] == z_dep_id + assert params_data["x"]["_dependency_id"] == x_dep_id + assert params_data["y"]["_dependency_id"] == y_dep_id + assert "_dependency_id" not in params_data["z"] assert "_dependency_map_dependency_ids" in params_data["z"] - # Test loading in different orders - test_orders = [ - ["x", "y", "z"], # Normal order - ["z", "x", "y"], # Dependent first - ["y", "z", "x"], # Mixed order - ["z", "y", "x"] # Dependent first, reverse order - ] - - for order in test_orders: - global_object.map._clear() - new_params = {} - - # Load in the specified order - for name in order: - new_params[name] = Parameter.from_dict(params_data[name]) - - # Verify dependency IDs are preserved - assert new_params["x"].dependency_id == x_dep_id - assert new_params["y"].dependency_id == y_dep_id - assert new_params["z"].dependency_id == z_dep_id - - # Resolve dependencies - resolve_all_parameter_dependencies(new_params) - - # Verify functionality regardless of loading order - assert new_params["z"].independent is False - assert new_params["z"].value == 50.0 - - # Test dependency updates still work - new_params["x"].value = 6.0 - assert new_params["z"].value == 60.0 # 6 * 10 + # THEN + global_object.map._clear() + new_params = {} - new_params["y"].value = 8.0 - assert new_params["z"].value == 48.0 # 6 * 8 + # Load in the specified order + for name in order: + new_params[name] = Parameter.from_dict(params_data[name]) - def test_dependency_id_fallback_to_unique_name(self, clear_global_map): - """Test that the system falls back to unique_name when dependency_id is not available.""" - # Create parameters - a = Parameter(name="a", value=3.0, unit="m") - b = Parameter.from_dependency( - name="b", - dependency_expression="2 * a", - dependency_map={"a": a}, - unit="m" - ) - - # Serialize parameters - params_data = { - "a": a.as_dict(), - "b": b.as_dict() - } + # EXPECT + # Verify dependency IDs are preserved + assert new_params["x"]._dependency_id == x_dep_id + assert new_params["y"]._dependency_id == y_dep_id - # Remove dependency_id information to simulate old format - del params_data["b"]["_dependency_map_dependency_ids"] - # Keep unique_name mapping for fallback + # Resolve dependencies + resolve_all_parameter_dependencies(new_params) - # Deserialize and resolve - global_object.map._clear() - new_params = {} - for name, data in params_data.items(): - new_params[name] = Parameter.from_dict(data) + # Verify functionality regardless of loading order + assert new_params["z"].independent is False + assert new_params["z"].value == 50.0 - resolve_all_parameter_dependencies(new_params) + # Test dependency updates still work + new_params["x"].value = 6.0 + assert new_params["z"].value == 60.0 # 6 * 10 - # Should still work using unique_name fallback - assert new_params["b"].independent is False - assert new_params["b"].value == 6.0 # 2 * 3 + new_params["y"].value = 8.0 + assert new_params["z"].value == 48.0 # 6 * 8 def test_deserialize_and_resolve_parameters_helper(self, clear_global_map): """Test the convenience helper function for deserialization and dependency resolution.""" @@ -482,28 +447,3 @@ def test_deserialize_and_resolve_parameters_helper(self, clear_global_map): pending = get_parameters_with_pending_dependencies(new_params) assert len(pending) == 0 - def test_serialization_error_dependency_without_unique_name(self, clear_global_map): - """Test that serialization raises an appropriate error when a dependency object lacks unique_name.""" - # Create a parameter with dependencies - a = Parameter(name="a", value=2.0, unit="m", min=0, max=10) - b = Parameter.from_dependency( - name="b", - dependency_expression="2 * a", - dependency_map={"a": a}, - unit="m" - ) - - # Artificially create a mock object without unique_name in the dependency map - # This is a defensive test since under normal circumstances this shouldn't happen - mock_obj = Mock() - # Mock obj has no unique_name attribute - if hasattr(mock_obj, 'unique_name'): - delattr(mock_obj, 'unique_name') - - # Manually inject the mock object into the dependency map to trigger the error - # This bypasses the normal type checking that would prevent this - b._dependency_map["mock"] = mock_obj - - # Attempt to serialize should raise a ValueError - with pytest.raises(ValueError, match="does not have a unique_name attribute"): - b.as_dict() From 033d47a707eafeb092ef85b02c4c125a19ac2cbc Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Mon, 10 Nov 2025 13:58:01 +0100 Subject: [PATCH 2/6] Move code blocks back to easier reviewing. Consider reverting this change after review. --- src/easyscience/variable/parameter.py | 128 +++++++++++++------------- 1 file changed, 64 insertions(+), 64 deletions(-) diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index 49c172c4..3714bd00 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -548,70 +548,6 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: return raw_dict - @classmethod - def from_dict(cls, obj_dict: dict) -> 'Parameter': - """ - Custom deserialization to handle parameter dependencies. - Override the parent method to handle dependency information. - """ - # Extract dependency information before creating the parameter - raw_dict = obj_dict.copy() # Don't modify the original dict - dependency_string = raw_dict.pop('_dependency_string', None) - dependency_map_dependency_ids = raw_dict.pop('_dependency_map_dependency_ids', None) - is_independent = raw_dict.pop('_independent', True) - # Note: Keep _dependency_id in the dict so it gets passed to __init__ - - # Create the parameter using the base class method (dependency_id is now handled in __init__) - param = super().from_dict(raw_dict) - - # Store dependency information for later resolution - if not is_independent: - param._pending_dependency_string = dependency_string - param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids - # Keep parameter as independent initially - will be made dependent after all objects are loaded - param._independent = True - - return param - - def resolve_pending_dependencies(self) -> None: - """Resolve pending dependencies after deserialization. - - This method should be called after all parameters have been deserialized - to establish dependency relationships using dependency_ids. - """ - if hasattr(self, '_pending_dependency_string'): - dependency_string = self._pending_dependency_string - dependency_map = {} - - if hasattr(self, '_pending_dependency_map_dependency_ids'): - dependency_map_dependency_ids = self._pending_dependency_map_dependency_ids - - # Build dependency_map by looking up objects by dependency_id - for key, dependency_id in dependency_map_dependency_ids.items(): - dep_obj = self._find_parameter_by_dependency_id(dependency_id) - if dep_obj is not None: - dependency_map[key] = dep_obj - else: - raise ValueError(f"Cannot find parameter with dependency_id '{dependency_id}'") - - # Establish the dependency relationship - try: - self.make_dependent_on(dependency_expression=dependency_string, dependency_map=dependency_map) - except Exception as e: - raise ValueError(f"Error establishing dependency '{dependency_string}': {e}") - - # Clean up temporary attributes - delattr(self, '_pending_dependency_string') - delattr(self, '_pending_dependency_map_dependency_ids') - - def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: - """Find a parameter by its dependency_id from all parameters in the global map.""" - for obj in self._global_object.map._store.values(): - if isinstance(obj, DescriptorNumber) and hasattr(obj, '_dependency_id') and obj._dependency_id == dependency_id: - return obj - return None - - def _revert_dependency(self, skip_detach=False) -> None: """ Revert the dependency to the old dependency. This is used when an error is raised during setting the dependency. @@ -654,6 +590,31 @@ def _process_dependency_unique_names(self, dependency_expression: str): raise ValueError(f'The object with unique_name {stripped_name} is not a Parameter or DescriptorNumber. Please check your dependency expression.') # noqa: E501 self._clean_dependency_string = clean_dependency_string + @classmethod + def from_dict(cls, obj_dict: dict) -> 'Parameter': + """ + Custom deserialization to handle parameter dependencies. + Override the parent method to handle dependency information. + """ + # Extract dependency information before creating the parameter + raw_dict = obj_dict.copy() # Don't modify the original dict + dependency_string = raw_dict.pop('_dependency_string', None) + dependency_map_dependency_ids = raw_dict.pop('_dependency_map_dependency_ids', None) + is_independent = raw_dict.pop('_independent', True) + # Note: Keep _dependency_id in the dict so it gets passed to __init__ + + # Create the parameter using the base class method (dependency_id is now handled in __init__) + param = super().from_dict(raw_dict) + + # Store dependency information for later resolution + if not is_independent: + param._pending_dependency_string = dependency_string + param._pending_dependency_map_dependency_ids = dependency_map_dependency_ids + # Keep parameter as independent initially - will be made dependent after all objects are loaded + param._independent = True + + return param + def __copy__(self) -> Parameter: new_obj = super().__copy__() new_obj._callback = property() @@ -987,3 +948,42 @@ def __abs__(self) -> Parameter: parameter = Parameter.from_scipp(name=self.name, full_value=new_full_value, min=min_value, max=max_value) parameter.name = parameter.unique_name return parameter + + def resolve_pending_dependencies(self) -> None: + """Resolve pending dependencies after deserialization. + + This method should be called after all parameters have been deserialized + to establish dependency relationships using dependency_ids. + """ + if hasattr(self, '_pending_dependency_string'): + dependency_string = self._pending_dependency_string + dependency_map = {} + + if hasattr(self, '_pending_dependency_map_dependency_ids'): + dependency_map_dependency_ids = self._pending_dependency_map_dependency_ids + + # Build dependency_map by looking up objects by dependency_id + for key, dependency_id in dependency_map_dependency_ids.items(): + dep_obj = self._find_parameter_by_dependency_id(dependency_id) + if dep_obj is not None: + dependency_map[key] = dep_obj + else: + raise ValueError(f"Cannot find parameter with dependency_id '{dependency_id}'") + + # Establish the dependency relationship + try: + self.make_dependent_on(dependency_expression=dependency_string, dependency_map=dependency_map) + except Exception as e: + raise ValueError(f"Error establishing dependency '{dependency_string}': {e}") + + # Clean up temporary attributes + delattr(self, '_pending_dependency_string') + delattr(self, '_pending_dependency_map_dependency_ids') + + def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: + """Find a parameter by its dependency_id from all parameters in the global map.""" + for obj in self._global_object.map._store.values(): + if isinstance(obj, DescriptorNumber) and hasattr(obj, '_dependency_id') and obj._dependency_id == dependency_id: + return obj + return None + \ No newline at end of file From afbe207fe673e227a8a435d1255700918e23c6aa Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Thu, 13 Nov 2025 16:23:24 +0100 Subject: [PATCH 3/6] temp --- .../variable/test_parameter_dependency_serialization.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index af916153..df9a3185 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -154,13 +154,16 @@ def test_unique_name_dependency_serialization(self, clear_global_map): # Deserialize both and resolve global_object.map._clear() - new_a = Parameter.from_dict(a_serialized) new_b = Parameter.from_dict(b_serialized) + new_a = Parameter.from_dict(a_serialized) resolve_all_parameter_dependencies({"a": new_a, "b": new_b}) + assert False # To remember that this needs to be fixed, currently falsely succeeds. + # Should work correctly assert new_b.independent is False - assert new_b.value == 6.0 # 2 * 3 + new_a.value = 4.0 + assert new_b.value == 8.0 # 2 * 4 def test_json_serialization_roundtrip(self, clear_global_map): """Test that parameter dependencies survive JSON serialization.""" From 696f7ee57ce039355d10e3d21fb86d84b0a65953 Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Fri, 14 Nov 2025 13:20:43 +0100 Subject: [PATCH 4/6] Name-mangle dependency_id. --- src/easyscience/variable/descriptor_number.py | 14 ++++++------- src/easyscience/variable/parameter.py | 4 ++-- .../variable/parameter_dependency_resolver.py | 2 +- ...test_parameter_dependency_serialization.py | 20 +++++++++---------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/easyscience/variable/descriptor_number.py b/src/easyscience/variable/descriptor_number.py index c72edb3c..b798caa4 100644 --- a/src/easyscience/variable/descriptor_number.py +++ b/src/easyscience/variable/descriptor_number.py @@ -68,8 +68,8 @@ def __init__( self._observers: List[DescriptorNumber] = [] # Extract dependency_id if provided during deserialization - if '_dependency_id' in kwargs: - self._dependency_id = kwargs.pop('_dependency_id') + if '__dependency_id' in kwargs: + self.__dependency_id = kwargs.pop('__dependency_id') if not isinstance(value, numbers.Number) or isinstance(value, bool): raise TypeError(f'{value=} must be a number') @@ -118,14 +118,14 @@ def from_scipp(cls, name: str, full_value: Variable, **kwargs) -> DescriptorNumb def _attach_observer(self, observer: DescriptorNumber) -> None: """Attach an observer to the descriptor.""" self._observers.append(observer) - if not hasattr(self, '_dependency_id'): - self._dependency_id = str(uuid.uuid4()) + if not hasattr(self, '_DescriptorNumber__dependency_id'): + self.__dependency_id = str(uuid.uuid4()) def _detach_observer(self, observer: DescriptorNumber) -> None: """Detach an observer from the descriptor.""" self._observers.remove(observer) if not self._observers: - del self._dependency_id + del self.__dependency_id def _notify_observers(self) -> None: """Notify all observers of a change.""" @@ -333,8 +333,8 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: raw_dict['value'] = self._scalar.value raw_dict['unit'] = str(self._scalar.unit) raw_dict['variance'] = self._scalar.variance - if hasattr(self, '_dependency_id'): - raw_dict['_dependency_id'] = self._dependency_id + if hasattr(self, '_DescriptorNumber__dependency_id'): + raw_dict['__dependency_id'] = self.__dependency_id return raw_dict def __add__(self, other: Union[DescriptorNumber, numbers.Number]) -> DescriptorNumber: diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index 3714bd00..1926cd50 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -544,7 +544,7 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: # Convert dependency_map to use dependency_ids raw_dict['_dependency_map_dependency_ids'] = {} for key, obj in self._dependency_map.items(): - raw_dict['_dependency_map_dependency_ids'][key] = obj._dependency_id + raw_dict['_dependency_map_dependency_ids'][key] = obj._DescriptorNumber__dependency_id return raw_dict @@ -983,7 +983,7 @@ def resolve_pending_dependencies(self) -> None: def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: """Find a parameter by its dependency_id from all parameters in the global map.""" for obj in self._global_object.map._store.values(): - if isinstance(obj, DescriptorNumber) and hasattr(obj, '_dependency_id') and obj._dependency_id == dependency_id: + if isinstance(obj, DescriptorNumber) and hasattr(obj, '_DescriptorNumber__dependency_id') and obj._DescriptorNumber__dependency_id == dependency_id: return obj return None \ No newline at end of file diff --git a/src/easyscience/variable/parameter_dependency_resolver.py b/src/easyscience/variable/parameter_dependency_resolver.py index bbb5789f..ad084e70 100644 --- a/src/easyscience/variable/parameter_dependency_resolver.py +++ b/src/easyscience/variable/parameter_dependency_resolver.py @@ -67,7 +67,7 @@ def _collect_parameters(item: Any, parameters: List[Parameter]) -> None: resolved_count += 1 except Exception as e: error_count += 1 - dependency_id = getattr(param, '_dependency_id', 'unknown') + dependency_id = getattr(param, '_DescriptorNumber__dependency_id', 'unknown') errors.append(f"Failed to resolve dependencies for parameter '{param.name}'" \ f" (unique_name: '{param.unique_name}', dependency_id: '{dependency_id}'): {e}") diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index df9a3185..a096efa8 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -71,7 +71,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should contain dependency information assert serialized['_dependency_string'] == "2 * a" - assert serialized['_dependency_map_dependency_ids'] == {"a": a._dependency_id} + assert serialized['_dependency_map_dependency_ids'] == {"a": a._DescriptorNumber__dependency_id} assert serialized['_independent'] is False # Deserialize @@ -81,7 +81,7 @@ def test_dependent_parameter_serialization(self, clear_global_map): # Should have pending dependency info assert hasattr(new_b, '_pending_dependency_string') assert new_b._pending_dependency_string == "2 * a" - assert new_b._pending_dependency_map_dependency_ids == {"a": a._dependency_id} + assert new_b._pending_dependency_map_dependency_ids == {"a": a._DescriptorNumber__dependency_id} assert new_b.independent is True # Initially independent until dependencies resolved def test_dependency_resolution_after_deserialization(self, clear_global_map): @@ -150,7 +150,7 @@ def test_unique_name_dependency_serialization(self, clear_global_map): # Should contain unique name mapping assert b_serialized['_dependency_string'] == '2 * "Parameter_0"' assert "__Parameter_0__" in b_serialized['_dependency_map_dependency_ids'] - assert b_serialized['_dependency_map_dependency_ids']["__Parameter_0__"] == a._dependency_id + assert b_serialized['_dependency_map_dependency_ids']["__Parameter_0__"] == a._DescriptorNumber__dependency_id # Deserialize both and resolve global_object.map._clear() @@ -350,8 +350,8 @@ def test_dependency_id_system_order_independence(self, clear_global_map, order): assert z.value == 50.0 # 5 * 10 # Get dependency IDs - x_dep_id = x._dependency_id - y_dep_id = y._dependency_id + x_dep_id = x._DescriptorNumber__dependency_id + y_dep_id = y._DescriptorNumber__dependency_id # Serialize all parameters params_data = { @@ -361,9 +361,9 @@ def test_dependency_id_system_order_independence(self, clear_global_map, order): } # Verify dependency IDs are in serialized data - assert params_data["x"]["_dependency_id"] == x_dep_id - assert params_data["y"]["_dependency_id"] == y_dep_id - assert "_dependency_id" not in params_data["z"] + assert params_data["x"]["__dependency_id"] == x_dep_id + assert params_data["y"]["__dependency_id"] == y_dep_id + assert "__dependency_id" not in params_data["z"] assert "_dependency_map_dependency_ids" in params_data["z"] # THEN @@ -376,8 +376,8 @@ def test_dependency_id_system_order_independence(self, clear_global_map, order): # EXPECT # Verify dependency IDs are preserved - assert new_params["x"]._dependency_id == x_dep_id - assert new_params["y"]._dependency_id == y_dep_id + assert new_params["x"]._DescriptorNumber__dependency_id == x_dep_id + assert new_params["y"]._DescriptorNumber__dependency_id == y_dep_id # Resolve dependencies resolve_all_parameter_dependencies(new_params) From 57085e7c81e97e5a1e6c47c3f59d3dd3d3249f58 Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Fri, 14 Nov 2025 14:27:47 +0100 Subject: [PATCH 5/6] Fix error with serialization of unique_name dependency expressions --- src/easyscience/variable/parameter.py | 2 +- .../variable/test_parameter_dependency_serialization.py | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index 1926cd50..f49f5512 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -536,7 +536,7 @@ def as_dict(self, skip: Optional[List[str]] = None) -> Dict[str, Any]: # Add dependency information for dependent parameters if not self._independent: # Save the dependency expression - raw_dict['_dependency_string'] = self._dependency_string + raw_dict['_dependency_string'] = self._clean_dependency_string # Mark that this parameter is dependent raw_dict['_independent'] = self._independent diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index a096efa8..5e89dbf6 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -147,18 +147,21 @@ def test_unique_name_dependency_serialization(self, clear_global_map): a_serialized = a.as_dict() b_serialized = b.as_dict() + del a_serialized['unique_name'] # Remove unique_name to force new assignment on deserialization + del b_serialized['unique_name'] + # Should contain unique name mapping - assert b_serialized['_dependency_string'] == '2 * "Parameter_0"' + assert b_serialized['_dependency_string'] == '2 * __Parameter_0__' assert "__Parameter_0__" in b_serialized['_dependency_map_dependency_ids'] assert b_serialized['_dependency_map_dependency_ids']["__Parameter_0__"] == a._DescriptorNumber__dependency_id # Deserialize both and resolve global_object.map._clear() + c = Parameter(name='c', value=0.0) # Dummy to occupy unique name, to force new unique_names + new_b = Parameter.from_dict(b_serialized) new_a = Parameter.from_dict(a_serialized) resolve_all_parameter_dependencies({"a": new_a, "b": new_b}) - - assert False # To remember that this needs to be fixed, currently falsely succeeds. # Should work correctly assert new_b.independent is False From 753fd4a3b5ea8b6669eca5732ec53ec04955f26d Mon Sep 17 00:00:00 2001 From: Christian Vedel Date: Mon, 17 Nov 2025 11:02:11 +0100 Subject: [PATCH 6/6] Add test for DescriptorNumber dependency serialization. --- src/easyscience/variable/parameter.py | 5 ++- ...test_parameter_dependency_serialization.py | 44 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/easyscience/variable/parameter.py b/src/easyscience/variable/parameter.py index f49f5512..3714ba7b 100644 --- a/src/easyscience/variable/parameter.py +++ b/src/easyscience/variable/parameter.py @@ -983,7 +983,8 @@ def resolve_pending_dependencies(self) -> None: def _find_parameter_by_dependency_id(self, dependency_id: str) -> Optional['DescriptorNumber']: """Find a parameter by its dependency_id from all parameters in the global map.""" for obj in self._global_object.map._store.values(): - if isinstance(obj, DescriptorNumber) and hasattr(obj, '_DescriptorNumber__dependency_id') and obj._DescriptorNumber__dependency_id == dependency_id: - return obj + if isinstance(obj, DescriptorNumber) and hasattr(obj, '_DescriptorNumber__dependency_id'): + if obj._DescriptorNumber__dependency_id == dependency_id: + return obj return None \ No newline at end of file diff --git a/tests/unit_tests/variable/test_parameter_dependency_serialization.py b/tests/unit_tests/variable/test_parameter_dependency_serialization.py index 5e89dbf6..4b741bdc 100644 --- a/tests/unit_tests/variable/test_parameter_dependency_serialization.py +++ b/tests/unit_tests/variable/test_parameter_dependency_serialization.py @@ -253,6 +253,50 @@ def test_multiple_dependent_parameters(self, clear_global_map): assert new_params["y"].value == 10.0 # 2 * 5 assert new_params["z"].value == 15.0 # 10 + 5 + def test_dependency_with_descriptor_number(self, clear_global_map): + """Test that dependencies involving DescriptorNumber serialize correctly.""" + from easyscience.variable import DescriptorNumber + # When + + x = DescriptorNumber(name="x", value=3.0, unit="m") + y = Parameter(name="y", value=4.0, unit="m") + z = Parameter.from_dependency( + name="z", + dependency_expression="x + y", + dependency_map={"x": x, "y": y}, + ) + + # Verify original functionality + assert z.value == 7.0 # 3 + 4 + + # Then + # Serialize all + params_data = { + "x": x.as_dict(), + "y": y.as_dict(), + "z": z.as_dict() + } + # Deserialize and resolve + global_object.map._clear() + new_params = {} + for name, data in params_data.items(): + if name == "x": + new_params[name] = DescriptorNumber.from_dict(data) + else: + new_params[name] = Parameter.from_dict(data) + + resolve_all_parameter_dependencies(new_params) + + # Expect + # Test that functionality still works + assert new_params["z"].value == 7.0 # 3 + 4 + new_x = new_params["x"] + new_y = new_params["y"] + new_x.value = 4.0 + assert new_params["z"].value == 8.0 # 4 + 4 + new_y.value = 6.0 + assert new_params["z"].value == 10.0 # 4 + 6 + def test_get_parameters_with_pending_dependencies(self, clear_global_map): """Test utility function for finding parameters with pending dependencies.""" # Create parameters