Error from equality check are swallowed when filtering for trait change events

[kitchoi]

The existing behaviour for on_trait_change (and observe) is that if equality check fails, the error will be swallowed and the change handler is fired. This issue is about whether we should do something else with the exception instead of or in addition to swallowing it.

Currently the relevant code is here, but that is being moved by #1165: https://github.com/enthought/traits/blob/aa6353148570cf4aa49144d7e147529874a3e2d6/traits/ctraits.c#L2366-L2370

Here is the existing behaviour (nothing fails, and nothing is logged):

from traits.api import *

class NotComparable:

    def __eq__(self):
        raise RuntimeError("Sorry!")

class Foo(HasTraits):
    dummy = Any()

def handler(_):
    print("changed")

foo = Foo()
foo.on_trait_change(handler, "dummy")
foo.dummy = NotComparable() # print 'changed'
foo.dummy = NotComparable() # print 'changed'

foo = Foo()
foo.observe(handler, "dummy")
foo.dummy = NotComparable() # print 'changed'
foo.dummy = NotComparable() # print 'changed'

[kitchoi]

Related to this block of code, which is related to this issue: https://github.com/enthought/traits/blob/05cd5206f609cfc6ecbb2f2f74776db5c70d75d7/traits/trait_notifiers.py#L659-L667

Upon running Enable test suite with Python 3.8, I saw deprecation warning on the bool(old != new) line:

test_bounds_default (enable.tests.primitives.test_image.ImageTest) ... /opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/traits/trait_notifiers.py:662: DeprecationWarning: elementwise comparison failed; this will raise an error in the future.
  return bool(old != new)
ok

[kitchoi]

I don't think the deprecation warning deserves a separate issue because it would have been an error in new Python, and the except Exception branch would kick in to maintain the existing behaviour. Just thought this piece of information might be useful.

[mdickinson]

Agreed about the deprecation warning. As an aside, it would be helpful if that warning indicated which error would be raised in the future. The warning's been around a long time, and from some of the NumPy issues, it may be that they end up choosing to return False rather than raise in some cases. Ex: https://github.com/numpy/numpy/issues/10095

Things we might consider for this issue (not all independent):

• always logging (probably just at DEBUG level) when the exception is silenced
• catching only ValueError, and propagating all other exceptions. ValueError is the only actual problematic case we know of that does't relate to a coding error
• propagating all exceptions, and making sure that there are good options out there for cases involving arrays; we likely couldn't do this without at least a deprecation period, and some work to identify the problematic patterns and good solutions for them

[kitchoi]

Perhaps logging is a good first cut? It will allow us to gather more information as to what kind of exceptions are being silenced, then we would be in a better place to decide whether the other options would be an improvement that facilitates most robust code or a nuisance that makes the interface harder to use.

[mdickinson]

Perhaps logging is a good first cut?

Yes, definitely logging this at DEBUG level seems worth doing.