Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected failure mode for dispatch="ui" when there is no UI #1732

Closed
corranwebster opened this issue Jan 19, 2023 · 6 comments · Fixed by #1740
Closed

Unexpected failure mode for dispatch="ui" when there is no UI #1732

corranwebster opened this issue Jan 19, 2023 · 6 comments · Fixed by #1740
Assignees
Milestone

Comments

@corranwebster
Copy link
Contributor

When using dispatch="ui" and there is no UI (in particular, Pyface has not done toolkit selection), the dispatcher fails with an attempt to call None as the handler:

>>> from traits.api import HasTraits, Int, observe
>>> class Test(HasTraits):
...    x = Int()
... 
>>> t = Test()
>>> def handler(event):
...     print(event)
... 
>>> t.observe(handler, 'x', dispatch='ui')
>>> t.x = 1 
Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<__main__.Test object at 0x10de8a270>, name='x', old=0, new=1)
Traceback (most recent call last):
  File "/Users/cwebster/.edm/envs/enclosure-38/lib/python3.8/site-packages/traits/observation/_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "/Users/cwebster/.edm/envs/enclosure-38/lib/python3.8/site-packages/traits/trait_notifiers.py", line 50, in ui_dispatch
    ui_handler(handler, *args, **kw)
TypeError: 'NoneType' object is not callable

This also happens when the Pyface toolkit is null (which may be a Pyface bug):

>>> from traits.etsconfig.api import ETSConfig
>>> ETSConfig.toolkit = 'null'
>>> import pyface.api
>>> t.x = 2     
Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<__main__.Test object at 0x10de8a270>, name='x', old=1, new=2)
Traceback (most recent call last):
  File "/Users/cwebster/.edm/envs/enclosure-38/lib/python3.8/site-packages/traits/observation/_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "/Users/cwebster/.edm/envs/enclosure-38/lib/python3.8/site-packages/traits/trait_notifiers.py", line 50, in ui_dispatch
    ui_handler(handler, *args, **kw)
TypeError: 'NoneType' object is not callable

I'm not entirely sure what the correct behaviour is here.

I think I would expect dispatch to work if the current thread is the main thread, given the short-circuit in the code here:

def ui_dispatch(handler, *args, **kw):
if threading.current_thread().ident == ui_thread:
handler(*args, **kw)
else:
ui_handler(handler, *args, **kw)

But given there is no way to dispatch something back to the main thread if there is no event loop around, I'd expect at least a better failure message if the dispatch is not on the main thread.

@corranwebster
Copy link
Contributor Author

Presumably there is a similar issue with on_trait_change-based dispatch:

def dispatch(self, handler, *args):
if threading.current_thread().ident == ui_thread:
handler(*args)
else:
ui_handler(handler, *args)

@mdickinson
Copy link
Member

Drive-by note: threading.current_thread() == threading.main_thread() would be a better test here.

Agreed that there should be a clear failure message if we're not on the main thread and there's no ui. And yes, I suppose we should just ignore dispatch="ui" and execute the handler directly if we're on the main thread. I'm not sure that that's what I'd choose if we were implementing this for the first time, but we're not. (Having an observer that sometimes executes synchronously and sometimes asynchronously sounds a bit like a recipe for confusion.)

@homosapien-lcy
Copy link
Contributor

homosapien-lcy commented Apr 17, 2023

Hi, I think the second exception may have already been solved by the latest pyface:

(py311) (base) cyliu@aus552cyliu traits % python3.11
Python 3.11.2 (main, Feb 16 2023, 03:15:23) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import HasTraits, Int, observe
>>> class Test(HasTraits):
...     x = Int()
... 
>>> t = Test()
>>> from traits.etsconfig.api import ETSConfig
>>> ETSConfig.toolkit = 'null'
>>> import pyface.api
>>> t.x = 2     
>>> 
>>> 

But the first error remains:

