-
Notifications
You must be signed in to change notification settings - Fork 892
fix: UI element interaction should respect overridden defs in embedded apps #7889
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
Conversation
This change fixes a bug in which cells run as a consequence of UI element interactions did not respect overriden definitions in embedded apps. This fix adds internal APIs: * check if the current runtime is embedded * get cells overriden by the passed-in definititions This fix also fixes a bug in which nested contexts had isolated AppKernelRunner registries; instead, contexts now share their parents registry to ensure that the registry to make sure runner retrieval can happen at runtime, in a nested context. The registries are still isolated across run sessions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
a1ef116 to
c0608d5
Compare
|
@yairchu I believe this change fixes your minimal example, thank you for debugging and brainstorming with me. Could you try this change out and see if it feels correct? I am not sure if it fixes the "locking" in your original app, I haven't tried that out |
|
@akshayka this does solve the problem of overridden defs! However a problem of strangely resetting sliders does remain in this example:
import marimo
__generated_with = "0.19.4"
app = marimo.App(width="full")
@app.cell
def _():
import marimo as mo
return
@app.cell
def _():
import embed_test_b
return (embed_test_b,)
@app.cell
def _(embed_test_b):
clone_a = embed_test_b.app.clone()
return (clone_a,)
@app.cell
async def _(clone_a):
embed_a = await clone_a.embed()
embed_a.output
return (embed_a,)
@app.cell
def _(embed_test_b):
clone_b = embed_test_b.app.clone()
return (clone_b,)
@app.cell
async def _(clone_b, embed_a):
(
await clone_b.embed(
defs={
"slider": None,
"value": embed_a.defs["value"],
"label": "their",
"kind": "warn",
}
)
).output
return
@app.cell
def _():
return
if __name__ == "__main__":
app.run()
import marimo
__generated_with = "0.19.2"
app = marimo.App(width="full")
@app.cell
def _():
import marimo as mo
import time
return (mo,)
@app.cell
def _():
state = None
return (state,)
@app.cell
def _(mo):
slider = mo.ui.slider(0, 10, 1, 3, label="A slider")
return (slider,)
@app.cell
def _(slider):
slider
return
@app.cell
def _(slider, state):
value = slider.value if state is None else state()
label = "our"
return label, value
@app.cell
def _():
kind = "info"
return (kind,)
@app.cell
def _(kind, label, mo, value):
mo.callout(
mo.md(f"""
The value of *{label}* slider is **{value}**!
"""), kind
)
return
@app.cell
def _():
return
if __name__ == "__main__":
app.run()The slider seems to be "reseting" or recreating constantly when moved. Try to move the slider and notice how the value jumps between the dragging value and the initial value of 3. |
|
Thanks so much @yairchu for trying it out, and for sharing this example — it's very helpful. I can see the incorrect behavior in the underlying code (slider in |
|
@akshayka, if it helps, I think that it is somehow related to the undisplayed slider being recreated in the second instance of What leads me to think this:
|
This fixes a bug in app.clone() in which the CellImpl was shallow copied; CellImpl has mutable state, and should not be shallow copied.
|
@yairchu, thanks so much! I pushed up additional changes, and the slider no longer resets. Want to give it one more try? cloned_embed.mp4 |
|
@akshayka works great now! |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.19.5-dev18 |
…d apps (#7889) fix: UI element interactions when defs are overridden This change fixes a bug in which cells run as a consequence of UI element interactions did not respect overridden definitions in embedded apps. This fix adds internal APIs: * check if the current runtime is embedded * get overrides of an app This fix also fixes a bug in which nested contexts had isolated `AppKernelRunner` registries. With this change, contexts now share their parent's registry to make sure runner retrieval can happen at runtime, in a nested context. The registries are still isolated across run sessions. Finally, it fixes a bug in `App.clone`, in which `CellImpl`s were shallow copied even though they have mutable state. Fixes #7685
…d apps (marimo-team#7889) fix: UI element interactions when defs are overridden This change fixes a bug in which cells run as a consequence of UI element interactions did not respect overridden definitions in embedded apps. This fix adds internal APIs: * check if the current runtime is embedded * get overrides of an app This fix also fixes a bug in which nested contexts had isolated `AppKernelRunner` registries. With this change, contexts now share their parent's registry to make sure runner retrieval can happen at runtime, in a nested context. The registries are still isolated across run sessions. Finally, it fixes a bug in `App.clone`, in which `CellImpl`s were shallow copied even though they have mutable state. Fixes marimo-team#7685
fix: UI element interactions when defs are overridden
This change fixes a bug in which cells run as a consequence of UI
element interactions did not respect overridden definitions in
embedded apps.
This fix adds internal APIs:
This fix also fixes a bug in which nested contexts had isolated
AppKernelRunnerregistries. With this change, contexts now sharetheir parent's registry to make sure runner retrieval can happen at runtime,
in a nested context. The registries are still isolated across run sessions.
Finally, it fixes a bug in
App.clone, in whichCellImpls were shallow copied even though they have mutable state.Fixes #7685