ADR Suggestion Detecting cyclic dependencies easily with pings

[damskii9992]

General

Since parameters can be made dependent after they're created, it is possible to end up in an infinite loop where a dependent parameters update triggers another dependent parameters update which in turn triggers the first parameters update and so on:

flowchart LR;
   A-->|update|B;
   B-->|update|C;
   C-->|update|D;
   D-->|update|B;

Loading

In ADR #10 a system for detecting cyclic dependencies were suggested and implemented. In this ADR I propose a simpler yet smarter system, which also properly handles the edge-case described in #10.

Current Implementation

In the current implementation, the cyclic dependencies are discovered through the observer pattern with update_ids:
If an update is triggered by a manual change, such as a value change of an independent parameter, the update gets assigned a unique id.
This unique id is then passed to the observers along with the unique_name of the object that triggered the update;

def _notify_observers(self, update_id=None) -> None:
     """Trigger an update in each subscriber."""
     if update_id is None:
        self._global_object.update_id_iterator += 1
        update_id = self._global_object.update_id_iterator
     for observer in self._observers:
         observer._update(update_id=update_id, updating_object=self.unique_name)

In the observers _update method, the update id is checked against an internal dictionary for the updating object. If the stored update id for the updating object is different from the new one, set the stored update id to the new one and proceed with the update, and notify its own observers, passing along the update_id, otherwise raise an error warning that a cyclic dependency has been detected:

def __update(self, update_id: int, updating_object: str) -> None
   if self._dependency_updates[updating_object] == update_id:
      raise RuntimeError
   else:
      update stuff
      self._dependency_updates[updating_object] = update_id
      self._notify_observers(update_id=update_id)

Proposed implementation

In setting up the dependent parameter, in the make_dependent_on method, before any of the parameters values are changed, a ping is sent to all its observers (if any) by calling the _ping_observers method (implemented in the DescriptorNumber):

def _ping_observers(self, ping_origin=None) -> None:
    """Ping all observers to check if any cyclic dependencies has been introduced.

    :param ping_origin: Unique_name of the origin of this ping. Used to avoid cyclic depenencies.
    """
    if ping_origin == self.unique_name:
        raise RuntimeError('\n Cyclic dependency detected!\n' +
                f'An update of {self.unique_name} leads to it updating itself.\n' +
                'Please check your dependencies.')
    if ping_origin is None:
        ping_origin = self.unique_name
    for observer in self._observers:
        observer._ping_observers(ping_origin=ping_origin)

Only in case this dependency-traversal doesn't raise an error, is the dependent parameter updated with the dependency expression.
This approach handles the edge-case described in #10, it simplifies the code base by making the _notify_observers and _update methods simpler, and it get rids of two attributes: the update_id_iterator in the global_object and the _dependency_updates dictionary on Parameter, since the unique_name of the Parameter issuing the ping is now used instead.

[rozyczko]

Maybe returning a bool and subsequent check in the caller rather than throwing is a better idea?

def _validate_dependencies(self, origin=None) -> bool:
    if origin == self.unique_name:
        write("..error message.")
        return False

    origin = origin or self.unique_name

    for observer in self._observers:
        if not observer._validate_dependencies(origin=origin):
            return False

    return True

This way, calling _validate_dependencies feels more natural as it returns a bool, which is expected from a validate-type call.
Especially considering the reuse in libraries and apps. We should probably let the dependent code decide on throwing or just notifying and continuing.

[damskii9992]

Hmm, the problem is if we continue we encounter an infinite regression which crashes the interpreter (at least when I tried it), so I really don't see how we can continue in case of cyclic dependencies in dependent code.
Also, if one uses easyscience in isolation, making a cyclic dependency should throw an error.

Considering this, I think it makes more sense to keep the current implementation with throwing an error and if dependent code wishes to avoid that, they simply wrap the make_dependent_on calls in try/catch statements.

[rozyczko]

OK, if the cyclic dependencies indeed crash the interpreter, then the best idea is to throw.

I still prefer _validate_dependencies over _ping_observers - the intent is much clearer.

[damskii9992]

Noted, have renamed method.