Implement ObserverGraph with NamedTraitObserver #976

kitchoi · 2020-04-01T11:45:33Z

This PR implements a few low-level objects required for implementing the new observer framework described in EEP-3.

Contribute towards #977

This PR mainly introduces the following:

~~ObserverPath~~ ObserverGraph:
A data structure that represents the attribute paths for traits being observed on an HasTraits instance

The following is included for context, I am happy to move them out to a separate PR

IObserver:
Interface for an observer. Currently it requires __eq__ and __hash__ as are required for ~~ObserverPath~~ ObserverGraph.
NamedTraitObserver:
An observer that can be used as a node in ~~ObserverPath~~ ObserverGraph.

Checklist

Tests
Update API reference (docs/source/traits_api_reference)
All the modules introduced here are private. I don't think the API reference includes private modules, but I could be wrong.
Update User manual (docs/source/traits_user_manual)
All the modules introduced here are private.
Update type annotation hints in traits-stubs
Nothing introduced in this PR are trait-ed.

Note:
The proof-of-concept PR is still being reviewed in #942. So there is still a chance this PR will need to be closed altogether or reworked.

…erPath

kitchoi · 2020-04-01T16:50:49Z

traits/observers/_observer_path.py

+# Thanks for using Enthought open source!
+
+
+class ObserverPath:


I don't know why I only realized this now: But this object actually represents many "paths", not just one.
An alternative name welcome...

Would ObserverTree be appropriate? ObserverGraph? ObserverTree certainly helps me when looking at the code: then I'm not surprised to see contents for this node along with a list of child nodes.

ObserverGraph? Or even ObserverDGraph? Trees can't have cycles and I am not sure we can confidently rule that possibility out at the moment...

kitchoi · 2020-04-07T17:24:34Z

traits/observers/_named_trait_observer.py

+# is also available online at http://www.enthought.com/licenses/BSD.txt
+#
+# Thanks for using Enthought open source!
+from traits.observers._interfaces import IObserver


On naming, should the module be named _iobserver and we have one module per interface object?

