-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Queue render state updates on a thread #182
Conversation
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
Should use a ThreadPool so that one users deephaven.ui components doesn't block another users deephaven.ui components. |
Thread pools now exposed to Python: deephaven/deephaven-core#4949 |
b162ed4
to
aecfbcf
Compare
def use_state(initial_value: T) -> (T, Callable[[T], None]): | ||
def use_state( | ||
initial_value: T, | ||
) -> tuple[T, Callable[[StateValue[T]], None]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this seems to imply the following:
foo, set_foo = ui.use_state(None)
#... much later
set_foo(lambda old: do_something_new(old, other_vars))
Did I just assign a function that foo
will now be equal to the next time my component renders? Or did I just pass a function that should be called with the old value as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passed a function that should be called with the old value. If you do want to set a function in a state variable, you need to have it be the return value of an updater function, e.g.
foo, set_foo = ui.use_state(None)
# Need to pass the function back as a result of the updater function
set_foo(lambda old: lambda : print("hello"))
Generally storing a function in a state variable is not a common use case.
- Still need to do rendering on a specific thread, and group set_state calls to a thread as well
- Now updates are all queued on the same thread - Fixes deephaven#116
- Build a CSV loader to show multi-threading example, add use_render_queue hook
- Add doc strings
- Callable takes the old value of the state - Tested using the following component: ``` import deephaven.ui as ui from deephaven.ui import use_state @ui.component def bar(): x, set_x = use_state(0) print(f"Render with x {x}") def handle_press(): # Call set_x twice in the same method, using a callable. This should result in x increasing by 2 each time the button is pressed set_x(lambda old_x: old_x + 1) set_x(lambda old_x: old_x + 1) return ui.action_button( f"x is {x}", on_press=handle_press ) b = bar() ```
- Each ElementMessageStream has it's own render queue - Render queue is processed by calling `submit_task` and queuing a render
- Forgot to set the context to the threads local data
- Add some missing typing in RenderContext - Split up the UpdaterFunction and InitializerFunction for setting state
- Was kind of weird how the initializer function was being handled, this makes it a little more clear
2883d6a
to
bdbfca6
Compare
- Also clean up the typing in use_memo a bit
- Fixes annotations in 3.8
@overload | ||
def use_state( | ||
initial_state: T | InitializerFunction[T] | None = None, | ||
) -> tuple[T | None, Callable[[T | UpdaterFunction[T]], None]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition looks the same as below, why do you need it here also?
The T definitions here in the second one also need to union in None, so that None can be safely passed as a possible value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using @overload
incorrectly - I should only have had to define the two @overload
and then not define typing in the actual implementation: https://docs.python.org/3/library/typing.html#typing.overload
But when I do that, pylance complains about no typings in the use_state
implementation.
In any case, I think I need Python 3.12 to describe the typings correctly using the type parameter syntax on the use_state
functions. I'll just get rid of the overload for now and always allow None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see discussion here about default parameters in generic functions: python/mypy#3737 (comment)
Without type parameter syntax to allow for doing something like x, set_x = use_state[int]()
, which would result in x: int | None
, I think the best we can do for now is use type T
when an initial_value
is passed in, then allow Any
when no initial_value
is passed in.
# Initial value provided, only accepts the type that was initially provided
x, set_x = use_state(4)
set_x(5) # This is valid
set_x("foo") # ERROR
set_x(None) # ERROR
# No initial_value provided, accepts being set to any value
y, set_y = use_state()
set_y(5) # valid
set_y("foo") # valid
set_y(None) # valid
def foo(val: int | None):
# Initial value provided, only accepts the type that was initially provided, including `None`
z, set_z = use_state(val)
set_z(5) # valid
set_z("foo") # ERROR
set_z(None) # valid
print(z)
At least this way we get proper type checking when you start with an initial_value
, and we don't block you from assigning a value if you don't declare an initial_value
.
Filed a ticket to improve typing in 3.12: #208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this makes more sense.
- Allow `Any` if no initial value is passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved api details, would prefer if @jnumainville gives it a +1 too.
Renderer
classuse_render_queue
hook and example for showing how to queue state updates