Fix/container gc retention#9918
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
Fixes a widget interactivity regression caused by container-like outputs “freezing” child HTML and dropping strong references to UI elements, allowing them to be garbage collected (and thereby removed from the weak UI registry).
Changes:
- Introduces
ContainerHtmlas a baseHtmlwrapper that retains strong references to childHtmlobjects and re-renders children live via.text. - Refactors
style,accordion,callout,carousel, andsidebarto useContainerHtml(and updates_BlockWrapped), preventing dropped-child GC retention bugs. - Adds regression tests that explicitly verify UI registry retention and live re-render behavior across the affected components.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
marimo/_output/hypertext.py |
Adds ContainerHtml and re-parents _BlockWrapped to retain strong child refs and render live. |
marimo/_plugins/stateless/style.py |
Refactors .style() implementation to a ContainerHtml wrapper so it keeps the wrapped object alive. |
marimo/_plugins/stateless/accordion.py |
Refactors accordion to retain strong refs to rendered tab content/labels and render live. |
marimo/_plugins/stateless/callout.py |
Refactors callout to a ContainerHtml wrapper that retains its child. |
marimo/_plugins/stateless/carousel.py |
Refactors carousel to retain strong refs to items and render live. |
marimo/_plugins/stateless/sidebar.py |
Re-parents sidebar to ContainerHtml and retains its processed item to prevent GC. |
marimo/_plugins/ui/_impl/tabs.py |
Makes tabs retain strong refs to rendered tab contents to avoid registry reaping. |
tests/_output/test_hypertext.py |
Adds targeted tests for ContainerHtml retention + live rendering behavior. |
tests/_plugins/stateless/test_style.py |
Adds regression coverage that .style() keeps UI elements registered after GC. |
tests/_plugins/ui/_impl/test_tabs.py |
Adds regression coverage that mo.ui.tabs keeps UI elements registered after GC. |
tests/_plugins/stateless/test_sidebar.py |
Updates assertions to use .text and adds multiple GC-retention + live-update regression tests. |
tests/_plugins/stateless/test_accordion.py |
Adds GC-retention + live-update regression tests for accordion (incl. lazy). |
tests/_plugins/stateless/test_callout.py |
Adds GC-retention + live-update regression tests for callout. |
tests/_plugins/stateless/test_carousel.py |
Adds GC-retention + live-update regression tests for carousel. |
| # Initialize combined_style with style dict if provided, | ||
| # otherwise empty dict | ||
| combined_style = style or {} | ||
|
|
||
| # Add kwargs to combined_style, converting snake_case to kebab-case | ||
| for key, value in kwargs.items(): | ||
| kebab_key = key.replace("_", "-") | ||
| combined_style[kebab_key] = value | ||
|
|
||
| self._style_str = ";".join( | ||
| [f"{key}:{value}" for key, value in combined_style.items()] | ||
| ) |
|
nvm you already did |
|
|
||
|
|
||
| class _BlockWrapped(Html): | ||
| class ContainerHtml(Html): |
There was a problem hiding this comment.
This is identical to
marimo/marimo/_plugins/stateless/flex.py
Lines 16 to 70 in c34002e
I will consolidate both classes in a follow up. @edwardwkrohne you can ignore this comment.
| # Initialize combined_style with style dict if provided, | ||
| # otherwise empty dict | ||
| combined_style = style or {} | ||
|
|
||
| # Add kwargs to combined_style, converting snake_case to kebab-case | ||
| for key, value in kwargs.items(): | ||
| kebab_key = key.replace("_", "-") | ||
| combined_style[kebab_key] = value | ||
|
|
||
| self._style_str = ";".join( | ||
| [f"{key}:{value}" for key, value in combined_style.items()] | ||
| ) |
There was a problem hiding this comment.
accordion is not a class not a function, so the Returns: part of doc string doesn't make sense here. Please remove this.
Also verify on all functions -> class conversions in the PR.
callout, style, carousel, accordion,
…y; fix of common bug.
…ning of a join call
…out, carousel, and sidebar, all of which are now classes instead of functions.
Bundle ReportChanges will increase total bundle size by 65.9kB (0.26%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
|
|
Thanks @edwardwkrohne I'll merge this |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.15-dev44 |
📝 Summary
Closes #9880
Widgets created as
would never fire their set_state methods. The issue is that
stylereturnswhich drops the actual python object
item, preventing the corresponding frontend ui object from actually interacting with the backend.Now,
styleis a class that derives from a new classContainerHtml, which explicitly contains strong references to a list of children to prevent them from being garbage collected. The offending code survives aswhich uses the explicitly stored child
self._children[0]instead of a local. Similar treatment has been given toaccordion,callout,carousel,tabs, andsidebar, all of which could potentially drop their children by converting to text and losing the original.tabswas slightly different in that it was already aUIElement, which would have been incompatible with the implementation of the newContainerHtmlclass, so it was modified to explicitly store its own children.sidebarwas reparented toContainerHtmland stores its children that way._BlockWrappedwas also reparented toContainerHtml.📋 Pre-Review Checklist
✅ Merge Checklist