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

Updating express inputs leads to an error #1034

Closed
cpsievert opened this issue Jan 19, 2024 · 6 comments · Fixed by #1049
Closed

Updating express inputs leads to an error #1034

cpsievert opened this issue Jan 19, 2024 · 6 comments · Fixed by #1049
Labels
Milestone

Comments

@cpsievert
Copy link
Collaborator

Consider the following app, which from a user perspective is functional, but does generate an error (since the 1st time it runs a session hasn't been established):

from shiny import reactive
from shiny.express import ui

ui.input_slider("n", "N", 1, 100, 50)

@reactive.effect
def _():
    ui.update_slider("n", value=10)
Traceback (most recent call last):
  File "/Users/cpsievert/github/py-shiny/shiny/reactive/_reactives.py", line 573, in _run
    await self._fn()
  File "/Users/cpsievert/github/py-shiny/shiny/_utils.py", line 243, in fn_async
    return fn(*args, **kwargs)
  File "/Users/cpsievert/github/py-shiny/sandbox/app.py", line 8, in _
    ui.update_slider("n", value=10)
  File "/Users/cpsievert/github/py-shiny/shiny/ui/_input_update.py", line 780, in update_slider
    session = require_active_session(session)
  File "/Users/cpsievert/github/py-shiny/shiny/session/_utils.py", line 127, in require_active_session
    raise RuntimeError(
RuntimeError: update_slider() must be called from within an active Shiny session.
/Users/cpsievert/github/py-shiny/shiny/reactive/_reactives.py:555: ReactiveWarning: Error in Effect: update_slider() must be called from within an active Shiny session.
  await self._run()
@wch wch added this to the v0.7.0 milestone Jan 19, 2024
@wch wch mentioned this issue Jan 19, 2024
55 tasks
@wch
Copy link
Collaborator

wch commented Jan 19, 2024

This is a real issue, but I will note that in practice, most uses will only call the update_* function after something else has happened. For example, it might use reactive.event. The example below will not throw an error:

from shiny import reactive
from shiny.express import input, ui

ui.input_slider("m", "M", 1, 100, 10)
ui.input_slider("n", "N", 1, 100, 50)


@reactive.effect
@reactive.event(input.m)
def _():
    ui.update_slider("n", value=input.m())

@cpsievert
Copy link
Collaborator Author

That's true, but it's also not great that if you try use any session methods, you get an unhelpful error. For example:

from shiny import reactive
from shiny.express import session

@reactive.effect
async def _():
    await session.send_custom_message("msg", {})
Error in Effect: '_MockSession' object has no attribute 'send_custom_message'

You also can't avoid the error with something like if session: since the mock session object is truthy

@jcheng5
Copy link
Collaborator

jcheng5 commented Jan 22, 2024

I think it's extremely important that @reactive.effect NOT run at all if it's created during the UI-generating phase of Express. We could run the UI generating pass with a MockSession in place and .close() it as soon as the UI returns, which would auto-destroy any reactive effects.

@jcheng5
Copy link
Collaborator

jcheng5 commented Jan 22, 2024

I see now that MockSession doesn't have .on_ended, it would need that too.

@wch
Copy link
Collaborator

wch commented Jan 22, 2024

Just to elaborate a bit on what @jcheng5 said about it being important that effects not run at all: I think the specific issue with session stuff being accessed is a manifestation of a more general issue, which is that a copy of these effects are created outside of the session, and they will try to execute, which the user probably will not expect.

@wch
Copy link
Collaborator

wch commented Jan 23, 2024

A problem with calling .close() is that it's async, which means that the caller, wrap_express_app, would also need to be async -- but that won't work because that function is called when you simply access the shiny.express.app.xxx, via a __getattr__ method, and that can't be async.

So I think we may need to change @reactive.effect so that it simply no-ops when running in the UI-generating phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants