[PoC][EEP-3] User-facing expression for defining target traits to observe

[kitchoi]

Related to #977

This PR is targeting the Observation Mini-Language section in EEP 3. This is another proof-of-concept PR that will be closed without merging. It is open for discussion.

This PR proposes an expression approach for describing what traits are to be observed, without introducing new syntax that are not already part of Python language, and without any need for parsing a string. This also copied over data structures introduced in another proof-of-concept PR (#942).

Benefits:

1. It is self-explanatory, therefore easier to learn as long as one learned Python (and English) already.
2. Easy to extend and to maintain (I hope): There is no need to parse anything.
3. Self-documenting. Documentation is available as docstrings, accessible from a Python prompt.
4. Self-discoverable: One can discover expressions using autocompletion on IPython prompt.
5. Avoid specially named "items", which prevents observing traits that are actually named "items"

Cost:

1. Increased verbosity

Edited: This PR also includes a text parser to translate the existing mini-language to the expression.

Examples for features supported by on_trait_change

• t("age")
  Notify when age changes.
• t("age") | t("number")
  Notify when age or number changes.
  Equivalent to "[age, number]" in the on_trait_change mini-language.
• t("child", notify=False).t("age")
  Notify when age on a child changes but only notify when child is reassigned.
  Equivalent to "child:age" in the on_trait_change mini-language.
• t("children").list_items().t("age")
  Notify for changes to a trait age on any items inside a list named children. Notify for reassignment of the list and mutation of the list as well.
  This is equivalent to ["children.age", "children_items.age"] in the on_trait_change mini-language.
• t("children", notify=False).list_items().t("age")
  Similar to the above, but not notify on reassignment on children.
  This is equivalent to ["children_items.age"] in the on_trait_change mini-language.
• t("child").then( t("age") | t("name") )
  Notify changes when child changes, or when either age or name on a child changes.
• t("root").recursive( t("left") | t("right") ).t("value")
  This matches a trait named value on a trait named left or right, matched recursively on another trait named root. e.g. it matches root.left.left.value, root.left.right.left.right.right.value.
• metadata("name", filter=lambda value: value is not None)
  Similar to HasTraits.traits filtering, this notifies for changes on traits that has a metadata named name with a value that is not None.

Examples for features NOT currently supported by on_trait_change

• filter_(lambda name, trait: name.startswith("abc")
  Observe traits with a generic filter.
  This example is equivalent to "prefix+" in the on_trait_change mini-language.
• t("children").list_items().list_items().list_items()
  Observe a nested list of list of list.
• t("children").list_items().dict_keys().t("mapping").dict_values().t("value")
  Observe a trait named "value", inside objects which are values of a dict named "mapping", which is inside objects which are dictionary keys, inside a list named "children".
  (ignore the fact one should not use mutables as dictionary keys)

Then with the observe decorator superseding on_trait_change, the usage may look like this:

@observe(t("name") | t("age"))
def handler(event):
   ...

and

Property(depends_on=(t("name") | t("age")))

Discussion:

• Is this approach generally desirable?
• Suggestions for different naming? e.g. is the shorthand t too brief?
• We could easily have items() which is equivalent to list_items() | dict_items() | set_items(), is it worth adding though?
• Suppose t("child").t("attr1") is supported, is it worth adding another top-level function for parsing a dotted string such as "child.attr1" and just this pattern? This introduces a second way to do the same thing merely for fewer keystrokes, and is a slippery slope for more complicated mini-language all over again. How sorely missed will these dotted strings be?
• Supposed we are to implement these expressions, which features should be implemented first for the next release?
• etc...

Notes:

• The implementation is probably not the most performant given all the deep copies and looping (edited: removed, but the implementation is still drafty). I wanted to test the idea and interface first, so I have not given much thought on how to speed things up. Suggestions for faster implementation are very welcome.

Checklist

• Tests
• Update API reference (docs/source/traits_api_reference)
• Update User manual (docs/source/traits_user_manual)
• Update type annotation hints in traits-stubs

[kitchoi]

For reference, traitlets does not support dotted name strings. This is traitlets's parser:
https://github.com/ipython/traitlets/blob/32ced29b0ab06914ff92e9bdcd8806536633d701/traitlets/traitlets.py#L181-L204

[kitchoi]

Updated main comment to add a section for examples that cannot be supported by the current on_trait_change mini-language.

[mdickinson]

Do we want to commit the generated parser

I think we want to commit it (in spite of the usual rules about not committing generated files), and ideally have an etstool.py command to regenerate it when necessary.

For sure we want to include the generated parser in any source distribution, else people installing from PyPI would have to have lark installed to be able to install.

[kitchoi]

From offline discussion, the first version is not going to have recursion support. We may have to add it later if it becomes obvious that the feature is well missed.

For future reference, this commit removes all the code and tests required for recursion support: ee6a376
The next commit reverts this.

Just to note an experience from working on this branch: The consideration to support recursion really drove how this branch evolved. There were two possible implementations at the start. After the attempt to try dealing with path comparisons involving cycles, it became clear that one of the two implementations was a dead end (or just became extremely difficult to work with). That said, it does not require a massive amount of code (excluding test code) to support recursion after I have settled on the one implementation that will support it.

[kitchoi]

Another note... because parse simply returns another Expression, it is possible to go somewhere between text parsing and no text parsing:

>>> api.parse("a.b:c").metadata("name").then(api.parse("d.e.f")).print()                                                                                                                                                                                                                     
 ---- Path ---- 
Node: <Name(name='a', notify=True, ...)>
    Node: <Name(name='b', notify=False, ...)>
        Node: <Name(name='c', notify=True, ...)>
            Node: <Metadata(metadata_name='name', value=<function _is_not_none at 0x1052bf160> notify=True)>
                Node: <Name(name='d', notify=True, ...)>
                    Node: <Name(name='e', notify=True, ...)>
                        Node: <Name(name='f', notify=True, ...)>

This wasn't what I initially had planned for...but maybe it is still desirable.

This PoC is aiming at discussing the API and experimenting with various implementations/tooling, and we have done both. I will be closing this PR in the next few days if there are no more discussions and experimentation needed here.

[kitchoi]

I would suggest not using t as part of the formal API (possibly trait instead), but try to make it part of the culture (like np for numpy) for those who want to reduce the amount of typing.

My apologies I did not reply to this. Renaming t to trait sounds good to me.
But it might be difficult to have a shorthand convention when it is called via the instance method, e.g. we could only go as far as this:

from api import trait as t
t("attr1").trait("attr2")

[kitchoi]

I tried to put together the expressions here, minus the recursion support, plus the notifiers implemented in other PRs, plus all the plumbing needed on HasTraits, into this mammoth branch: observer-framework-test. The diff is too big for human consumption. What I wanted to show is this notebook which shows the proposed user-facing API:

https://github.com/enthought/traits/blob/3f71d81fce787e51e7da5cd8441fed2081a5cbca/examples/tutorials/observers/Demo.ipynb

[corranwebster]

I would suggest not using t as part of the formal API (possibly trait instead), but try to make it part of the culture (like np for numpy) for those who want to reduce the amount of typing.

My apologies I did not reply to this. Renaming t to trait sounds good to me.
But it might be difficult to have a shorthand convention when it is called via the instance method, e.g. we could only go as far as this:

from api import trait as t
t("attr1").trait("attr2")

Ah, that hadn't occurred to me. I think trait is better then, since the other methods like metadata and then are fully-spelled out, and don't worry about shorthand t at all for now.

[kitchoi]

Ah, that hadn't occurred to me. I think trait is better then, since the other methods like metadata and then are fully-spelled out, and don't worry about shorthand t at all for now.

Sounds good to me!

[corranwebster]

API note: I think it would make sense to have an overloaded signature: if expression is a string, then we should parse it to get the expression.

I generally don't like overloaded API's like this, but for a top-level, frequently-used method, I think it is appropriate.

[kitchoi]

Agreed. It is fairly easy to exercise parse once only at the layer closest to the user. In this other mammoth fully-integrated branch, I tried to do this once only on HasTraits.observe instance method:

traits/traits/has_traits.py

Lines 2118 to 2142 in 3f71d81

	def observe(self, handler, expression, *, remove=False, dispatch="same"):
	""" Attach or detach event handler that would fire when one or many
	traits change.
	Pararameters
	------------
	handler : callable(event)
	A callable that will receive the change event when the observed
	trait changes.
	expression : str or Expression
	A description of what traits are being observed.
	remove : boolean, optional
	Whether to remove the event handler.
	dispatch : str, optional
	Option for how the handler is dispatched.
	"""
	if isinstance(expression, str):
	expression = _observers_api.parse(expression)
	_observers_api.observe(
	object=self,
	expression=expression,
	handler=handler,
	remove=remove,
	dispatcher=_observer_dispatchers[dispatch],
	)

And then the Expression gets converted to ObserverGraph as you go deeper into the stack.

traits/traits/observers/observe.py

Lines 21 to 52 in 3f71d81

	def observe(
	object, expression, handler,
	*, remove=False, dispatcher=dispatch_same):
	""" Observer or unobserve traits on an object.
	Parameters
	----------
	object : IObservable
	An object to be observed.
	expression : Expression
	An object describing what traits are being observed.
	handler : callable(event)
	User-defined callable to handle change events.
	``event`` is an object representing the change.
	Its type and content depends on the change.
	remove : boolean, optional
	If true, remove notifiers. i.e. unobserve the traits.
	dispatcher : callable(callable, event)
	Callable for dispatching the user-defined handler, i.e. dispatching
	callback on a different thread.
	"""
	# ``expression`` can be anything with a method ``as_graphs`` that yields
	# ObserverGraph. Hence ObserverGraph can be supplied as ``expression``.
	for graph in expression.as_graphs():
	_add_or_remove_notifiers(
	object=object,
	graph=graph,
	handler=handler,
	target=object,
	dispatcher=dispatcher,
	remove=remove,
	)

i.e. no isinstance(expression, str) anywhere else.

I am sorry that the naming has changed in this other massive branch. ObserverGraph there is ObserverPath here...

[corranwebster]

Yep, that looks right.

[corranwebster]

from traits.trait_base import not_none

[corranwebster]

Similarly, this is available in traits.trait_base, as not_none.

[mdickinson]

It's a bit bike-sheddy, but using a simple lambda v: v is not None seems more readable to me - it's understandable from the immediate context (so the reader has no need to look up the function to check that it's doing what they think it is). The not_none in traits.trait_base feels like clutter.

[kitchoi]

I have been thinking about exposing object to the user as well. So the callable will have a signature callable(HasTraits, str, TraitType). The HasTraits object is available: https://github.com/enthought/traits/pull/942/files#diff-b70bb1540b20e1ffe710b12a7774b069R343-R345

[corranwebster]

This could be useful, but it makes the requirement below of producing the same result more difficult to guarantee.

In particular if you pass the object you could use this for metadata (assuming you aren't already somewhere).

[kitchoi]

Right... in order to cleanup observers/notifiers, the result of this filter for a given set of input should always be the same. Exposing object risks violating this requirement and maybe a bug trap.
Yes I agree this metadata can reuse this filter_ method.

[kitchoi]

Thank you @corranwebster and @mdickinson for your feedback! These will be taken into account in the future PR targeting #1024, and we can continue with more discussion there.
This PR has done its job to prove a concept. I am going to close it now.