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

Ensure pending writes are dispatched in order and only from correct thread #6443

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Mar 5, 2024

As usual with this corner of the code this was a tough one to pin down. Here we had multiple issues involving threaded dispatching. In particular the ChatInterface now uses threads quite extensively, this, combined with the fact that Lumen was incorrectly using the unlocked decorator resulted in threads pushing across the websocket in indeterministic order (particularly when a lot of stuff was pushed at the same time).

To avoid this we do two things:

  1. We keep a global _pending_writes dict ensuring that writes are always processed in the order they were created
  2. We make sure that unlocked does not unlock if we're not on the expected, i.e. usually the main, thread

I still have to confirm that 1. is actually needed, and I think I should also handle potential attempts at writing to a closed session.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 71.55%. Comparing base (6608244) to head (8ba9f99).
Report is 1 commits behind head on main.

Files Patch % Lines
panel/io/document.py 32.00% 17 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6443       +/-   ##
===========================================
- Coverage   82.07%   71.55%   -10.53%     
===========================================
  Files         310      310               
  Lines       45920    45928        +8     
===========================================
- Hits        37691    32863     -4828     
- Misses       8229    13065     +4836     
Flag Coverage Δ
ui-tests ?
unitexamples-tests 71.55% <34.61%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahuang11
Copy link
Contributor

ahuang11 commented Mar 6, 2024

I got hit with this traceback while trying to test out outlines

Traceback (most recent call last):
  File "/Users/ahuang/repos/panel/panel/io/document.py", line 347, in unlocked
    curdoc.unhold()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/document.py", line 776, in unhold
    self.callbacks.unhold()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 431, in unhold
    self.trigger_on_change(event)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 413, in trigger_on_change
    invoke_with_curdoc(doc, invoke_callbacks)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 443, in invoke_with_curdoc
    return f()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 412, in invoke_callbacks
    cb(event)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 276, in <lambda>
    self._change_callbacks[receiver] = lambda event: event.dispatch(receiver)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/events.py", line 353, in dispatch
    super().dispatch(receiver)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/events.py", line 219, in dispatch
    cast(DocumentPatchedMixin, receiver)._document_patched(self)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/server/session.py", line 244, in _document_patched
    raise RuntimeError("_pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes")
RuntimeError: _pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/tornado/ioloop.py", line 738, in _run_callback
    ret = callback()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/tornado/ioloop.py", line 762, in _discard_future_result
    future.result()
  File "/Users/ahuang/repos/panel/panel/io/server.py", line 184, in wrapper
    state._handle_exception(e)
  File "/Users/ahuang/repos/panel/panel/io/state.py", line 447, in _handle_exception
    raise exception
  File "/Users/ahuang/repos/panel/panel/io/server.py", line 182, in wrapper
    return await func(*args, **kw)
  File "/Users/ahuang/repos/panel/panel/param.py", line 855, in _eval_async
    raise e
  File "/Users/ahuang/repos/panel/panel/param.py", line 834, in _eval_async
    async for new_obj in awaitable:
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/reactive.py", line 415, in wrapped
    async for val in evaled:
  File "/Users/ahuang/repos/lumen/lumen/ai/agents.py", line 170, in _render_component
    memory["current_spec"] = spec
  File "/Users/ahuang/repos/lumen/lumen/ai/memory.py", line 45, in __setitem__
    self._update_view(key, value)
  File "/Users/ahuang/repos/lumen/lumen/ai/memory.py", line 70, in _update_view
    accordion[i] = new_item
  File "/Users/ahuang/repos/panel/panel/layout/base.py", line 676, in __setitem__
    self.objects = new_objects
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/parameterized.py", line 525, in _f
    instance_param.__set__(obj, val)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/parameterized.py", line 527, in _f
    return f(self, obj, val)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/parameterized.py", line 1545, in __set__
    obj.param._call_watcher(watcher, event)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/parameterized.py", line 2486, in _call_watcher
    self_._execute_watcher(watcher, (event,))
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/param/parameterized.py", line 2468, in _execute_watcher
    watcher.fn(*args, **kwargs)
  File "/Users/ahuang/repos/panel/panel/reactive.py", line 374, in _param_change
    self._apply_update(named_events, properties, model, ref)
  File "/Users/ahuang/repos/panel/panel/reactive.py", line 301, in _apply_update
    with unlocked():
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/contextlib.py", line 142, in __exit__
    next(self.gen)
  File "/Users/ahuang/repos/panel/panel/io/document.py", line 363, in unlocked
    curdoc.unhold()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/document.py", line 776, in unhold
    self.callbacks.unhold()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 431, in unhold
    self.trigger_on_change(event)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 413, in trigger_on_change
    invoke_with_curdoc(doc, invoke_callbacks)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 443, in invoke_with_curdoc
    return f()
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 412, in invoke_callbacks
    cb(event)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 276, in <lambda>
    self._change_callbacks[receiver] = lambda event: event.dispatch(receiver)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/document/events.py", line 875, in dispatch
    cast(SessionCallbackAddedMixin, receiver)._session_callback_added(self)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/server/session.py", line 261, in _session_callback_added
    self._callbacks.add_session_callback(wrapped)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/server/callbacks.py", line 201, in add_session_callback
    self._group.add_next_tick_callback(callback_obj.callback, callback_obj.id)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/util/tornado.py", line 247, in add_next_tick_callback
    self._assign_remover(callback, callback_id, self._next_tick_callback_removers, remover)
  File "/Users/ahuang/miniconda3/envs/lumen/lib/python3.10/site-packages/bokeh/util/tornado.py", line 207, in _assign_remover
    raise ValueError("A callback of the same type has already been added with this ID")
ValueError: A callback of the same type has already been added with this ID

panel/io/document.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member Author

I got hit with this traceback while trying to test out outlines

Lumen still needs a fix.

@philippjfr
Copy link
Member Author

I have decided that the original fix isn't appropriate, this was a matter of Lumen using the unlocked context manager incorrectly so we should fix it there and issue an error using the logging module that the decorator is being misused.

@philippjfr philippjfr merged commit 385901e into main Mar 6, 2024
11 of 15 checks passed
@philippjfr philippjfr deleted the pending_writes_order branch March 6, 2024 13:04
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 this pull request may close these issues.

None yet

3 participants