One module per IObserver subclass sounds fine to me, especially since we have a traits.observers subpackage (so we won't be cluttering the main traits package).

So _named_trait_observer seems like a fine name.

I ended up naming this module to _i_observer (not the _iobserver I initially proposed), to be inlined with naming in pyface (maybe elsewhere too).

kitchoi · 2020-04-07T17:28:48Z

traits/observers/_observer_path.py

+        # Remove loops
+        self_nexts = set(path for path in self.nexts if path is not self)
+        other_nexts = set(path for path in other.nexts if path is not other)


We can also handle loops later when we implement the methods for supporting recursion.
Handling just the loop back to self is not enough actually, considering the example of root.[left,right]*.value, there are actually two paths in the loop.

#969 attempted an alternative for handling cycles. I will remove this loop handling here.

codecov-io · 2020-04-14T12:50:14Z

Codecov Report

Merging #976 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #976   +/-   ##
=======================================
  Coverage   74.92%   74.92%           
=======================================
  Files          54       54           
  Lines        6477     6477           
  Branches     1280     1280           
=======================================
  Hits         4853     4853           
  Misses       1253     1253           
  Partials      371      371

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05889d5...05889d5. Read the comment docs.

kitchoi · 2020-04-17T13:59:04Z

traits/observers/_observer_graph.py

+    def __hash__(self):
+        """ Return the hash of this ObserverGraph."""
+        return hash(
+            (type(self), self.node, frozenset(self.children))


Previously I had hash(frozenset(self.children)) here, the hash is a bit redundant, and is removed.

kitchoi · 2020-04-17T14:24:22Z

Thank you for the review!
The class is now renamed to ObserverGraph and so I also updated the title and description. Ready for re-review.

mdickinson · 2020-04-18T08:51:43Z

One question about the use of sets here: will the unpredictability of the set ordering leak into user-visible behaviour (e.g., notifiers firing in different orders from run to run)?

While it's true that well-written code shouldn't care about the order in which listeners are fired, I think we probably do still want to have predictability here.

mdickinson · 2020-04-20T07:21:53Z

One other thought on the set ordering: right now that ordering should be genuinely unpredictable from run to run, thanks to the inclusion of the types themselves in the __hash__ algorithms. If we change that (e.g., by replacing the type with the name of the type, so that we have deterministic __hash__) then we'll end up in an arguably worse situation, which is that on any given platform behaviour will be consistent from run to run, but there could be platform differences (e.g., due to the hash on Windows not matching that on macOS/Linux). The latter seems like a recipe for hard-to-diagnose bugs.

kitchoi · 2020-04-20T08:05:19Z

Good question. In order to address that, I need to understand if traits make any promises about (1) the order of notifications and (2) whether the order of notifications is deterministic. For (1), what order is promised?

Before dict becomes order, on_trait_change listeners are registered in a dict and have always been iterated through in an order that is not deterministic. But the fact that dict becomes ordered in CPython 3.6/Python 3.7 might mean traits has extended that order guarantee.

mdickinson · 2020-04-20T08:10:20Z

No promises, except that it's known through folklore that static listeners (those arising from _mytrait_changed magic method names) fire before dynamic ones (those coming from @on_trait_change-decorated methods, or direct on_trait_change calls on objects). I don't know if that's actually written down in the documentation, but if not, it should be. Beyond that, I don't think we have any guarantees.

Nevertheless, we've definitely seen code that happens to work only because listeners happen to be fired in the right order. And with developers often working on macOS or Linux but deploying on Windows, I'd like to be sure that we don't end up in a situation where the behaviour is consistent from run to run on local machines, but differs from platform to platform.

kitchoi · 2020-04-20T08:34:30Z

Related doc question: Should it be documented that users should not make assumptions on the order for when the change handlers are called?

e.g. the following may assume _handle_name_change is called after _update_number, and should be discouraged even if it works now:

class Foo(HasTraits):
      @on_trait_change("name")
       def _update_number(self):
             self.number += 1

      @on_trait_change("name")
      def _handle_name_change(self):
             self.packages = [self.name] * self.number

mdickinson · 2020-04-20T08:36:18Z

Yes, should definitely be documented, and that would be a nice example to include in that documentation.

kitchoi · 2020-04-20T08:38:12Z

And if the order concern is about interactions among change handlers, then I think the ordering of ObserverGraph.children would not cause that, because there is only one handler associated to the root node of the entire tree. But it probably still be a good idea to keep the order as given so we don't lose that information.

I pushed some new commits. See what you think.

kitchoi · 2020-04-20T08:54:56Z

I think the ordering of ObserverGraph.children would not cause that, because there is only one handler associated to the root node of the entire tree.

To clarify with an example, the "children" are b and c in the following:

@on_trait_change("a.[b,c]")
def update_handler(self):
    pass

So the ordering will affect whether update_handler is hooked up with b first or c first.

mdickinson

LGTM; one nitpick and one suggestion for a minor change

mdickinson · 2020-04-22T07:41:23Z

traits/observers/_i_observer.py

+# is also available online at http://www.enthought.com/licenses/BSD.txt
+#
+# Thanks for using Enthought open source!
+import abc


Just a style nitpick: could we keep a blank line separating the copyright header from the module body?

traits/observers/_named_trait_observer.py

mdickinson · 2020-04-22T08:01:46Z

traits/observers/_named_trait_observer.py

+        )
+
+
+IObserver.register(NamedTraitObserver)


Suggestion: use this as a class decorator: @IObserver.register, to better match the familiarity of the way we use @provides.

Yeah that might be more readable too!

mdickinson · 2020-04-22T08:16:34Z

LGTM!

kitchoi · 2020-04-22T08:29:22Z

Thanks! Merging.

kitchoi added 22 commits March 30, 2020 14:48

ObserverPath with basic handling of equality

24ab695

Handle more cases for equality check

87ed1c5

Add a test that fails due to loops

7e8b69b

Handling of loops in path

0a70631

Flake8

03a2d23

Rename cycle -> loop

780d205

Check the hash as well

59e01da

Expand comment and docstring

dbc4d29

Add one observer so it can be used with an ObserverPath

4d31c91

Make NamedTraitObserver immutable

658812b

Expand docstring for name

63a7363

Implement hashing on NamedTraitObserver

7608bbd

Clean up and renaming

2748837

Move the location of integration test

918fa43

Flake8

96cbeb1

Refine docstring

7e159be

Flake8

4e5d7a1

Remove unnecessary dependency on helper function in test

c9da46b

Require keyword arguments

2343b10

Expand a comment

b166dd0

Rename modules

b281326

Remove notify from IObserver for now as it is not relevant for Observ…

e31b3b0

…erPath

kitchoi mentioned this pull request Apr 1, 2020

[POC] Listener rework for EEP-3 #942

Closed

7 tasks

kitchoi commented Apr 1, 2020

View reviewed changes

kitchoi mentioned this pull request Apr 1, 2020

Implement new framework to observe and notify for trait changes #977

Closed

19 tasks

kitchoi commented Apr 7, 2020

View reviewed changes

kitchoi added 2 commits April 8, 2020 12:17

Remove redundant length check

fdf2796

Remove loops for now

1697d70

kitchoi added 4 commits April 17, 2020 14:44

Rename nexts -> children

25552f2

Rename module from _observer_path to _observer_graph

396d9b6

Rename ObserverPath to ObserverGraph

8afbe83

Remove negative assertions on hashes

5939e54

kitchoi changed the title ~~Implement ObserverPath with NamedTraitObserver~~ Implement ObserverGraph with NamedTraitObserver Apr 17, 2020

kitchoi added 2 commits April 17, 2020 14:57

Fix error messages mentioning subclasses

0203663

Remove a redundant hashing

a712224

kitchoi commented Apr 17, 2020

View reviewed changes

kitchoi requested a review from mdickinson April 17, 2020 16:24

kitchoi added 4 commits April 20, 2020 09:26

Make children ordered, but still compare regardless of ordering

3bd2323

Rename path -> graph

54d667e

Flake8

32648b3

Raise early

05889d5

Rename the test module from test_observer_path -> test_observer_graph

9255074

mdickinson approved these changes Apr 22, 2020

View reviewed changes

kitchoi added 2 commits April 22, 2020 09:10

Add a newline between copyright header and imports

df7b604

Use register as a class decorator

07b141f

kitchoi merged commit 245dcb3 into master Apr 22, 2020

kitchoi deleted the observer-path branch April 22, 2020 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ObserverGraph with NamedTraitObserver #976

Implement ObserverGraph with NamedTraitObserver #976

kitchoi commented Apr 1, 2020 •

edited

kitchoi Apr 1, 2020

mdickinson Apr 17, 2020

kitchoi Apr 17, 2020

kitchoi Apr 7, 2020

mdickinson Apr 17, 2020

kitchoi Apr 17, 2020

kitchoi Apr 7, 2020 •

edited

kitchoi Apr 14, 2020

codecov-io commented Apr 14, 2020 •

edited

kitchoi Apr 17, 2020

kitchoi commented Apr 17, 2020

mdickinson commented Apr 18, 2020

mdickinson commented Apr 20, 2020

kitchoi commented Apr 20, 2020

mdickinson commented Apr 20, 2020 •

edited

kitchoi commented Apr 20, 2020

mdickinson commented Apr 20, 2020

kitchoi commented Apr 20, 2020

kitchoi commented Apr 20, 2020

mdickinson left a comment

mdickinson Apr 22, 2020

mdickinson Apr 22, 2020

kitchoi Apr 22, 2020

mdickinson commented Apr 22, 2020

kitchoi commented Apr 22, 2020

		# Thanks for using Enthought open source!


		class ObserverPath:

		)


		IObserver.register(NamedTraitObserver)

Implement ObserverGraph with NamedTraitObserver #976

Implement ObserverGraph with NamedTraitObserver #976

Conversation

kitchoi commented Apr 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitchoi Apr 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 14, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

kitchoi commented Apr 17, 2020

mdickinson commented Apr 18, 2020

mdickinson commented Apr 20, 2020

kitchoi commented Apr 20, 2020

mdickinson commented Apr 20, 2020 • edited

kitchoi commented Apr 20, 2020

mdickinson commented Apr 20, 2020

kitchoi commented Apr 20, 2020

kitchoi commented Apr 20, 2020

mdickinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Apr 22, 2020

kitchoi commented Apr 22, 2020

kitchoi commented Apr 1, 2020 •

edited

kitchoi Apr 7, 2020 •

edited

codecov-io commented Apr 14, 2020 •

edited

mdickinson commented Apr 20, 2020 •

edited