feat: Return callables from callables in Deephaven UI #540
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mofojed
requested changes
Jun 13, 2024
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
mofojed
requested changes
Jun 18, 2024
Comment on lines
+149
to
+151
| // Do NOT add anything that logs result | ||
| // It creates a strong ref to the result object in the console logs | ||
| // As a result, any returned callables will never be GC'd and the finalization registry will never clean them up |
There was a problem hiding this comment.
Would love a little blog post about this
There was a problem hiding this comment.
Ya not sure what else would be covered in a blog post. Guess it could be on weakrefs in general
There was a problem hiding this comment.
WeakRef, FinalizationRegistry, garbage collection - would be short and interesting
plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Outdated
Show resolved
Hide resolved
mofojed
approved these changes
Jun 19, 2024
mofojed
added a commit
to mofojed/deephaven-plugins
that referenced
this pull request
Feb 10, 2025
- Client was calling a closeCallable function each time any callable was no longer rendered
- Callables as part of the regular render cycle that didn't change were being recreated every time
- Since it wasn't the same object between renders, closeCallable was getting called for a previous instance of that callable but with the same ID
- Server side was unnecessarily listening for those to close callables as part of the regular render cycle
- Things that are no longer rendered are already cleaned up by the server, don't need to/shouldn't listen to the client for when to clean those up. Only needed for the temporary callables
- Tested by using the snippet from DH-18536:
```python
from deephaven import ui, time_table
@ui.component
def ui_resetable_table():
table, set_table = ui.use_state(lambda: time_table("PT1s"))
handle_press = ui.use_liveness_scope(lambda _: set_table(time_table("PT1s")), [])
return [
ui.action_button(
"Reset",
on_press=handle_press,
),
table,
]
resetable_table = ui_resetable_table()
```
- Also tested using the snippets from previous callable implentation ticket: deephaven#540
```python
from deephaven import ui
@ui.component
def my_comp():
on_press = ui.use_callback(lambda d: print(d), [])
on_press_nested = ui.use_callback(lambda: print, [])
on_press_double_nested = ui.use_callback(lambda: { "nestedFn": print, "some_val": 4 }, [])
on_press_unserializable = ui.use_callback(lambda: set([1, 2, 3]), [])
return [
ui.button("Normal", on_press=on_press),
ui.button("Nested", on_press=on_press_nested),
ui.button("Double Nested", on_press=on_press_double_nested),
ui.button("Not Serializable", on_press=on_press_unserializable)
]
c = my_comp()
```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #322. Will be required for full completion of #321.
Currently a little hard to test, but this should demonstrate it if you watch the JS debug logs on
WidgetHandler.Run this Python code.
Normalbutton should behave the same as latest dh.ui.Nestedbutton will return a callable as the direct result of the callback function. This should allow us to doconst fn = await callable(args);basically. It is not called, but there is a log fromWidgetHandlerafter 5-10s that it was cleaned up. Or you can use theMemorytab in dev tools and click the broom icon to trigger a manual GC.Double Nestedbutton will return and wrap an object containing a new callable. You can filter the browser logs forWidgetUtilsand look forcallable ID result stringwhich won't have the parsed function, but will contain the object representing it as well as the other parts of the returned object.Not serializablebutton will basically do nothing except log an error in the Python console. The returned result will include that it was a serialization error to JS, but we do nothing with it.