Python 3.11.2 (main, Feb 16 2023, 03:15:23) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import HasTraits, Int, observe
>>> class Test(HasTraits):
...     x = Int()
... 
>>> t = Test()
>>> from traits.etsconfig.api import ETSConfig
>>> ETSConfig.toolkit = 'null'
>>> import pyface.api
>>> t.x = 2     
>>> 
>>> def handler(event):
...     print(event)
... 
>>> t.observe(handler, 'x', dispatch='ui')
>>> t.x = 1 
Exception occurred in traits notification handler for event object: TraitChangeEvent(object=<__main__.Test object at 0x1015a8590>, name='x', old=2, new=1)
Traceback (most recent call last):
  File "/Users/cyliu/Documents/3.11_test/traits/traits/observation/_trait_event_notifier.py", line 122, in __call__
    self.dispatcher(handler, event)
  File "/Users/cyliu/Documents/3.11_test/traits/traits/trait_notifiers.py", line 50, in ui_dispatch
    ui_handler(handler, *args, **kw)
TypeError: 'NoneType' object is not callable

Environment: Python3.11, MacOS
Packages:
Package Version Editable project location


attrdict 2.0.1
attrdict3 2.0.2
chaco 5.1.0 /Users/cyliu/Documents/3.11_test/chaco
configobj 5.0.8
enable 5.4.0.dev28 /Users/cyliu/Documents/3.11_test/enable
fonttools 4.39.2
joblib 1.2.0
numpy 1.24.2
pandas 1.5.3
Pillow 9.4.0
pip 22.3.1
PyAudio 0.2.13
pyface 8.0.0
pygarrayimage 1.0
pyglet 2.0.5 /Users/cyliu/Documents/3.11_test/pyglet
Pygments 2.14.0
pyparsing 3.0.9
PySide6 6.4.3
PySide6-Addons 6.4.3
PySide6-Essentials 6.4.3
python-dateutil 2.8.2
pytz 2023.2
reportlab 3.6.12
scikit-learn 1.2.2
scipy 1.10.1
setuptools 65.6.3
shiboken6 6.4.3
six 1.16.0
threadpoolctl 3.1.0
traits 7.0.0.dev1840 /Users/cyliu/Documents/3.11_test/traits
traitsui 7.4.3
wxPython 4.2.0

[notice] A new release of pip available: 22.3.1 -> 23.1
[notice] To update, run: pip install --upgrade pip

@homosapien-lcy
Copy link
Contributor

After some testing, I found the first error from Corran will resolve if the main thread check in the ui_dispatch is changed to the following:

def ui_dispatch(handler, *args, **kw):
    if threading.current_thread().ident == threading.main_thread().ident:
        handler(*args, **kw)
    else:
        ui_handler(handler, *args, **kw)

Then the code will run successfully:

Python 3.11.2 (main, Feb 16 2023, 03:15:23) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import HasTraits, Int, observe
^[[A
>>> 
>>> 
>>> class Test(HasTraits):
...     x = Int()
... 
>>> t = Test()
>>> def handler(event):
...     print(event)
... 
>>> t.observe(handler, 'x', dispatch='ui')
>>> t.x = 1 
TraitChangeEvent(object=<__main__.Test object at 0x104ec0720>, name='x', old=0, new=1)

So is the dispatch supposed to run when there is no ui but on the main thread?

@mdickinson mdickinson added this to the 7.0.0 release milestone Apr 17, 2023
@homosapien-lcy homosapien-lcy self-assigned this Apr 17, 2023
@mdickinson
Copy link
Member

So is the dispatch supposed to run when there is no ui but on the main thread?

Yes, I think that's probably the right behaviour here.

@homosapien-lcy
Copy link
Contributor

So is the dispatch supposed to run when there is no ui but on the main thread?

Yes, I think that's probably the right behaviour here.

OK, I will create a PR for this change then

mdickinson added a commit that referenced this issue Apr 24, 2023
)

Previously the ui_dispatch(handler, *args, **kw) function in
traits/traits/trait_notifiers.py only checks current_thread().ident with
ui_thread. But supposedly, this current thread id should be compared
with the main thread (threading.main_thread()). This discrepancy caused
a no ui failure in ui dispatch as mentioned in issue #1732. This PR
attempts to resolve this failure. Closes #1732

**Checklist**
- [ ] Tests
- [x] Update API reference (`docs/source/traits_api_reference`) (seems
no need to change for this PR)
- [ ] Update User manual (`docs/source/traits_user_manual`) (seems no
need to change for this PR)
- [x] Update type annotation hints in stub files (seems no need to
change for this PR)

---------

Co-authored-by: Chengyu Liu <cyliu@aus552cyliu.local>
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants